Skip to content

Commit

Permalink
Fix anticipated pre-release calculation
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@glasser
glasser committed Dec 1, 2014
1 parent 0713b7c commit de95361
Show file tree
Hide file tree
Showing 5 changed files with 144 additions and 71 deletions.
2 changes: 1 addition & 1 deletion packages/constraint-solver/constraint-solver-tests.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
39 changes: 26 additions & 13 deletions packages/constraint-solver/constraint-solver.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Expand All @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down
50 changes: 41 additions & 9 deletions packages/constraint-solver/resolver-tests.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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!
Expand All @@ -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) {
Expand Down
78 changes: 35 additions & 43 deletions packages/constraint-solver/resolver.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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) {
Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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;
};
46 changes: 41 additions & 5 deletions tools/project-context.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit de95361

Please sign in to comment.