From eb830870467747aaf95e32c90e24efd476a800db Mon Sep 17 00:00:00 2001
From: David Greenspan <dgreenspan@alum.mit.edu>
Date: Wed, 1 Oct 2014 16:42:14 -0700
Subject: [PATCH] Publish versioned dependencies of core packages
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When publishing a core package, you’re allowed to omit version numbers in package.js.  With this change, we determine the correct versions of all the dependent packages based on the current checkout, and we send them to troposphere (instead of core packages having “null” constraints on their dependencies).

We throw an error if you have any missing version constraints on your package dependencies and you are not using versionsFrom or publishing a core package from a checkout.

We’ve already fixed this (no constraints on core package deps) retroactively in troposphere.

It speeds up the constraint solver by orders of magnitude when publishing a package.
---
 packages/constraint-solver/resolver.js |  2 +-
 tools/package-client.js                | 36 +++++++++++++++++++-------
 tools/package-version-parser.js        |  9 +++++--
 3 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/packages/constraint-solver/resolver.js b/packages/constraint-solver/resolver.js
index d7fa36b3ab..77d318b727 100644
--- a/packages/constraint-solver/resolver.js
+++ b/packages/constraint-solver/resolver.js
@@ -311,7 +311,7 @@ ConstraintSolver.UnitVersion = function (name, unitVersion, ecv) {
   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!)
-  self.version = unitVersion.replace(/\+.*$/, '');
+  self.version = PackageVersion.removeBuildID(unitVersion);
   self.dependencies = [];
   self.constraints = new ConstraintSolver.ConstraintsList();
   // a string in a form of "1.2.0"
diff --git a/tools/package-client.js b/tools/package-client.js
index 1061cb5f2f..878c424896 100644
--- a/tools/package-client.js
+++ b/tools/package-client.js
@@ -13,6 +13,7 @@ var buildmessage = require('./buildmessage.js');
 var compiler = require('./compiler.js');
 var uniload = require('./uniload.js');
 var Console = require('./console.js').Console;
+var packageVersionParser = require('./package-version-parser.js');
 
 // Use uniload to load the packages that we need to open a connection to the
 // current package server and use minimongo in memory. We need the following
@@ -537,22 +538,39 @@ exports.publishPackage = function (packageSource, compileResult, conn, options)
   var packageDeps =  packageSource.getDependencyMetadata();
   var badConstraints = [];
   _.each(packageDeps, function(refs, label) {
-    // HACK: we automatically include the meteor package and there is no way for
-    // anyone to set its dependency data correctly, so I guess we shouldn't
-    // penalize the user for not doing that. It will be resolved at runtime
-    // anyway.
-    if (label !== "meteor" &&
-        refs.constraint == null) {
-      badConstraints.push(label);
+    if (refs.constraint == null) {
+      if (packageSource.isCore && files.inCheckout() &&
+          catalog.complete.isLocalPackage(label)) {
+        // Core package is using or implying another core package,
+        // without a version number.  We fill in the version number.
+        var versionString = catalog.complete.getLatestVersion(label).version;
+        // strip suffix like "+local"
+        versionString = packageVersionParser.removeBuildID(versionString);
+        // modify the constraint on this dep that will be sent to troposphere
+        refs.constraint = versionString;
+      } else if (label === "meteor") {
+        // HACK: We are willing to publish a package with a "null"
+        // constraint on the "meteor" package to troposphere.  This
+        // happens for non-core packages when not running from a
+        // checkout, because all packages implicitly depend on the
+        // "meteor" package, but do not necessarily specify an
+        // explicit version for it, and we don't have a great way to
+        // choose one here.
+        // XXX come back to this, especially if we are incrementing the
+        // major version of "meteor".  hopefully we will have more data
+        // about the package system by then.
+      } else {
+        badConstraints.push(label);
+      }
     }
   });
 
   // If we are not a core package and some of our constraints are unspecified,
   // then we should force the user to specify them. This is because we are not
   // sure about pre-0.90 package versions yet.
-  if (!packageSource.isCore && !_.isEqual(badConstraints, [])) {
+  if (!_.isEqual(badConstraints, [])) {
     Console.stderr.write(
-"You must specify a version constraint for the following packages:");
+      "You must specify a version constraint for the following packages:");
     _.each(badConstraints, function(bad) {
       Console.stderr.write(" " + bad);
     });
diff --git a/tools/package-version-parser.js b/tools/package-version-parser.js
index 42fcbc8147..9f21119a45 100644
--- a/tools/package-version-parser.js
+++ b/tools/package-version-parser.js
@@ -188,8 +188,8 @@ PV.parseVersionConstraint = function (versionString, options) {
   PV.getValidServerVersion(versionString);
 
   if (options.removeBuildIDs) {
-    versionString = versionString.replace(/\+.*$/, '');
-   }
+    versionString = PV.removeBuildID(versionString);
+  }
 
   versionDesc.version = versionString;
 
@@ -340,3 +340,8 @@ PV.invalidFirstFormatConstraint = function (validConstraint) {
   return (/_/.test(validConstraint) ||
           /\|/.test(validConstraint));
 };
+
+// Remove a suffix like "+local" if present.
+PV.removeBuildID = function (versionString) {
+  return versionString.replace(/\+.*$/, '');
+};