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

Be more understanding of Windows' filesystem limitations. #8500

Merged
merged 3 commits into from
Mar 22, 2017

Conversation

abernix
Copy link
Contributor

@abernix abernix commented Mar 20, 2017

Presently, the renaming of directories that are in-use will fail on Windows. This is already compensated for when process.platform is
set to win32. However, within BashOnWindows/WSL (Windows Subsystem for Linux), process.platform is equal to linux, though the underlying filesystem is still the same and subject to similar limitations.

Microsoft has stated that it is unlikely that they will remove "Microsoft" from the os.release() value so this checks for that in a similar fashion to how we used to check for process.platform === "win32". Also, this change causes EACCES to be tolerated in the same was as EPERM was already. (EACCES is another type of permission error)

I would point out that this change might contribute to breaking atomicity in files.renameDirAlmostAtomically since it uses files.rename but I'd argue that it was already jeopardized (on Windows and only on Windows) with the previous exception and this change simply provides consistent behavior across Windows implementations.

Probably worth mentioning that this seems to make Meteor work under BashOnWindows on the latest Windows Insider Build (15058) which I believe is a release candidate for Windows Creators Update which is expected to come out later this month, but I'm hesitant to suggest this constitutes support for BashOnWindows (like #7583).

Hopefully issues like this are fixed by microsoft/WSL#1529 and microsoft/WSL#1420, but I'm not sure what the timeline on those getting resolved is.

@abernix
Copy link
Contributor Author

abernix commented Mar 20, 2017

Interestingly, (and frustratingly!) I had to commit ccded04 in order to bust the git submodule caching on this as CircleCI insisted on running the tests against the Git submodule of @sethmurphy18 (through no fault of his own), seen in the CircleCI results here.

(We had intentionally set the submodule to his repository during #8378 testing)

@eagerestwolf
Copy link

Yeah, sorry about that. I forgot to clear the git submodule cache before recommiting. I probably should have disabled my Circle caching during testing as well...or just used a different branch. I can relate to the Windows limitations though because I use Windows myself and meteor is slow and sometimes doesn't work correctly, no fault of Meteor there, just Microsoft. Gotta love em.

// https://github.com/Microsoft/BashOnWindows/issues/423#issuecomment-221627364
var isWindowsLikeFilesystem = function () {
return process.platform === "win32" ||
(process.platform !== "win32" && os.release().indexOf("Microsoft") > -1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplify this?

Presently, the renaming of directories that are in-use will fail on
Windows.  This is already compensated for when `process.platform` is
set to `win32`.  However, within BashOnWindows/WSL (Windows Subsystem
for Linux), `process.platform` is equal to `linux`, though the
underlying filesystem is still the same.

Microsoft has stated that it is unlikely that they will remove
`Microsoft` from the `os.release()` value so we check for that.
@abernix abernix merged commit 33567f8 into meteor:devel Mar 22, 2017
@abernix abernix deleted the abernix/wsl-fs-fix branch March 22, 2017 19:06
@abernix
Copy link
Contributor Author

abernix commented Mar 22, 2017

Force merged (despite the failing tests) which failed because:

  1. The dev_bundle needing to be rebuilt on devel (already, thanks to use fs.move() from fs-extra to fix EXDEV cross device error in docker builds (issue 7852) #8491)
  2. The submodule commit above which was necessary to include meteor/blaze@0aed367.

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.

3 participants