From de9536172e4f73fd1c4b0d2b21e907a8d693577c Mon Sep 17 00:00:00 2001 From: David Glasser Date: Sun, 30 Nov 2014 22:45:42 -0500 Subject: [PATCH] Fix anticipated pre-release calculation This commit fixes a 0.9.3 regression where the calculation of the topLevelPrereleases object was not updated with the introduction of disjunction constraints (||) and would always be empty. topLevelPrereleases and useRCsOK are merged into a single anticipatedPrereleases option which is now passed in to the constraint-solver package rather than constructed inside it. (This object will also be used in ProjectContext as part of PackageDelta calculation.) The usedRCs return flag is now called neededToUseUnanticipatedPrereleases. --- .../constraint-solver-tests.js | 2 +- .../constraint-solver/constraint-solver.js | 39 ++++++---- packages/constraint-solver/resolver-tests.js | 50 +++++++++--- packages/constraint-solver/resolver.js | 78 +++++++++---------- tools/project-context.js | 46 +++++++++-- 5 files changed, 144 insertions(+), 71 deletions(-) diff --git a/packages/constraint-solver/constraint-solver-tests.js b/packages/constraint-solver/constraint-solver-tests.js index 49e2fa11c36..e48e4f6d5de 100644 --- a/packages/constraint-solver/constraint-solver-tests.js +++ b/packages/constraint-solver/constraint-solver-tests.js @@ -87,7 +87,7 @@ var testWithResolver = function (test, resolver, f) { var constraints = splitArgs(deps).constraints; var resolvedDeps = resolver.resolve(dependencies, constraints, options); - test.equal(resolvedDeps, { answer: expected }); + test.equal(resolvedDeps.answer, expected); }; var FAIL = function (deps, regexp) { diff --git a/packages/constraint-solver/constraint-solver.js b/packages/constraint-solver/constraint-solver.js index ed9263466e6..0356ef240e4 100644 --- a/packages/constraint-solver/constraint-solver.js +++ b/packages/constraint-solver/constraint-solver.js @@ -106,16 +106,21 @@ ConstraintSolver.PackagesResolver.prototype._loadPackageInfo = function ( // - type - constraint type // options: // - upgrade - list of dependencies for which upgrade is prioritized higher -// than keeping the old version +// than keeping the old version // - previousSolution - mapping from package name to a version that was used in -// the previous constraint solver run +// the previous constraint solver run +// - anticipatedPrereleases: mapping from package name to version to true; +// included versions are the only pre-releases that are allowed to match +// constraints that don't specifically name them during the "try not to +// use unanticipated pre-releases" pass ConstraintSolver.PackagesResolver.prototype.resolve = function ( dependencies, constraints, options) { var self = this; // clone because we mutate options options = _.extend({ _testing: false, - upgrade: [] + upgrade: [], + anticipatedPrereleases: {} }, options || {}); check(dependencies, [String]); @@ -131,7 +136,9 @@ ConstraintSolver.PackagesResolver.prototype.resolve = function ( check(options, { _testing: Match.Optional(Boolean), upgrade: [String], - previousSolution: Match.Optional(Object) + previousSolution: Match.Optional(Object), + anticipatedPrereleases: Match.Optional( + Match.ObjectWithValues(Match.ObjectWithValues(Boolean))) }); _.each(dependencies, function (packageName) { @@ -166,6 +173,8 @@ ConstraintSolver.PackagesResolver.prototype.resolve = function ( options.rootDependencies = dependencies; var resolverOptions = self._getResolverOptions(options); var res = null; + var neededToUseUnanticipatedPrereleases = false; + // If a previous solution existed, try resolving with additional (weak) // equality constraints on all the versions from the previous solution (except // those we've explicitly been asked to update). If it's possible to solve the @@ -208,17 +217,19 @@ ConstraintSolver.PackagesResolver.prototype.resolve = function ( } } - // As a last-ditch effort, let's take a look at all the prerelease - // versions. Is it possible that a pre-release version will satisfy our - // constraints? + // As a last-ditch effort, let's allow ANY pre-release version found in the + // catalog, not only those that are asked for at some level. if (!res) { - resolverOptions["useRCs"] = true; + resolverOptions.anticipatedPrereleases = true; + neededToUseUnanticipatedPrereleases = true; + // Unlike the previous calls, this one throws a constraintSolverError on + // failure. res = self.resolver.resolve(dependencies, constraints, resolverOptions); } - var ret = { answer: resolverResultToPackageMap(res) }; - if (resolverOptions.useRCs) - ret.usedRCs = true; - return ret; + return { + answer: resolverResultToPackageMap(res), + neededToUseUnanticipatedPrereleases: neededToUseUnanticipatedPrereleases + }; }; var resolverResultToPackageMap = function (choices) { @@ -245,7 +256,9 @@ ConstraintSolver.PackagesResolver.prototype._getResolverOptions = function (options) { var self = this; - var resolverOptions = {}; + var resolverOptions = { + anticipatedPrereleases: options.anticipatedPrereleases + }; if (options._testing) { resolverOptions.costFunction = function (state) { diff --git a/packages/constraint-solver/resolver-tests.js b/packages/constraint-solver/resolver-tests.js index e0283e32f2e..cf4f6aa3793 100644 --- a/packages/constraint-solver/resolver-tests.js +++ b/packages/constraint-solver/resolver-tests.js @@ -147,13 +147,15 @@ Tinytest.add("constraint solver - resolver, cost function - avoid upgrades", fun Tinytest.add("constraint solver - resolver, don't pick rcs", function (test) { var resolver = new ConstraintSolver.Resolver(); + var A100rc1 = new ConstraintSolver.UnitVersion("a", "1.0.0-rc.1"); + var A100rc2 = new ConstraintSolver.UnitVersion("a", "1.0.0-rc.2"); var A100 = new ConstraintSolver.UnitVersion("a", "1.0.0"); - var A100rc1 = new ConstraintSolver.UnitVersion("a", "1.0.0-rc1"); resolver.addUnitVersion(A100rc1); + resolver.addUnitVersion(A100rc2); resolver.addUnitVersion(A100); var basicConstraint = resolver.getConstraint("a", ""); - var rcConstraint = resolver.getConstraint("a", "1.0.0-rc1"); + var rc1Constraint = resolver.getConstraint("a", "1.0.0-rc.1"); // Make the non-rc one more costly. But we still shouldn't choose it unless it // was specified in an initial constraint! @@ -162,19 +164,49 @@ Tinytest.add("constraint solver - resolver, don't pick rcs", function (test) { var name = mori.first(nameAndUv); var uv = mori.last(nameAndUv); // Make the non-rc one more costly. But we still shouldn't choose it! - if (uv.version === "1.0.0") - return 100; - return 0; + switch (uv.version) { + case "1.0.0": return 100; + case "1.0.0-rc.1": return 50; + case "1.0.0-rc.2": return 0; + } + throw Error("unknown version " + uv.version); }, state.choices)); }; - var solution = resolver.resolve( - ["a"], [basicConstraint], {costFunction: proRcCostFunction }); + // No RCs are mentioned in a constraint or known as anticipated. + var solution = resolver.resolve(["a"], [basicConstraint], { + costFunction: proRcCostFunction + }); resultEquals(test, solution, [A100]); - solution = resolver.resolve( - ["a"], [rcConstraint], {costFunction: proRcCostFunction }); + // All RCs are anticipated, so we choose the cheapest one (rc.2). + solution = resolver.resolve(["a"], [basicConstraint], { + costFunction: proRcCostFunction, + anticipatedPrereleases: true + }); + resultEquals(test, solution, [A100rc2]); + + // Only RC1 is anticipated, and it is cheaper than 1.0.0, so we choose it. + solution = resolver.resolve(["a"], [basicConstraint], { + costFunction: proRcCostFunction, + anticipatedPrereleases: {a: {"1.0.0-rc.1": true}} + }); resultEquals(test, solution, [A100rc1]); + + // No RCs are anticipated but the constraint mentions a prerelease so we allow + // any prerelease (specifically, the cheapest). + solution = resolver.resolve(["a"], [rc1Constraint], { + costFunction: proRcCostFunction + }); + resultEquals(test, solution, [A100rc2]); + + // All RCs are anticipated so we choose the cheapest, not the one mentioned + // explicitly in the constraint. + solution = resolver.resolve(["a"], [rc1Constraint], { + costFunction: proRcCostFunction, + anticipatedPrereleases: true + }); + resultEquals(test, solution, [A100rc2]); }); function semver2number (semverStr) { diff --git a/packages/constraint-solver/resolver.js b/packages/constraint-solver/resolver.js index 315a66bb497..cd0718b8f7b 100644 --- a/packages/constraint-solver/resolver.js +++ b/packages/constraint-solver/resolver.js @@ -94,10 +94,11 @@ ConstraintSolver.Resolver.prototype.resolve = function ( }, combineCostFunction: function (cost, anotherCost) { return cost + anotherCost; - } + }, + anticipatedPrereleases: {} }, options); - var resolveContext = new ResolveContext; + var resolveContext = new ResolveContext(options.anticipatedPrereleases); // Mapping that assigns every package an integer priority. We compute this // dynamically and in the process of resolution we try to resolve packages @@ -110,33 +111,8 @@ ConstraintSolver.Resolver.prototype.resolve = function ( var startState = new ResolverState(self, resolveContext); - if (options.useRCs) { - resolveContext.useRCsOK = true; - } - _.each(constraints, function (constraint) { startState = startState.addConstraint(constraint, mori.list()); - - // Keep track of any top-level constraints that mention a pre-release. - // These will be the only pre-release versions that count as "reasonable" - // for "any-reasonable" (ie, unconstrained) constraints. - // - // Why only top-level mentions, and not mentions we find while walking the - // graph? The constraint solver assumes that adding a constraint to the - // resolver state can't make previously impossible choices now possible. If - // pre-releases mentioned anywhere worked, then applying the constraints - // "any reasonable" followed by "1.2.3-rc1" would result in "1.2.3-rc1" - // ruled first impossible and then possible again. That's no good, so we - // have to fix the meaning based on something at the start. (We could try - // to apply our prerelease-avoidance tactics solely in the cost functions, - // but then it becomes a much less strict rule.) - if (constraint.version && /-/.test(constraint.version)) { - if (!_.has(resolveContext.topLevelPrereleases, constraint.name)) { - resolveContext.topLevelPrereleases[constraint.name] = {}; - } - resolveContext.topLevelPrereleases[constraint.name][constraint.version] - = true; - } }); _.each(dependencies, function (unitName) { @@ -354,19 +330,35 @@ ConstraintSolver.Constraint.prototype.isSatisfied = function ( var prereleaseNeedingLicense = false; - // We try not to allow "pre-release" versions (versions with a '-') - // unless they are explicitly mentioned. If the `useRCsOK` flag is - // set, all pre-release versions are allowed. Pre-release versions - // mentioned explicitly in top-level constraints are always allowed. - // Otherwise, if `candidateUV` is a pre-release, it needs to be - // "licensed" by being mentioned by name in this constraint or - // matched by an inexact constraint whose version also has a '-'. - if ((! resolveContext.useRCsOK) && /-/.test(candidateUV.version)) { - var isTopLevelPrerelease = ( - _.has(resolveContext.topLevelPrereleases, self.name) && - _.has(resolveContext.topLevelPrereleases[self.name], + // We try not to allow "pre-release" versions (versions with a '-') unless + // they are explicitly mentioned. If the `anticipatedPrereleases` option is + // `true` set, all pre-release versions are allowed. Otherwise, + // anticipatedPrereleases lists pre-release versions that are always allow + // (this corresponds to pre-release versions mentioned explicitly in + // *top-level* constraints). + // + // Otherwise, if `candidateUV` is a pre-release, it needs to be "licensed" by + // being mentioned by name in *this* constraint or matched by an inexact + // constraint whose version also has a '-'. + // + // Note that a constraint "@2.0.0" can never match a version "2.0.1-rc.1" + // unless anticipatedPrereleases allows it, even if another constraint found + // in the graph (but not at the top level) explicitly mentions "2.0.1-rc.1". + // Why? The constraint solver assumes that adding a constraint to the resolver + // state can't make previously impossible choices now possible. If + // pre-releases mentioned anywhere worked, then applying the constraint + // "@2.0.0" followed by "@=2.0.1-rc.1" would result in "2.0.1-rc.1" ruled + // first impossible and then possible again. That will break this algorith, so + // we have to fix the meaning based on something known at the start of the + // search. (We could try to apply our prerelease-avoidance tactics solely in + // the cost functions, but then it becomes a much less strict rule.) + if (resolveContext.anticipatedPrereleases !== true + && /-/.test(candidateUV.version)) { + var isAnticipatedPrerelease = ( + _.has(resolveContext.anticipatedPrereleases, self.name) && + _.has(resolveContext.anticipatedPrereleases[self.name], candidateUV.version)); - if (! isTopLevelPrerelease) { + if (! isAnticipatedPrerelease) { prereleaseNeedingLicense = true; } } @@ -408,9 +400,9 @@ ConstraintSolver.Constraint.prototype.isSatisfied = function ( // An object that records the general context of a resolve call. It can be // different for different resolve calls on the same Resolver, but is the same // for every ResolverState in a given call. -var ResolveContext = function () { +var ResolveContext = function (anticipatedPrereleases) { var self = this; - // unitName -> version string -> true - self.topLevelPrereleases = {}; - self.useRCsOK = false; + // EITHER: "true", in which case all prereleases are anticipated, or a map + // unitName -> version string -> true + self.anticipatedPrereleases = anticipatedPrereleases; }; diff --git a/tools/project-context.js b/tools/project-context.js index c9a69fd8da9..cef6f86d478 100644 --- a/tools/project-context.js +++ b/tools/project-context.js @@ -346,12 +346,16 @@ _.extend(exports.ProjectContext.prototype, { buildmessage.assertInCapture(); var depsAndConstraints = self._getRootDepsAndConstraints(); + var cachedVersions = self.packageMapFile.getCachedVersions(); + var anticipatedPrereleases = self._getAnticipatedPrereleases( + depsAndConstraints.constraints, cachedVersions); var resolver = self._buildResolver(); var solution; buildmessage.enterJob("selecting package versions", function () { var resolveOptions = { - previousSolution: self.packageMapFile.getCachedVersions() + previousSolution: cachedVersions, + anticipatedPrereleases: anticipatedPrereleases }; if (self._upgradePackageNames) resolveOptions.upgrade = self._upgradePackageNames; @@ -370,8 +374,8 @@ _.extend(exports.ProjectContext.prototype, { if (!solution) return; // error is already in buildmessage - // XXX #3006 Check solution.usedRCs and maybe print something about it. This - // code used to exist in catalog.js. + // XXX #3006 Check solution.neededToUseUnanticipatedPrereleases and maybe + // print something about it. This code used to exist in catalog.js. // XXX #3006 For commands other than create and test-packages, show package // changes. This code used to exist in project.js. #ShowPackageChanges @@ -475,6 +479,37 @@ _.extend(exports.ProjectContext.prototype, { }); }, + _getAnticipatedPrereleases: function (rootConstraints, cachedVersions) { + var self = this; + + var anticipatedPrereleases = {}; + var add = function (packageName, version) { + if (! /-/.test(version)) { + return; + } + if (! _.has(anticipatedPrereleases, packageName)) { + anticipatedPrereleases[packageName] = {}; + } + anticipatedPrereleases[packageName][version] = true; + }; + + // Pre-release versions that are root constraints (in .meteor/packages, in + // the release, or the version of a local package) are anticipated. + _.each(rootConstraints, function (constraintObject) { + _.each(constraintObject.constraints, function (alternative) { + var version = alternative.version; + version && add(constraintObject.name, version); + }); + }); + + // Pre-release versions we decided to use in the past are anticipated. + _.each(cachedVersions, function (version, packageName) { + add(packageName, version); + }); + + return anticipatedPrereleases; + }, + _buildResolver: function () { var self = this; @@ -787,8 +822,9 @@ _.extend(exports.PackageMapFile.prototype, { // Note that this is really specific to wanting to know what versions are in // the .meteor/versions file on disk, which is a slightly different question - // from "so, what versions should I be building with?" Usually you want a - // PackageMap instead! + // from "so, what versions should I be building with?" Usually you want the + // PackageMap produced by resolving constraints instead! Returns a map from + // package name to version. getCachedVersions: function () { var self = this; return _.clone(self._versions);