-
Notifications
You must be signed in to change notification settings - Fork 4
If fs.renameSync fails, try fs.copySync and fs.unlinkSync instead #13
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
Conversation
fs.renameSync fails (e.g. because source and destination files a…|
Hello @MillironX @emiller88 ! Would you have a chance to take a look at this? |
edmundmiller
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm trying to think of things this could break...
Maybe we could hide it behind a variable?
I guess worst case, we can roll it back. I'm fine with it, but let's wait for @MillironX!
|
I recently joined the nf-core org and am a new face in this repo, but I disagree with merging these changes. It adds a lot of lines of code to deal with an unexpected interaction with a not very commonly used action (maximize-build-space only has 205 stars). Also, I think it should be on easimon/maximize-build-space to ensure that their action doesn't break common commands like |
|
@emiller88: Thanks for the positive feedback! @kenibrewer: I guess I should have better explained what the root cause of the issue is, namely that As a side note, what
I'd argue that providing a failover mechanism for errors that your users are encountering is not added complexity, but just graceful handling. - fs.renameSync(nf_dl_path, nf_path)
+ try {
+ fs.renameSync(nf_dl_path, nf_path)
+ } catch (err: unknown) {
+ core.debug(`Failed to rename file: ${err}`)
+ fs.copyFileSync(nf_dl_path, nf_path)
+ fs.unlinkSync(nf_dl_path)
+ }
Star count is not a good metric for how useful a tool is. That aside, I'm actually surprised that I'm the first to report this issue, since the In particular, I use it to test Nextflow pipelines which require large Docker images (e.g. the nvcr.io/nvidia/pytorch easily use up 10GB of disk space) and which require large test datasets. As such, I'd be surprised if I'm the first one to encounter this issue. A potential workaround would be to just download Nextflow the old fashion way on github CI (i.e.
I don't think this is possible. The "EXDEV: cross-device link not permitted" is an error inherent to I hope this addresses any concerns you may have, @kenibrewer. If you feel my proposed solution could be improved upon, I'd be happy to incorporate your suggestions in my PR. |
I agree, I was more afraid of silent errors because it was handled. 😆 Do you have an example of it in use? Sorry, I forgot to kick off the tests, let's see if they all pass! |
|
@rcannood Thanks for the explanation, and I apologize for being a bit too harsh on your pull request when I didn't fully understand the context behind the changes. Everything you've explained makes sense to me and your solution seems like the best course forward to me too. Cheers! |
Issue
When
setup-nextflowis ran aftereasimon/maximize-build-space, thefs.renameSyncfails.In github action:
Results:
Changes
fs.renameSyncfails (e.g. because source and destination files are on different partitions), tryfs.copySyncandfs.unlinkSyncinstead.