Recommendation to switch fs-extra for an alternate implementation.
#8569
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…n docker builds. (meteor#8491)" Unfortunately, `fs-extra` is still not as perfect at handling various file system conditions as would be ideal. It seemed sensical to try and use a library like this however, it turns out that the Meteor suite of file system functions stands up best on Windows, which is where I encountered most problems. For example, `fs-extra` still tries to create symlinks as an unprivileged user – a forbidden task on Windows unless running as Administrator. In addition, I ran into a constant stream of other errors: `ENOTEMPTY`, `EBUSY`, `EEXIST` – all for various reasons. My current recommendation is that we remove `fs-extra` and replace the `Builder#complete` `renameDirAlmostAtomically` call (which does not absolutely _have_ to be done atomically) with a `try`/`catch` which resorts to a basic copy if `err.code === 'EXDEV'`. All other functionality stays the same. This reverts commits: * d49f3e2 * 3257baf * 74cb8eb * 5bbdcc9 * 6a0767b
This operation does not have to be done atomically and this should allow cross-device copies to be made if necessary.
abernix
commented
Apr 6, 2017
tools/isobuild/builder.js
Outdated
| @@ -687,7 +687,16 @@ Previous builder: ${previousBuilder.outputPath}, this builder: ${outputPath}` | |||
| // case of renameDirAlmostAtomically since that one is constructing files to | |||
| // be checked in to version control, but here we could get away with it. | |||
| if (this.buildPath !== this.outputPath) { | |||
| files.renameDirAlmostAtomically(this.buildPath, this.outputPath); | |||
| try { | |||
| files.rename(this.buildPath, this.outputPath); | |||
There was a problem hiding this comment.
This portion was just a proposal. This is failing because the renames and rm_recursives from renameDirAlmostAtomically are necessary to make way for the incoming tree.
As a thought, maybe we should continue to use renameDirAlmostAtomically here instead of rename, and just catch the EXDEV.
The `rename` is not sufficient on its own since it won't pave the way for the new directory by ensuring that it's empty first. `renameDirAlmostAtomically` will do that.
This also makes it possible to disable it in places where we were not checking against the `METEOR_DISABLE_FS_FIBERS` env. variable, like in `rm_recursive`.
benjamn
reviewed
Apr 6, 2017
tools/isobuild/builder.js
Outdated
| } catch (e) { | ||
| if (e.code === "EXDEV") { | ||
| files.rm_recursive(this.outputPath); | ||
| files.cp_r(this.buildPath, this.outputPath); |
There was a problem hiding this comment.
Let's move this logic into files.renameDirAlmostAtomically, as discussed offline.
...Versus reassigning them immediately after. (Whitespace-only view is recommended, due to indentation change)
This logic is now handled in `files.renameDirAlmostAtomically`.
benjamn
approved these changes
Apr 6, 2017
benjamn
reviewed
Apr 6, 2017
tools/fs/files.js
Outdated
|
|
||
| // ... and take out the trash. | ||
| if (cleanupGarbage) { | ||
| files.rm_recursive(garbageDir); |
There was a problem hiding this comment.
Remember that we could do this cleanup asynchronously.
There was a problem hiding this comment.
Yup, thanks! Done with ae35f13!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This proposes reverting/replacing #8491, which has been problematic in the 1.4.4 series of RCs, specifically on Windows. More so, I believe that
fs-extracould be overboard and we could go with a much more simple implementation to accomplish the goal of fixing cross-device copies.Unfortunately,
fs-extrais still not as perfect at handling various file system conditions as would be ideal. It seemed sensical to try and use a library like this, however it turns out that the Meteor suite of file system functions performs better on Windows. For example,fs-extrastill tries to create symlinks as an unprivileged user – a forbidden task on Windows unless running as Administrator.In addition, I ran into a constant stream of other errors:
ENOTEMPTY,EBUSY,EEXISTandEPERM(on anopen, no less) – all for various, Windows-only (from what I can gather) reasons.My current recommendation is that we remove
fs-extraand replace theBuilder#completerenameDirAlmostAtomicallycall (which does not absolutely have to be done atomically; "fast" is the goal) with atry/catchwhich resorts to a basic copy iferr.code === 'EXDEV'. All other functionality stays the same.I've crudely implemented it with 523c7ce but would be open to other suggestions.
This reverts commits:
/cc @jshimko