From 42983d70631341f5e7aa75b0f283fe6b1743af21 Mon Sep 17 00:00:00 2001 From: Matt Date: Thu, 22 Mar 2018 17:18:34 -0400 Subject: [PATCH 1/3] Removed Fiber import from oauth package. With commit 857edc2, Fiber is no longer used in oauth server so it is no longer necessary to import. --- packages/oauth/oauth_server.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/oauth/oauth_server.js b/packages/oauth/oauth_server.js index 0df5ec978a..b3cde82f33 100644 --- a/packages/oauth/oauth_server.js +++ b/packages/oauth/oauth_server.js @@ -1,4 +1,3 @@ -var Fiber = Npm.require('fibers'); var url = Npm.require('url'); OAuth = {}; From e0c800587c956b83d469b6e6b88c84052dc75612 Mon Sep 17 00:00:00 2001 From: Matt Date: Thu, 22 Mar 2018 17:19:24 -0400 Subject: [PATCH 2/3] Bump oauth Meteor package version to 1.2.3. --- packages/oauth/package.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/oauth/package.js b/packages/oauth/package.js index baae0d24ee..f96f76920e 100644 --- a/packages/oauth/package.js +++ b/packages/oauth/package.js @@ -1,6 +1,6 @@ Package.describe({ summary: "Common code for OAuth-based services", - version: "1.2.2" + version: "1.2.3" }); Package.onUse(function (api) { From 22e3f995652c8c81e3cc25965d1882834d3ad9dc Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Thu, 22 Mar 2018 21:43:16 -0400 Subject: [PATCH 3/3] Fix copying/symlinking of node_modules directories during build. The bulk of this commit implements `builder.copyNodeModulesDirectory` to allow more than one `node_modules` directory to be copied to the same destination with as much safe symlinking as possible. However, the crux of the fix for #9738 is the removal of the call to `builder.generateFilename`, which deserves additional explanation. If multiple directories are copied to the same output path by the builder, in some cases it makes sense to ensure distinct directory names by adding numeric suffixes to some of the directories. In general, `builder.generateFilename` can get away with this renaming only if the exact names of the directories are an implementation detail. However, the code changed by this commit was altering the names of `node_modules` directories whenever a package had both an `Npm.depends` and a local `node_modules` directory. Not only is it totally invalid to change the name of a `node_modules` directory, but there is also no harm in copying the contents of multiple `node_modules` directories into one final directory called `node_modules`. Should fix #9738. --- tools/isobuild/builder.js | 92 ++++++++++++++++++++++--- tools/isobuild/bundler.js | 2 +- tools/isobuild/isopack.js | 2 +- tools/isobuild/unibuild.js | 6 +- tools/tests/old/test-bundler-options.js | 67 ++++++++++++------ 5 files changed, 133 insertions(+), 36 deletions(-) diff --git a/tools/isobuild/builder.js b/tools/isobuild/builder.js index b5d3f573cb..fdb5973fba 100644 --- a/tools/isobuild/builder.js +++ b/tools/isobuild/builder.js @@ -390,6 +390,56 @@ Previous builder: ${previousBuilder.outputPath}, this builder: ${outputPath}` return generated; } + // A version of copyDirectory that works better for copying node_modules + // directories when symlinks are involved. + copyNodeModulesDirectory(options) { + assert.strictEqual(files.pathBasename(options.from), "node_modules"); + assert.strictEqual(files.pathBasename(options.to), "node_modules"); + + if (options.symlink) { + // If we're going to use symlinks to speed up this copy, then we + // need to make sure we've reserved all directories that are not + // package directories, such as the node_modules directory itself, + // as well as node_modules/meteor and the parent directories of any + // scoped npm packages. + this._ensureAllNonPackageDirectories( + files.realpath(options.from), + options.to + ); + } + + // Call this._copyDirectory rather than this.copyDirectory so that the + // subBuilder hacks from Builder#enter won't apply a second time. + return this._copyDirectory(options); + } + + _ensureAllNonPackageDirectories(absFromDir, relToDir) { + const dirStat = optimisticStatOrNull(absFromDir); + if (! (dirStat && dirStat.isDirectory())) { + return; + } + + const absFromPackageJson = + files.pathJoin(absFromDir, "package.json"); + + const stat = optimisticStatOrNull(absFromPackageJson); + if (stat && stat.isFile()) { + // If the directory has a package.json file, it's a package + // directory, and we should not call this._ensureDirectory, so that + // the package directory can later be symlinked in copyDirectory. + return; + } + + this._ensureDirectory(relToDir); + + optimisticReaddir(absFromDir).forEach(item => { + this._ensureAllNonPackageDirectories( + files.pathJoin(absFromDir, item), + files.pathJoin(relToDir, item) + ); + }); + } + // Recursively copy a directory and all of its contents into the // bundle. But if the symlink option was passed to the Builder // constructor, then make a symlink instead, if possible. @@ -408,7 +458,13 @@ Previous builder: ${previousBuilder.outputPath}, this builder: ${outputPath}` // entries that end with a slash if it's a directory. // - specificFiles: just copy these paths (specified as relative to 'to'). // - symlink: true if the directory should be symlinked instead of copying - copyDirectory({ + copyDirectory(options) { + // TODO(benjamn) Remove this wrapper when Builder#enter is no longer + // implemented using ridiculous hacks. + return this._copyDirectory(options); + } + + _copyDirectory({ from, to, ignore, specificFiles, @@ -608,21 +664,31 @@ Previous builder: ${previousBuilder.outputPath}, this builder: ${outputPath}` // The sub-builder returned does not have all Builder methods (for // example, complete() wouldn't make sense) and you should not rely // on it being instanceof Builder. + // + // TODO(benjamn) This nonsense should be ripped out by any means + // necessary... whenever someone has the time. enter(relPath) { - const methods = ["write", "writeJson", "reserve", "generateFilename", - "copyDirectory", "enter"]; const subBuilder = {}; const relPathWithSep = relPath + files.pathSep; + const methods = [ + "write", + "writeJson", + "reserve", + "generateFilename", + "copyDirectory", + "copyNodeModulesDirectory", + "enter", + ]; methods.forEach(method => { subBuilder[method] = (...args) => { - if (method !== "copyDirectory") { - // Normal method (relPath as first argument) - args[0] = files.pathJoin(relPath, args[0]); - } else { - // with copyDirectory the path we have to fix up is inside - // an options hash + if (method === "copyDirectory" || + method === "copyNodeModulesDirectory") { + // The copy methods take their relative paths via options.to. args[0].to = files.pathJoin(relPath, args[0].to); + } else { + // Other methods have relPath as the first argument. + args[0] = files.pathJoin(relPath, args[0]); } let ret = this[method](...args); @@ -763,7 +829,13 @@ function symlinkWithOverwrite(source, target) { // Wrap slow methods into Profiler calls const slowBuilderMethods = [ - '_ensureDirectory', 'write', 'enter', 'copyDirectory', 'enter', 'complete' + "_ensureDirectory", + "write", + "enter", + "copyDirectory", + "copyNodeModulesDirectory", + "enter", + "complete", ]; slowBuilderMethods.forEach(method => { diff --git a/tools/isobuild/bundler.js b/tools/isobuild/bundler.js index b4c05bee64..6b84a4a5e5 100644 --- a/tools/isobuild/bundler.js +++ b/tools/isobuild/bundler.js @@ -2253,7 +2253,7 @@ class JsImage { copyOptions.filter = prodPackagePredicate; } - builder.copyDirectory(copyOptions); + builder.copyNodeModulesDirectory(copyOptions); } }); diff --git a/tools/isobuild/isopack.js b/tools/isobuild/isopack.js index aca695b1fb..553f3e44b7 100644 --- a/tools/isobuild/isopack.js +++ b/tools/isobuild/isopack.js @@ -1121,7 +1121,7 @@ _.extend(Isopack.prototype, { // If unibuilds included node_modules, copy them in. _.each(npmDirsToCopy, (bundlePath, sourcePath) => { - builder.copyDirectory({ + builder.copyNodeModulesDirectory({ from: sourcePath, to: bundlePath, npmDiscards: self.npmDiscards, diff --git a/tools/isobuild/unibuild.js b/tools/isobuild/unibuild.js index 58361331ac..d3bc27a1a4 100644 --- a/tools/isobuild/unibuild.js +++ b/tools/isobuild/unibuild.js @@ -255,10 +255,8 @@ export class Unibuild { const bundlePath = _.has(npmDirsToCopy, nmd.sourcePath) // We already have this npm directory from another unibuild. ? npmDirsToCopy[nmd.sourcePath] - : npmDirsToCopy[nmd.sourcePath] = builder.generateFilename( - nmd.getPreferredBundlePath("isopack"), - { directory: true } - ); + : npmDirsToCopy[nmd.sourcePath] = + nmd.getPreferredBundlePath("isopack"); node_modules[bundlePath] = nmd.toJSON(); }); diff --git a/tools/tests/old/test-bundler-options.js b/tools/tests/old/test-bundler-options.js index 12b9a8c5e8..2482ca4999 100644 --- a/tools/tests/old/test-bundler-options.js +++ b/tools/tests/old/test-bundler-options.js @@ -123,28 +123,55 @@ var runTest = function () { if (process.platform !== "win32") { // Windows doesn't have symlinks console.log("includeNodeModules"); - assert.doesNotThrow(function () { - var tmpOutputDir = tmpDir(); - var result = bundler.bundle({ - projectContext: projectContext, - outputPath: tmpOutputDir, - includeNodeModules: 'symlink' - }); - assert.strictEqual(result.errors, false); - // sanity check -- main.js has expected contents. - assert.strictEqual(files.readFile(files.pathJoin(tmpOutputDir, "main.js"), "utf8"), - bundler._mainJsContents); - // node_modules directory exists and is a symlink - assert(files.lstat(files.pathJoin(tmpOutputDir, "programs", "server", "node_modules")).isSymbolicLink()); - // node_modules contains fibers - assert(files.exists(files.pathJoin(tmpOutputDir, "programs", "server", "node_modules", "fibers"))); - // package node_modules directory also a symlink - // XXX might be breaking this - assert(files.lstat(files.pathJoin( - tmpOutputDir, "programs", "server", "npm", "node_modules", "meteor", "ddp-server", "node_modules")) - .isSymbolicLink()); + var tmpOutputDir = tmpDir(); + var result = bundler.bundle({ + projectContext: projectContext, + outputPath: tmpOutputDir, + includeNodeModules: 'symlink' }); + + console.log("after bundler.bundle"); + + assert.strictEqual(result.errors, false); + + console.log("before bundler._mainJsContents"); + + // sanity check -- main.js has expected contents. + assert.strictEqual( + files.readFile(files.pathJoin(tmpOutputDir, "main.js"), "utf8"), + bundler._mainJsContents + ); + + console.log("before programs/server/node_modules check"); + + // node_modules directory exists and is a symlink + assert(files.lstat(files.pathJoin( + tmpOutputDir, "programs", "server", "node_modules" + )).isSymbolicLink()); + + console.log("before programs/server/node_modules/fibers check"); + + // node_modules contains fibers as a symlink + assert(files.lstat(files.pathJoin( + tmpOutputDir, "programs", "server", "node_modules", "fibers" + )).isDirectory()); + + console.log("before ddp-server/node_modules check") + + // package node_modules directory also a directory + assert(files.lstat(files.pathJoin( + tmpOutputDir, "programs", "server", "npm", "node_modules", + "meteor", "ddp-server", "node_modules" + )).isDirectory()); + + console.log("before ddp-server/node_modules/sockjs check"); + + // ddp-server/node_modules/sockjs is a symlink + assert(files.lstat(files.pathJoin( + tmpOutputDir, "programs", "server", "npm", "node_modules", + "meteor", "ddp-server", "node_modules", "sockjs" + )).isSymbolicLink()); } };