Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes 192: Use file.copy for cross-disk flexibility in HYDAT download #205

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mpdavison
Copy link

Replaced file.rename with file.copy to handle situations where the temp folder and the destination are on different physical disks (e.g. Docker container with different mount points for the tempdir and the destination).

Replaced file.rename with file.copy to handle situations where the temp folder and the destination are on different physical disks (e.g. Docker container with different mount points for the tempdir and the destination).
@boshek
Copy link
Collaborator

boshek commented Sep 29, 2024

Hi there. Thanks for the contribution.

Can you please provide an example where this is an issue? Also how do you think that we might test this?

@boshek
Copy link
Collaborator

boshek commented Sep 29, 2024

Actually testing this could an issue which is my fault for including too much into that download function. I've made #206 for that.

But I would really like to see how this actually fails. So if you could provide a Dockerfile that can actually trigger this issue that would be great.

@mpdavison
Copy link
Author

This issue occurs when files are being renamed across different partitions. In my scenario, the rename action fails due to a cross-device link error. This is a limitation of the function, as detailed in the docs for file.rename. Though not as efficient, switching to file.copy avoids this by simply copying files between devices.

Though it's probably possible to write a test for this, it might not be appropriate.

After reading through those docs, I see that I need to update the call to file.copy. I should pass overwrite = TRUE, otherwise downloading a new version of hydat wouldn't actually overwrite the old version. I'll update the PR.

One-liner that reproduces the issue with docker:

$ docker run \
  -v /tmp/some_folder:/root \
  rocker/r-ver \
  Rscript -e 'install.packages("tidyhydat"); library(tidyhydat); download_hydat(ask = FALSE)'

<snip />

✔ Downloading HYDAT to /root/.local/share/tidyhydat
• Downloading new version of HYDAT created on 2024-07-18
  |======================================================================| 100%
• Extracting HYDAT
✖ HYDAT not successfully downloaded
Error in hy_src(hydat_path) : 
  No Hydat.sqlite3 found at /root/.local/share/tidyhydat. Run download_hydat() to download the database.
Calls: download_hydat -> hy_check -> hy_src
In addition: Warning message:
In file.rename(list.files(tempdir, pattern = "\\.sqlite3$", full.names = TRUE),  :
  cannot rename file '/tmp/RtmplUpKBw/extracted/Hydat.sqlite3' to '/root/.local/share/tidyhydat/Hydat.sqlite3', reason 'Invalid cross-device link'
Execution halted
$

One-line that completes successfully using the branch:

$ docker run \
  -v /tmp/some_folder:/root \
  rocker/r-ver \
  Rscript -e 'install.packages("devtools"); devtools::install_github("thompson-aquatic/tidyhydat", ref="enhance/flexible-hydat-file-copy"); library(tidyhydat); download_hydat(ask = FALSE)'

<snip />

✔ Downloading HYDAT to /root/.local/share/tidyhydat
• Downloading new version of HYDAT created on 2024-07-18
  |======================================================================| 100%
• Extracting HYDAT
★ HYDAT successfully downloaded
$

R/download.R Show resolved Hide resolved
@boshek
Copy link
Collaborator

boshek commented Oct 1, 2024

Ok this is awesome. Thanks for this contribution. I am going to try to fix up a couple things before I ship this to CRAN. LMK if you are needing a CRAN release soon for your use cas.e

@boshek
Copy link
Collaborator

boshek commented Oct 1, 2024

Can also please add a NEWS item and your handle for attribution?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants