From f78184c49794b920d21e7f2a990e01d63c30c754 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Tue, 7 Oct 2014 17:03:44 -0700 Subject: [PATCH 1/4] Disallow plugins and imply in debugOnly packages --- tools/package-source.js | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/tools/package-source.js b/tools/package-source.js index a50525116f..76ed7780b3 100644 --- a/tools/package-source.js +++ b/tools/package-source.js @@ -517,7 +517,7 @@ _.extend(PackageSource.prototype, { "trying to initialize a nonexistent base package " + value); } } else if (key === "debugOnly") { - self.debugOnly = value; + self.debugOnly = !!value; } else { // Do nothing. We might want to add some keys later, and we should err // on the side of backwards compatibility. @@ -869,6 +869,21 @@ _.extend(PackageSource.prototype, { // recover by ignoring } + // We want the "debug mode" to be a property of the *bundle* operation + // (turning a set of packages, including the app, into a star), not the + // *compile* operation (turning a package source into an isopack). This is + // so we don't have to publish two versions of each package. But we have no + // way to mark a file in an isopack as being the result of running a plugin + // from a debugOnly dependency, and so there is no way to tell which files + // to exclude in production mode from a published package. Eventually, we'll + // add such a flag to the isopack format, but until then we'll sidestep the + // issue by disallowing build plugins in debugOnly packages. + if (self.debugOnly && !_.isEmpty(self.pluginInfo)) { + buildmessage.error( + "can't register build plugins in debugOnly packages"); + // recover by ignoring + } + if (self.version === null && options.requireVersion) { if (options.defaultVersion) { self.version = options.defaultVersion; @@ -1137,6 +1152,17 @@ _.extend(PackageSource.prototype, { * @param {String|String[]} packageSpecs Name of a package, or array of package names, with an optional @version component for each. */ imply: function (names, arch) { + // We currently disallow build plugins in debugOnly packages; but if + // you could use imply in a debugOnly package, you could pull in the + // build plugin from an implied package, which would have the same + // problem as allowing build plugins directly in the package. So no + // imply either! + if (self.debugOnly) { + buildmessage.error("can't use imply in debugOnly packages"); + // recover by ignoring + return; + } + names = toArray(names); arch = toArchArray(arch); From 9bf291c8a045895ee5e2fa56a42e2047b0a63747 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Tue, 7 Oct 2014 17:15:52 -0700 Subject: [PATCH 2/4] Read and write debugOnly flag in isopack --- tools/compiler.js | 3 ++- tools/isopack.js | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/tools/compiler.js b/tools/compiler.js index 13d7c77d96..6fb07fb596 100644 --- a/tools/compiler.js +++ b/tools/compiler.js @@ -1106,7 +1106,8 @@ compiler.compile = function (packageSource, options) { cordovaDependencies: packageSource.cordovaDependencies, buildTimeDirectDependencies: buildTimeDeps.directDependencies, buildTimePluginDependencies: buildTimeDeps.pluginDependencies, - includeTool: packageSource.includeTool + includeTool: packageSource.includeTool, + debugOnly: packageSource.debugOnly }); // Compile unibuilds. Might use our plugins, so needs to happen second. diff --git a/tools/isopack.js b/tools/isopack.js index 5c836319d0..4a5440149c 100644 --- a/tools/isopack.js +++ b/tools/isopack.js @@ -235,6 +235,7 @@ var Isopack = function () { self.version = null; self.earliestCompatibleVersion = null; self.isTest = false; + self.debugOnly = false; // Unibuilds, an array of class Unibuild. self.unibuilds = []; @@ -317,6 +318,7 @@ _.extend(Isopack.prototype, { self.buildTimeDirectDependencies = options.buildTimeDirectDependencies; self.buildTimePluginDependencies = options.buildTimePluginDependencies; self.includeTool = options.includeTool; + self.debugOnly = options.debugOnly; }, // Programmatically add a unibuild to this Isopack. Should only be @@ -626,6 +628,7 @@ _.extend(Isopack.prototype, { self.version = mainJson.version; self.earliestCompatibleVersion = mainJson.earliestCompatibleVersion; self.isTest = mainJson.isTest; + self.debugOnly = !!mainJson.debugOnly; } _.each(mainJson.plugins, function (pluginMeta) { rejectBadPath(pluginMeta.path); @@ -765,6 +768,9 @@ _.extend(Isopack.prototype, { plugins: [] }; + if (self.debugOnly) { + mainJson.debugOnly = true; + } if (! _.isEmpty(self.cordovaDependencies)) { mainJson.cordovaDependencies = self.cordovaDependencies; } From 5590d194c5f21a41d87a74422a1c539b80af0b1f Mon Sep 17 00:00:00 2001 From: David Glasser Date: Tue, 7 Oct 2014 17:28:21 -0700 Subject: [PATCH 3/4] debug-only flag only affects bundler, not compiler --- tools/bundler.js | 27 ++++++++----- tools/catalog.js | 47 ---------------------- tools/commands.js | 4 +- tools/compiler.js | 88 ++++++----------------------------------- tools/package-loader.js | 9 ----- tools/project.js | 44 +++------------------ 6 files changed, 36 insertions(+), 183 deletions(-) diff --git a/tools/bundler.js b/tools/bundler.js index ce06c1a0d0..4628ebcbce 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -527,14 +527,16 @@ _.extend(Target.prototype, { var packageLoader = self.packageLoader; // Find the roots - var rootUnibuilds = - _.map(options.packages || [], function (p) { - if (typeof p === "string") { - return packageLoader.getUnibuild(p, self.arch); - } else { - return p.getUnibuildAtArch(self.arch); - } - }); + var rootUnibuilds = []; + _.each(options.packages, function (p) { + if (typeof p === "string") { + p = packageLoader.getPackage(p, { throwOnError: true }); + } + if (p.debugOnly && !project.project.includeDebug) { + return; + } + rootUnibuilds.push(p.getUnibuildAtArch(self.arch)); + }); // PHASE 1: Which unibuilds will be used? // @@ -552,7 +554,9 @@ _.extend(Target.prototype, { usedUnibuilds[unibuild.id] = unibuild; usedPackages[unibuild.pkg.name] = true; compiler.eachUsedUnibuild( - unibuild.uses, self.arch, packageLoader, addToGetsUsed); + unibuild.uses, self.arch, packageLoader, { + skipDebugOnly: !project.project.includeDebug + }, addToGetsUsed); }; _.each(rootUnibuilds, addToGetsUsed); @@ -590,7 +594,10 @@ _.extend(Target.prototype, { // SOMETHING uses strongly). compiler.eachUsedUnibuild( unibuild.uses, self.arch, packageLoader, - { skipUnordered: true, acceptableWeakPackages: usedPackages}, + { skipUnordered: true, + acceptableWeakPackages: usedPackages, + skipDebugOnly: !project.project.includeDebug + }, function (usedUnibuild) { if (onStack[usedUnibuild.id]) { buildmessage.error("circular dependency between packages " + diff --git a/tools/catalog.js b/tools/catalog.js index 865a5a05b1..5ddb806ab9 100644 --- a/tools/catalog.js +++ b/tools/catalog.js @@ -300,53 +300,6 @@ _.extend(LayeredCatalog.prototype, { return ret.answer; }, - // Separates out debugOnly packages and their dependencies. Goes through - // constraints recursively, and selects a subset of our versions that we use - // if we omit debug-only packages and their dependencies. - // - // Takes in a set of constraints (list of packages that we explicitly depend - // on) and a set of versions of those packages that we have agreed to use. - // - // Returns an object with news: versions (the versions we will use) and - // excluded (the versions that we will NOT use). - separateOutDebugDeps: function (constraints, versions) { - var self = this; - var processed = {}; - var prodDeps = function (seed, versions) { - _.each(seed, function (s) { - if (processed[s]) - return; - var vRec = self.getVersion(s, versions[s]); - if (vRec && !!vRec.debugOnly) { - // This is a debugOnly dependency. Stop. - return; - } else { - processed[s] = versions[s]; - var next = []; - // Weak dependencies don't lead to packages that get bundled, so we - // don't have to worry if they are debugOnly. (Unless they are - // mentioned as non-weak dependencies elsewhere, in which case we - // will get to them then) - var next = _.filter(_.keys(vRec.dependencies), function (p) { - return _.has(versions, p); - }); - prodDeps(next, versions); - } - }); - }; - prodDeps(constraints, versions); - var excluded = {}; - _.each(versions, function(v, p) { - if (!_.has(processed, p)) { - excluded[p] = v; - } - }); - return { - versions: processed, - excluded: excluded - }; - }, - // Refresh the catalogs referenced by this catalog. // options: // - forceRefresh: even if there is a future in progress, refresh the catalog diff --git a/tools/commands.js b/tools/commands.js index c0e1e23705..f94030aff9 100644 --- a/tools/commands.js +++ b/tools/commands.js @@ -1264,9 +1264,7 @@ main.registerCommand({ project.setMuted(true); // Mute output where applicable // Hardset the proper release. project.writeMeteorReleaseVersion(release.current.name || 'none'); - // Set debug mode directly, mostly to avoid running the constraint solver an - // extra time. - project.includeDebug = !options.production; + project.setDebug(!options.production); project.forceEditPackages( [options['driver-package'] || 'test-in-browser'], 'add'); diff --git a/tools/compiler.js b/tools/compiler.js index 6fb07fb596..810e112846 100644 --- a/tools/compiler.js +++ b/tools/compiler.js @@ -76,20 +76,21 @@ compiler.eachUsedUnibuild = function ( return; if (use.weak && !_.has(acceptableWeakPackages, use.package)) return; - if (packageLoader.excludedPackage(use.package)) { - return; - } usesToProcess.push(use); }); while (!_.isEmpty(usesToProcess)) { var use = usesToProcess.shift(); - if (packageLoader.excludedPackage(use.package)) { - return; - } + var usedPackage = packageLoader.getPackage( + use.package, { throwOnError: true }); - var unibuild = packageLoader.getUnibuild(use.package, arch); + // Ignore this package if we were told to skip debug-only packages and it is + // debug-only. + if (usedPackage.debugOnly && options.skipDebugOnly) + continue; + + var unibuild = usedPackage.getUnibuildAtArch(arch); if (!unibuild) { // The package exists but there's no unibuild for us. A buildmessage has // already been issued. Recover by skipping. @@ -117,20 +118,12 @@ compiler.eachUsedUnibuild = function ( // Output is an object with keys: // - directDependencies: map from package name to version string, for the // package's direct, ordered, strong, non-implied dependencies. -// - packageDependencies: map from package name to version string to complete -// transitive dependency in this package. We need for the version lock file -// and to deal with implies. -// - packageExcluded: map from package name to version string of transitive -// dependencies that are not included in packageDependencies because we will -// not bundle them with the package. If we are bundling for production mode -// (default) this is the debugOnly packages and their dependencies. // - pluginDependencies: map from plugin name to complete (transitive) // version information for all packages used to build the plugin, as // a map from package name to version string. -// - pluginExcluded: see: packageExcluded, but per plugin. -// -// Options: -// - (see options for the constraint solver in catalog-local.js) +// - packageDependencies: map from package name to version string to complete +// transitive dependency in this package. We need for the version lock file +// and to deal with implies. // // XXX You may get different results from this function depending on // when you call it (if, for example, the packages in the catalog @@ -227,7 +220,6 @@ var determineBuildTimeDependencies = function (packageSource, // -- Dependencies of Plugins -- ret.pluginDependencies = {}; - var pluginConstraints = {}; var pluginVersions = packageSource.dependencyVersions.pluginDependencies; _.each(packageSource.pluginInfo, function (info) { var constraints = {}; @@ -248,7 +240,6 @@ var determineBuildTimeDependencies = function (packageSource, }); var pluginVersion = pluginVersions[info.name] || {}; - pluginConstraints[info.name] = constraints_array; try { ret.pluginDependencies[info.name] = packageSource.catalog.resolveConstraints( @@ -284,59 +275,6 @@ var determineBuildTimeDependencies = function (packageSource, release.current.getCurrentToolsVersion()); } - // If we are building for production mode, we also care about filtering out - // debugOnly dependencies. Note, however, that we still record them in the - // versions file, since they influence the versions of other dependencies. - // - // We build for production mode by default, unless our project says - // otherwise. And we never build in debug mode for uniload, because that makes - // no sense. - _.each(["packageExcluded", "pluginExcluded"], function (key) { - ret[key] = {}; - }); - // Don't bother filtering things if we are in uniload. That's too complicated. - if (packageSource.catalog !== catalog.complete) { - return ret; - } - // If you have decided to build this package already, and this package is - // debugOnly, then it doesn't make sense to filter out its debugOnly subtrees. - if (packageSource.debugOnly) { - return ret; - } - // If the project tells us to include debugOnly packages, we shall do it. - var project = require('./project.js').project; - if (project && project.includeDebug) { - return ret; - } - - var directVersions = - packageSource.catalog.separateOutDebugDeps( - _.pluck(constraints_array, 'name'), - ret.directDependencies); - - var packageVersions = - packageSource.catalog.separateOutDebugDeps( - _.pluck(constraints_array, 'name'), - ret.packageDependencies); - - var pluginExcluded = {}; - var pluginVersions = {}; - _.each(ret.pluginDependencies, function (versions, name) { - var filteredVersions = - packageSource.catalog.separateOutDebugDeps( - _.pluck(pluginConstraints[name], 'name'), - versions); - pluginVersions[name] = filteredVersions.versions; - pluginExcluded[name] = filteredVersions.excluded; - }); - - ret = { - directDependencies : directVersions.versions, - packageDependencies: packageVersions.versions, - packageExcluded: packageVersions.excluded, - pluginDependencies: pluginVersions, - pluginExcluded: pluginExcluded - }; return ret; }; @@ -1028,8 +966,7 @@ compiler.compile = function (packageSource, options) { var loader = new packageLoader.PackageLoader({ versions: buildTimeDeps.pluginDependencies[info.name], - catalog: packageSource.catalog, - excluded: buildTimeDeps.pluginExcluded[info.name] + catalog: packageSource.catalog }); loader.downloadMissingPackages({serverArch: archinfo.host() }); @@ -1114,7 +1051,6 @@ compiler.compile = function (packageSource, options) { var loader = new packageLoader.PackageLoader({ versions: buildTimeDeps.packageDependencies, catalog: packageSource.catalog, - excluded: buildTimeDeps.packageExcluded, constraintSolverOpts: { ignoreProjectDeps: options.ignoreProjectDeps } diff --git a/tools/package-loader.js b/tools/package-loader.js index 0af4686cdc..be3cbb234a 100644 --- a/tools/package-loader.js +++ b/tools/package-loader.js @@ -28,7 +28,6 @@ exports.PackageLoader = function (options) { self.uniloadDir = options.uniloadDir; self.constraintSolverOpts = options.constraintSolverOpts; self.catalog = options.catalog; - self.excluded = options.excluded; }; _.extend(exports.PackageLoader.prototype, { @@ -117,13 +116,5 @@ _.extend(exports.PackageLoader.prototype, { tropohouse.default.downloadMissingPackages(self.versions, { serverArch: options.serverArch }); - }, - - // Sometimes, we have figured out the versions for packages, but we have no - // intention of loading them. In this case, they are considered 'excluded - // packages'. This function lets us know if we have hit one of those. - excludedPackage: function (packageName) { - var self = this; - return self.excluded && self.excluded[packageName]; } }); diff --git a/tools/project.js b/tools/project.js index c3ba2067eb..8c21cdb5aa 100644 --- a/tools/project.js +++ b/tools/project.js @@ -117,11 +117,7 @@ var Project = function () { _.extend(Project.prototype, { setDebug: function (debug) { var self = this; - self._ensureDepsUpToDate(); - if (self.includeDebug !== debug) { - self.includeDebug = debug; - self._generatePackageLoader(); - } + self.includeDebug = debug; }, // Sets the mute flag on the project. Muted projects don't print out non-error @@ -255,9 +251,11 @@ _.extend(Project.prototype, { process.exit(1); } - // We have successfully set our versions, so let's generate the package - // loader. - self._generatePackageLoader(); + // Finally, initialize the package loader. + self.packageLoader = new packageLoader.PackageLoader({ + versions: newVersions, + catalog: catalog.complete + }); // We are done! self._depsUpToDate = true; @@ -265,36 +263,6 @@ _.extend(Project.prototype, { } }, - // Given a set of versions and combined constraints, generate a package loader - // for this project. - // - // This is part of _ensureDepsUpToDate. Assumes that VERSIONS HAVE BEEN - // GENERATED. If you are not sure if your project is up to date before you - // call this function, call _ensureUpToDate first. - _generatePackageLoader: function () { - var self = this; - // Finally, initialize the package loader. If we are building for prod, we - // are going to not load debug packages, so filter that out here. - if (self.includeDebug) { - self.packageLoader = new packageLoader.PackageLoader({ - versions: self.dependencies, - catalog: catalog.complete, - excluded: {} - }); - } else { - var prodVersions = - catalog.complete.separateOutDebugDeps( - _.pluck(self.combinedConstraints, 'name'), - self.dependencies); - - self.packageLoader = new packageLoader.PackageLoader({ - versions: prodVersions.versions, - catalog: catalog.complete, - excluded: prodVersions.excluded - }); - } - }, - // Given a set of packages from a release, recalculates all the constraints on // a given project: combines the constraints from all the programs, the // packages file and the release packages. From 3faf1aa21e83ec78dd640c9a1c8f760ebe201cf1 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Tue, 7 Oct 2014 18:19:00 -0700 Subject: [PATCH 4/4] make test more convincing --- .../packages/debug-only/debug-only.js | 3 +-- tools/tests/package-tests.js | 20 +++++++++---------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/tools/tests/apps/package-tests/packages/debug-only/debug-only.js b/tools/tests/apps/package-tests/packages/debug-only/debug-only.js index 23162b0db5..3740f30ec1 100644 --- a/tools/tests/apps/package-tests/packages/debug-only/debug-only.js +++ b/tools/tests/apps/package-tests/packages/debug-only/debug-only.js @@ -1,2 +1 @@ -// Write your package code here! -console.log("Testing a thing"); +global.DEBUG_ONLY_LOADED = true; diff --git a/tools/tests/package-tests.js b/tools/tests/package-tests.js index 871023e27d..2c3af8602b 100644 --- a/tools/tests/package-tests.js +++ b/tools/tests/package-tests.js @@ -289,18 +289,18 @@ selftest.define("add packages to app", ["net"], function () { run = s.run("add", "debug-only"); run.match("debug-only"); run.expectExit(0); - run = s.run(); - run.waitSecs(15); - run.match("Testing a thing"); - run.match("Started"); - run.stop(); - run = s.run("--production"); + s.mkdir("server"); + s.write("server/debug.js", + "process.exit(global.DEBUG_ONLY_LOADED ? 234 : 235)"); + + run = s.run("--once"); run.waitSecs(15); - run.match("Started MongoDB.\n"); - run.waitSecs(5); - run.read("=> Starting your app"); - run.stop(); + run.expectExit(234); + + run = s.run("--once", "--production"); + run.waitSecs(15); + run.expectExit(235); }); // Add a package that adds files to specific client architectures.