This would have been a nice optimization if it had worked, but (in
addition to leaving garbage directories lying around sometimes if the
process was killed), it appears to have some unintended consequences for
meteorNpm.rebuildIfNonPortable and related functions, since the garbage
directories are easily confused for npm package directories.
Example stack trace:
Error: ENOENT: no such file or directory, open '/home/travis/build/meteor/galaxy-server/node_modules/heapdump-garbage-1c2jqib/package.json'
at Error (native)
=> awaited here:
at Promise.await (/home/travis/.meteor/packages/less/.2.7.9.9fh5c1++os+web.browser+web.cordova/plugin.compileLessBatch.os/npm/node_modules/meteor/promise/node_modules/meteor-promise/promise_server.js:39:12)
at copyFileHelper (/tools/fs/files.js:633:6)
at Object.files.cp_r (/tools/fs/files.js:532:7)
at /tools/isobuild/meteor-npm.js:393:11
at Array.forEach (native)
at copyNpmPackageWithSymlinkedNodeModules (/tools/isobuild/meteor-npm.js:386:29)
at /tools/isobuild/meteor-npm.js:325:5
at Array.forEach (native)
at Object.rebuildIfNonPortable (/tools/isobuild/meteor-npm.js:315:17)
at NodeModulesDirectory.rebuildIfNonPortable (/tools/isobuild/bundler.js:273:22)
at /tools/isobuild/compiler.js:650:13
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`.
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:
* d49f3e2704
* 3257bafc84
* 74cb8ebdc2
* 5bbdcc9baa
* 6a0767bbac
* Improve `fs-extra.move` calls for Windows platform.
This is a follow-up to meteor/meteor#8491 which worked properly on Unix
platforms, but failed in a variety of ways on Windows due to its lack
of Fiber-awareness and desire to create symlinks as unprivileged users
(something not always possible on Windows).
The Fiber issue was observed when trying to remove "src" directories
within the `move` function (which tries a variety of OS/OS/arch-specific
techniques to accomplish its goal) after they had been copied to "dest".
On Windows, this resulted in `EDIRNOTEMPTY` errors since Windows appears
to temporarily cache the file-handle or doesn't release the file-handle
until the next tick.
The symlink issue will hopefully improve in an upcoming release of
Windows (Creator Edition) when Microsoft makes it possible to create
symlinks as an unprivileged user, however it will still require enabling
"Developer" mode in Windows settings. This implements the same catch
which was already in place for `fs.rename` on the `fs.move` provided by
`fs-extra`.
Performance gains were the same in tests comparing before and after
these changes.
Relates to:
https://github.com/meteor/meteor/issues/8558#issuecomment-291194385
* A few code-cleanups to my original commit.
The `wrapFsFunc` function accepts an array of indexes indicating which
arguments are paths. This is particularly important on Windows, due to
the path-conversation which takes place on those strings.
The docs say:
> Indices of arguments that have paths, these arguments will be
> converted to the correct OS slashes
This follows up on the change made from meteor/meteor#8491 which failed
in our release pipeline when publishing for the Windows architecture
for `1.4.4-rc.3`.
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.
Windows has no concept of the executable bit so it is not applied by the
`fstream` `Reader` when building the tarball which is used in both
`meteor build` and `meteor deploy`. For Windows users, this causes
important scripts (such as `node-pre-gyp`) to not be executable when
the bundles are deployed to Unix platforms (such as Galaxy).
To avoid giving every file executable bits, this applies an executable
bit to the file only if it has read permission (something Windows _is_
aware of) and if it is in a location that Node bin links are typically
placed, the `/node_modules/.bin/` directories.
The options.transformFilename function is only supposed to transform
target file names, not the names of source files to be copied.
This behavior was broken by c5809a4a1c
between Meteor 1.4.0.1 and 1.4.1. Thank goodness for `git bisect`!
The biggest symptom of this mistake was that `meteor create --package`
no longer created files based on the ~fs-name~.js and ~fs-name~-tests.js
template files.
This probably merits a 1.4.1.4 release in addition to 1.4.2.2.
Set METEOR_DISABLE_FS_FIBERS=false to un-disable files.* fibers if you
suspect this commit has introduced a problematic difference in behavior.
cc @veered
It's a shame that Pathwatcher issues this warning using console.error,
without taking any verbosity options into account:
https://github.com/atom/node-pathwatcher/blob/7ef76e5dfd/src/main.coffee#L53
Fortunately, I believe I've identified the underlying reason why this
happens, which may help resolve the following issue:
https://github.com/atom/node-pathwatcher/issues/98
If all goes well, I'll submit an upstream pull request.
I've also reinstated an old file watching test that I mistakenly removed
when I attempted to switch to chokidar instead of pathwatcher.
This is a bit different from the previous strategy of invalidating
optimistic functions for specific npm package names.
Now, whenever we make changes to the contents of a specific node_modules
directory, or whenever the developer independently modifies an app's
node_modules directory, all optimistic results derived from paths
contained within that node_modules directory will be marked as dirty, and
thus may need to be recomputed.
This strategy prioritizes starting fewer watchers (just one per
node_modules directory) while still allowing npm packages to be added or
removed while the app is running:
https://github.com/meteor/meteor/pull/7668#issuecomment-255120373
The drawback is that changes within subdirectories of node_modules will
not be detected until the server is fully restarted, but that seems like
an acceptable tradeoff, since npm packages change much less often than
application code.
If a test process does not explicitly call process.exit, pathwatcher
watchers may keep it alive indefinitely (either that, or there's a bug
with the persistent:false option to fs.watchFile).
This accidental immortality can be prevented by explicitly closing all
watchers when we no longer have any interest in file change notifications.
In particular, optimisticReaddir was getting called before relevant dirty
callbacks were firing, and thus failing to notice actual changes on the
file system.
In general, the tools/fs/watch.js code is the one place where we really
need an up-to-date view of the file system. Put another way, if optimistic
functions worked perfectly, we wouldn't need to rely so much on WatchSet
logic, but for now it's a balance of equally important strategies, and we
shouldn't be compromising one by intermingling it with the other.
This partially reverts commit 96c95629eb.
When running `meteor update`, we call writeReleaseFileAndDevBundleLink
immediately before reading .meteor/release, so there's no time for a file
change notification to invalidate the cached contents of that file.
In the future, we could perhaps call optimisticReadFile.dirty(path) as a
consequence of files.writeFile(path, ...), but that may be tricky.
Judging from the variety and extent of test failures, switching to
chokidar.watch was too drastic a change for this late-beta stage of the
release cycle.
The problem with pathwatcher.watch was that watches don't survive the
deletion of the watched file, because (like fs.watch) it watches files
based on inodes, not paths. This problem can be solved in a relatively
narrow way, by attempting to rewatch the file after any "delete" or
"rename" events.
Especially on Linux, chokidar.watch tends to throw ENOSPC errors when too
many files are being watched.
I removed this logic earlier because I thought chokidar could handle such
failures by itself, but that was wishful thinking.
Healthy competition among fs.watch wrappers appears to have produced a
clear winner: https://www.npmjs.com/package/chokidar
This wrapper is better for Meteor than pathwatcher was, because it can
watch directory trees recursively, and it has no trouble watching
nonexistent file paths, whereas pathwatcher would throw an exception.
As of optimism@0.3.0, the value of `this` within subscribe functions is no
longer the optimistic wrapper function object (instead: null or global, as
if called with no receiver object), so we need to retain a reference to
the optimistic function so that the watcher can call .dirty(...args).
I originally added the exception-caching functionality in order to avoid
calling files.stat repeatedly for files known to be missing, but now that
we're using statOrNull, knowledge of missing files (as indicated by
statOrNull returning null) is being properly cached.
The reason it's dangerous to cache exceptions is that (for example) when
an ENOENT exception indicates the file is missing, there will be no more
change events for that file, effectively making the exception permanent,
even if the file comes to exist at a later time.
