From 7d1cb122fed2f0a458d288ff7f47e20e7a5f4274 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Tue, 11 Nov 2014 01:05:36 -0800 Subject: [PATCH 1/4] constraint solver should know about plugin deps --- tools/package-source.js | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tools/package-source.js b/tools/package-source.js index fd507fee8b..fc4bba5919 100644 --- a/tools/package-source.js +++ b/tools/package-source.js @@ -1936,6 +1936,30 @@ _.extend(PackageSource.prototype, { _.each(arch.implies, _.partial(processUse, true)); }); + _.each(self.pluginInfo, function (info) { + _.each(info.use, function (spec) { + var parsedSpec = utils.splitConstraint(spec); + if (!_.has(dependencies, parsedSpec.package)) { + dependencies[parsedSpec.package] = { + constraint: null, + references: [] + }; + allConstraints[parsedSpec.package] = []; + } + var d = dependencies[parsedSpec.package]; + + if (parsedSpec.constraint) { + allConstraints[parsedSpec.package].push(parsedSpec.constraints); + if (d.constraint === null) { + d.constraint = parsedSpec.constraint; + } else if (d.constraint !== parsedSpec.constraint) { + failed = true; + } + } + d.references.push({arch: 'os'}); + }); + }); + if (failed && options.logError) { _.each(allConstraints, function (constraints, name) { constraints = _.uniq(constraints); From db90bd69448b46047bf72818ba10294ec217a342 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Sat, 22 Nov 2014 14:20:10 -0800 Subject: [PATCH 2/4] os was later changed to plugin --- tools/package-source.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/package-source.js b/tools/package-source.js index fc4bba5919..996dc96fb0 100644 --- a/tools/package-source.js +++ b/tools/package-source.js @@ -1956,7 +1956,7 @@ _.extend(PackageSource.prototype, { failed = true; } } - d.references.push({arch: 'os'}); + d.references.push({arch: 'plugin'}); }); }); From cec75e527cb9979e21f9547bd163ad57811cc367 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Sat, 22 Nov 2014 13:54:38 -0800 Subject: [PATCH 3/4] constraint edges are by package, not unibuild Consider the following situation: - app uses package P - Every version of P contains `api.use('s', 'server')` - Every version of S contains `api.use('c', 'client')` - There's nothing else around using S or C When we bundle our app, we will not end up putting any unibuilds from C into the bundle. That's fine. The previous version of the constraint solver understood this, and so C wasn't even part of the constraint solver solution. HOWEVER, even though C does not contribute any unibuilds to the app bundle, we still need to compile C in order to compile S. That's because our current implementation never compiles only part of an isopack, even if only part of the isopack will be needed for the app. The structured project initialization done via ProjectContext will thus decide to not build or load C, which will make it fail to compile S when it gets around to compiling the client unibuilds in S. We could make the model more complex by making it possible to compile only part of S. Instead, we'll make the Meteor package model simpler. Constraints, as far as the constraint solver is concerned, are now at a package level. So in this case, "C" will actually be part of the project (will end up in .meteor/versions, etc) even though it will continue to not provide any part of any of the bundled client or server programs. This means that nodes in the constraint solver's graph will now just be package versions, not unibuild versions. That's already the language that the constraint solver spoke in as far as its inputs, outputs, and error messages were concerned! An example of an app that couldn't be built on the isopack-cache branch before this commit and can be now is https://github.com/glasser/precise-constraints-example (It can be built with 1.0, but only by compiling a version of `c` that isn't part of .meteor/versions!) Note that this also means that .meteor/versions expresses enough to allow us to implement a simpler constraint check that doesn't need to do the full tree walk of the constraint solver. Such a checker would be implemented as: - gather root dependencies and constraints (project constraints, release constraints, etc) - add the dependencies and constraints from all versions mentioned in .meteor/versions - see if the choices made in .meteor/versions satisfy these dependencies and constraints This algorithm did NOT work previously, because you couldn't just look at the constraints coming from `s@0.0.0` and say "they're satifisfied" because you had to know to "ignore" the constraint on c#web.browser because s#web.browser is not part of the app, which is not a fact that actually got recorded in .meteor/versions. (This commit does not implement this simpler constraint check, though.) --- .../constraint-solver-tests.js | 2 +- .../constraint-solver/constraint-solver.js | 192 ++++-------------- .../constraint-solver/constraints-list.js | 5 +- packages/constraint-solver/resolver-state.js | 28 +-- packages/constraint-solver/resolver.js | 12 +- tools/package-version-parser.js | 2 +- 6 files changed, 59 insertions(+), 182 deletions(-) diff --git a/packages/constraint-solver/constraint-solver-tests.js b/packages/constraint-solver/constraint-solver-tests.js index e9d90cf732..49e2fa11c3 100644 --- a/packages/constraint-solver/constraint-solver-tests.js +++ b/packages/constraint-solver/constraint-solver-tests.js @@ -156,7 +156,7 @@ Tinytest.add("constraint solver - no results", function (test) { && error.message.match(/bad-2@1\.0\.0/) // We shouldn't get shown indirect itself in a pathway: that would just // be an artifact of there being a path that passes through another - // unibuild. (Note: we might change our mind and decide that all these + // package. (Note: we might change our mind and decide that all these // lines should end in the relevant constraint, which would probably be // nice! But in that case, we should test that no line ends with TWO // mentions of indirect.) diff --git a/packages/constraint-solver/constraint-solver.js b/packages/constraint-solver/constraint-solver.js index 9809614058..0f6a0bf594 100644 --- a/packages/constraint-solver/constraint-solver.js +++ b/packages/constraint-solver/constraint-solver.js @@ -1,14 +1,3 @@ -// Copied from archinfo.matches() in tools/ -// -// archMatches("os", "os") => true -// archMatches("web.cordova", "web") => true -// archMatches("web.cordova", "web.cordova") => true -var archMatches = function (arch, baseArch) { - return arch.substr(0, baseArch.length) === baseArch && - (arch.length === baseArch.length || - arch.substr(baseArch.length, 1) === "."); -}; - ConstraintSolver = {}; // `catalog` has the following methods: @@ -71,67 +60,41 @@ ConstraintSolver.PackagesResolver.prototype._loadPackageInfo = function ( packageName) { var self = this; - // XXX in theory there might be different archs but in practice they are - // always "os", "web.browser" and "web.cordova". Fix this once we - // actually have different archs used. - var allArchs = ["os", "web.browser", "web.cordova"]; - // We rely on sortedness in the constraint solver, since one of the cost // functions wants to be able to quickly find the earliest or latest version. var sortedVersions = self.catalog.getSortedVersions(packageName); _.each(sortedVersions, function (version) { var versionDef = self.catalog.getVersion(packageName, version); - var unibuilds = {}; - - _.each(allArchs, function (arch) { - var unitName = packageName + "#" + arch; - unibuilds[unitName] = new ConstraintSolver.UnitVersion(unitName, version); - self.resolver.addUnitVersion(unibuilds[unitName]); - }); + var unitVersion = new ConstraintSolver.UnitVersion(packageName, version); + self.resolver.addUnitVersion(unitVersion); _.each(versionDef.dependencies, function (dep, depName) { self._ensurePackageInfoLoaded(depName); - _.each(dep.references, function (ref) { - _.each(allArchs, function (arch) { - if (archMatches(arch, ref.arch)) { - var unitName = packageName + "#" + arch; - var unitVersion = unibuilds[unitName]; + // "dep" contains a list of references, which describes which unibuilds of + // this unitVersion depend on depName, as well as a constraint, which + // constraints the versions it depends on. - if (! unitVersion) - throw new Error("A non-standard arch " + arch + " for package " + packageName); + // The package->package dependency is weak if ALL of the underlying + // unibuild->unibuild dependencies are weak. ie, + // api.use('dep', 'server', { weak: true }); + // api.use('dep', 'client'); + // is not weak at the package->package level. - var targetUnitName = depName + "#" + arch; - - // Add the dependency if needed - if (! ref.weak) - unitVersion.addDependency(targetUnitName); - - // Add a constraint if such exists - if (dep.constraint && dep.constraint !== "none") { - var constraint = - self.resolver.getConstraint(targetUnitName, dep.constraint); - unitVersion.addConstraint(constraint); - } - } - }); + var isWeak = _.all(dep.references, function (ref) { + return ref.weak; }); - }); - // Every unibuild implies that if it is picked, other unibuilds are - // constrained to the same version. - _.each(unibuilds, function (unibuild, unibuildName) { - _.each(unibuilds, function (other, otherUnibuildName) { - if (unibuild === other) - return; + // Add the dependency if needed. + if (! isWeak) + unitVersion.addDependency(depName); - // Constraint is the exact same version of a unibuild - var constraintStr = "=" + version; - var constraint = - self.resolver.getConstraint(otherUnibuildName, constraintStr); - unibuild.addConstraint(constraint); - }); + // Add a constraint if needed. + if (dep.constraint && dep.constraint !== "none") { + var constraint = self.resolver.getConstraint(depName, dep.constraint); + unitVersion.addConstraint(constraint); + } }); }); }; @@ -182,33 +145,26 @@ ConstraintSolver.PackagesResolver.prototype.resolve = function ( self._ensurePackageInfoLoaded(packageName); }); - // XXX glasser and ekate added this filter to strip some undefineds that - // were causing crashes, but maybe the real answer is that there shouldn't - // have been undefineds? if (options.previousSolution) { - options.previousSolution = - _.filter(_.flatten( - _.map(options.previousSolution, function (version, packageName) { - return _.map(self._unibuildsForPackage(packageName), function (unitName) { - return self.resolver._unitsVersionsMap[unitName + "@" + version]; - }); - })), _.identity); + // Replace previousSolution map with a list of the UnitVersions that we know + // about that were mentioned. (_.compact drops unknown UnitVersions.) + options.previousSolution = _.compact( + _.map(options.previousSolution, function (version, packageName) { + return self.resolver.getUnitVersion(packageName, version); + })); } - // split every package name to one or more archs belonging to that package - // (["foobar"] => ["foobar#os", "foobar#web.browser", ...]) - // XXX for now just hardcode in all of the known architectures - var upgradeUnibuilds = {}; + // Convert options.upgrade to a map for O(1) access. + // XXX we should probably just change the API so it's passed in this way + var upgradePackages = {}; _.each(options.upgrade, function (packageName) { - _.each(self._unibuildsForPackage(packageName), function (unibuildName) { - upgradeUnibuilds[unibuildName] = true; - }); + upgradePackages[packageName] = true; }); - options.upgrade = upgradeUnibuilds; + options.upgrade = upgradePackages; - var dc = self._splitDepsToConstraints(dependencies, constraints); + constraints = self._makeConstraintObjects(constraints); - options.rootDependencies = dc.dependencies; + options.rootDependencies = dependencies; var resolverOptions = self._getResolverOptions(options); var res = null; // If a previous solution existed, try resolving with additional (weak) @@ -226,7 +182,7 @@ ConstraintSolver.PackagesResolver.prototype.resolve = function ( // guide which things are encouraged to be upgraded vs stay the same in the // heuristic.) if (!_.isEmpty(options.previousSolution) && _.isEmpty(options.upgrade)) { - var constraintsWithPreviousSolutionLock = _.clone(dc.constraints); + var constraintsWithPreviousSolutionLock = _.clone(constraints); _.each(options.previousSolution, function (uv) { constraintsWithPreviousSolutionLock.push( self.resolver.getConstraint(uv.name, '=' + uv.version)); @@ -235,7 +191,7 @@ ConstraintSolver.PackagesResolver.prototype.resolve = function ( // Try running the resolver. If it fails to resolve, that's OK, we'll keep // working. res = self.resolver.resolve( - dc.dependencies, constraintsWithPreviousSolutionLock, resolverOptions); + dependencies, constraintsWithPreviousSolutionLock, resolverOptions); } catch (e) { if (!(e.constraintSolverError)) throw e; @@ -246,8 +202,7 @@ ConstraintSolver.PackagesResolver.prototype.resolve = function ( // without locking in the previous solution as strict equality. if (!res) { try { - res = self.resolver.resolve( - dc.dependencies, dc.constraints, resolverOptions); + res = self.resolver.resolve(dependencies, constraints, resolverOptions); } catch (e) { if (!(e.constraintSolverError)) throw e; @@ -259,8 +214,7 @@ ConstraintSolver.PackagesResolver.prototype.resolve = function ( // constraints? if (!res) { resolverOptions["useRCs"] = true; - res = self.resolver.resolve( - dc.dependencies, dc.constraints, resolverOptions); + res = self.resolver.resolve(dependencies, constraints, resolverOptions); } var ret = { answer: resolverResultToPackageMap(res) }; if (resolverOptions.useRCs) @@ -268,84 +222,24 @@ ConstraintSolver.PackagesResolver.prototype.resolve = function ( return ret; }; -var removeUnibuild = function (unitName) { - return unitName.split('#')[0]; -}; - var resolverResultToPackageMap = function (choices) { var packageMap = {}; mori.each(choices, function (nameAndUv) { var name = mori.first(nameAndUv); var uv = mori.last(nameAndUv); - // Since we don't yet define the interface for a an app to depend only on - // certain unibuilds of the packages (like only web unibuilds) and we know - // that each unibuild weakly depends on other sibling unibuilds of the same - // version, we can safely output the whole package for each unibuild in the - // result. - packageMap[removeUnibuild(name)] = uv.version; + packageMap[name] = uv.version; }); return packageMap; }; -// takes dependencies and constraints and rewrites the names from "foo" to -// "foo#os" and "foo#web.browser" and "foo#web.cordova" -// XXX right now creates a dependency for every unibuild it can find -ConstraintSolver.PackagesResolver.prototype._splitDepsToConstraints = - function (inputDeps, inputConstraints) { +ConstraintSolver.PackagesResolver.prototype._makeConstraintObjects = function ( + inputConstraints) { var self = this; - var dependencies = []; - var constraints = []; - - _.each(inputDeps, function (packageName) { - _.each(self._unibuildsForPackage(packageName), function (unibuildName) { - dependencies.push(unibuildName); - }); + return _.map(inputConstraints, function (constraint) { + return self.resolver.getConstraint( + constraint.name, constraint.constraintString); }); - - _.each(inputConstraints, function (constraint) { - _.each(self._unibuildsForPackage(constraint.name), function (unibuildName) { - //XXX: This is kind of dumb -- we make this up, so we can reparse it - //later. Todo: clean this up a bit. - if (!constraint.constraintString) { - var constraintArray = []; - _.each(constraint.constraints, function (c) { - if (c.type == "exact") { - constraintArray.push("+" + c.version); - } else if (c.version) { - constraintArray.push(c.version) - } - }); - if (!_.isEmpty(constraintArray)) { - constraint.constraintString = - _.reduce(constraintArray, - function(x, y) { - return x + " || " + y; - }); - } else { - constraint.constraintString = ""; - } - } - constraints.push( - self.resolver.getConstraint(unibuildName, constraint.constraintString)); - }); - }); - - return { dependencies: dependencies, constraints: constraints }; -}; - -ConstraintSolver.PackagesResolver.prototype._unibuildsForPackage = - function (packageName) { - var self = this; - var unibuildPrefix = packageName + "#"; - var unibuilds = []; - // XXX hardcode all common architectures assuming that every package has the - // same set of architectures. - _.each(["os", "web.browser", "web.cordova"], function (arch) { - unibuilds.push(unibuildPrefix + arch); - }); - - return unibuilds; }; ConstraintSolver.PackagesResolver.prototype._getResolverOptions = diff --git a/packages/constraint-solver/constraints-list.js b/packages/constraint-solver/constraints-list.js index d20abf6777..0c56dc11a9 100644 --- a/packages/constraint-solver/constraints-list.js +++ b/packages/constraint-solver/constraints-list.js @@ -117,14 +117,13 @@ ConstraintSolver.ConstraintsList.prototype.isSatisfied = function ( return satisfied; }; -ConstraintSolver.ConstraintsList.prototype.toString = function (options) { +ConstraintSolver.ConstraintsList.prototype.toString = function () { var self = this; - options = options || {}; var strs = []; self.each(function (c) { - strs.push(c.toString({removeUnibuild: options.removeUnibuild})); + strs.push(c.toString()); }); strs = _.uniq(strs); diff --git a/packages/constraint-solver/resolver-state.js b/packages/constraint-solver/resolver-state.js index fdc0513c35..f6eb7b0f51 100644 --- a/packages/constraint-solver/resolver-state.js +++ b/packages/constraint-solver/resolver-state.js @@ -42,9 +42,9 @@ _.extend(ResolverState.prototype, { self.error = util.format( "conflict: constraint %s is not satisfied by %s.\n" + "Constraints on %s come from:\n%s", - constraint.toString({removeUnibuild: true}), + constraint.toString(), chosen.version, - removeUnibuild(constraint.name), + constraint.name, self._shownPathwaysForConstraintsIndented(constraint.name)); return self; } @@ -60,7 +60,7 @@ _.extend(ResolverState.prototype, { self.error = util.format( "conflict: constraints on %s cannot all be satisfied.\n" + "Constraints come from:\n%s", - removeUnibuild(constraint.name), + constraint.name, self._shownPathwaysForConstraintsIndented(constraint.name)); } else if (mori.count(newAlternatives) === 1) { // There's only one choice, so we can immediately choose it. @@ -83,7 +83,7 @@ _.extend(ResolverState.prototype, { self = self._clone(); if (!_.has(self._resolver.unitsVersions, unitName)) { - self.error = "unknown package: " + removeUnibuild(unitName); + self.error = "unknown package: " + unitName; return self; } @@ -99,7 +99,7 @@ _.extend(ResolverState.prototype, { self.error = util.format( "conflict: constraints on %s cannot be satisfied.\n" + "Constraints come from:\n%s", - removeUnibuild(unitName), + unitName, self._shownPathwaysForConstraintsIndented(unitName)); return self; } else if (mori.count(alternatives) === 1) { @@ -211,15 +211,6 @@ var filter = function (v, pred) { return mori.into(mori.vector(), mori.filter(pred, v)); }; -// Users are mostly confused by seeing "package#web.browser" instead of just -// "package". Remove it for error messages. -removeUnibuild = function (unitName) { - // For debugging constraint solver issues. - if (process.env.METEOR_SHOW_UNIBUILDS) - return unitName; - return unitName.split('#')[0]; -}; - // XXX from Underscore.String (http://epeli.github.com/underscore.string/) // XXX how many copies of this do we have in Meteor? var startsWith = function(str, starts) { @@ -229,18 +220,13 @@ var startsWith = function(str, starts) { var showPathway = function (pathway, dropIfFinal) { var pathUnits = mori.into_array(mori.map(function (uv) { - return uv.toString({removeUnibuild: true}); + return uv.toString(); }, mori.reverse(pathway))); - var dropPrefix = removeUnibuild(dropIfFinal) + '@'; + var dropPrefix = dropIfFinal + '@'; while (pathUnits.length && startsWith(_.last(pathUnits), dropPrefix)) { pathUnits.pop(); } - // This is a bit of a hack: we're using _.uniq in "it's sorted" mode, whose - // implementation is "drop adjacent duplicates". This is what we want (we're - // trying to avoid seeing "foo -> foo" which represents "foo#os -> - // foo#web.browser") even though it's not actually sorted. - pathUnits = _.uniq(pathUnits, true); return pathUnits.join(' -> '); }; diff --git a/packages/constraint-solver/resolver.js b/packages/constraint-solver/resolver.js index 3c7a261058..3f0f85b64e 100644 --- a/packages/constraint-solver/resolver.js +++ b/packages/constraint-solver/resolver.js @@ -270,6 +270,8 @@ ConstraintSolver.UnitVersion = function (name, unitVersion) { self.name = name; // Things with different build IDs should represent the same code, so ignore // them. (Notably: depending on @=1.3.1 should allow 1.3.1+local!) + // XXX we no longer automatically add build IDs to things as part of our build + // process, but this still reflects semver semantics. self.version = PackageVersion.removeBuildID(unitVersion); self.dependencies = []; self.constraints = new ConstraintSolver.ConstraintsList(); @@ -300,11 +302,9 @@ _.extend(ConstraintSolver.UnitVersion.prototype, { self.constraints = self.constraints.push(constraint); }, - toString: function (options) { + toString: function () { var self = this; - options = options || {}; - var name = options.removeUnibuild ? removeUnibuild(self.name) : self.name; - return name + "@" + self.version; + return self.name + "@" + self.version; } }); @@ -332,9 +332,7 @@ ConstraintSolver.Constraint = function (name, versionString) { ConstraintSolver.Constraint.prototype.toString = function (options) { var self = this; - options = options || {}; - var name = options.removeUnibuild ? removeUnibuild(self.name) : self.name; - return name + "@" + self.constraintString; + return self.name + "@" + self.constraintString; }; diff --git a/tools/package-version-parser.js b/tools/package-version-parser.js index d22ce475b3..1b9e9c950b 100644 --- a/tools/package-version-parser.js +++ b/tools/package-version-parser.js @@ -243,7 +243,7 @@ PV.parseConstraint = function (constraintString, options) { var splitted = constraintString.split('@'); var name = splitted[0]; - var versionString = splitted[1]; + var versionString = splitted[1] || ''; if (splitted.length > 2) { // throw error complaining about @ From fb6b39d956a89a4c28451a3d555aae5c81e018ad Mon Sep 17 00:00:00 2001 From: David Glasser Date: Sat, 22 Nov 2014 14:18:53 -0800 Subject: [PATCH 4/4] explicitly search for non-weak dep dep.references ought to never be empty, but if it is we should consider that to not be a dependency! --- packages/constraint-solver/constraint-solver.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/constraint-solver/constraint-solver.js b/packages/constraint-solver/constraint-solver.js index 0f6a0bf594..ed9263466e 100644 --- a/packages/constraint-solver/constraint-solver.js +++ b/packages/constraint-solver/constraint-solver.js @@ -81,13 +81,12 @@ ConstraintSolver.PackagesResolver.prototype._loadPackageInfo = function ( // api.use('dep', 'server', { weak: true }); // api.use('dep', 'client'); // is not weak at the package->package level. - - var isWeak = _.all(dep.references, function (ref) { - return ref.weak; + var createsDependency = _.any(dep.references, function (ref) { + return !ref.weak; }); // Add the dependency if needed. - if (! isWeak) + if (createsDependency) unitVersion.addDependency(depName); // Add a constraint if needed.