support github tarballs as dependencies

tools/meteor_npm.js

@@ -25,9 +25,18 @@ cleanup.onExit(function () {
 var meteorNpm = module.exports = {
   _tmpDirs: [],
 
+  _isGitHubTarball: function (x) {
+    return /^https:\/\/github.com\/.*\/tarball\/[0-9a-f]{40}/.test(x);
+  },
+
   ensureOnlyExactVersions: function(npmDependencies) {
+    var self = this;
     _.each(npmDependencies, function(version, name) {
-      if (!semver.valid(version))
+      // We want a given version of a smart package (package.js +
+      // .npm/npm-shrinkwrap.json) to pin down its dependencies precisely, so we
+      // don't want anything too vague. For now, we support semvers and github
+      // tarballs pointing at an exact commit.
+      if (!semver.valid(version) && !self._isGitHubTarball(version))
         throw new Error(
           "Must declare exact version of npm package dependency: " + name + '@' + version);
     });
@@ -121,13 +130,7 @@ var meteorNpm = module.exports = {
         }
       });
 
-      // if we had no installed dependencies to begin with, *DON'T*
-      // shrinkwrap. this is important so that we can pin versions of
-      // deep dependencies to tarballs, e.g.
-      // https://github.com/meteor/js-bson/tarball/master
-      if (!_.isEmpty(installedDependencies)) {
-        self._shrinkwrap(newPackageNpmDir);
-      }
+      self._shrinkwrap(newPackageNpmDir);
       self._createReadme(newPackageNpmDir);
       self._renameAlmostAtomically(newPackageNpmDir, packageNpmDir);
     }
@@ -242,24 +245,30 @@ var meteorNpm = module.exports = {
 
   // map the structure returned from `npm ls` into the structure of
   // npmDependencies (e.g. {gcd: '0.0.0'}), so that they can be
-  // diffed.
+  // diffed. This only returns top-level dependencies.
   _installedDependencies: function(dir) {
     var self = this;
     return _.object(
       _.map(
         self._installedDependenciesTree(dir).dependencies, function(properties, name) {
-          return [name, properties.version];
+          if (self._isGitHubTarball(properties.from))
+            return [name, properties.from];
+          else
+            return [name, properties.version];
         }));
   },
 
   _installNpmModule: function(name, version, dir) {
     this._ensureConnected();
 
+    var installArg = this._isGitHubTarball(version)
+          ? version : (name + "@" + version);
+
     // We don't use npm.commands.install since we couldn't
     // figure out how to silence all output (specifically the
     // installed tree which is printed out with `console.log`)
     this._execFileSync(path.join(files.get_dev_bundle(), "bin", "npm"),
-                       ["install", name + "@" + version],
+                       ["install", installArg],
                        {cwd: dir});
   },
 

tools/tests/test_bundler_npm.js

@@ -40,15 +40,20 @@ var _assertCorrectPackageNpmDir = function(deps) {
   // to write these tests, so just transplant that information
   var actualMeteorNpmShrinkwrapDependencies = JSON.parse(fs.readFileSync(path.join(testPackageDir, ".npm", "npm-shrinkwrap.json"), 'utf8')).dependencies;
   var expectedMeteorNpmShrinkwrapDependencies = _.object(_.map(deps, function(version, name) {
-    var val = {version: version};
+    var expected = {};
+    if (/tarball/.test(version)) {
+      expected.from = version;
+    } else {
+      expected.version = version;
+    }
 
     // copy fields with values generated by shrinkwrap that can't be known to the
-    // test author.
-    //
-    // 'from' and 'resolved' were added when we upraded
-    // to Node 0.8.18 (5a54a5c79f71b7449c58d7ee809086bc0a542a15)
-    _.each(['dependencies', 'from', 'resolved'], function(key) {
-      if (actualMeteorNpmShrinkwrapDependencies[name][key])
+    // test author. We set keys on val always in this order so that comparison works well.
+    var val = {};
+    _.each(['version', 'from', 'resolved', 'dependencies'], function(key) {
+      if (expected[key])
+        val[key] = expected[key];
+      else if (actualMeteorNpmShrinkwrapDependencies[name][key])
         val[key] = actualMeteorNpmShrinkwrapDependencies[name][key];
     });
 
@@ -70,14 +75,13 @@ var _assertCorrectPackageNpmDir = function(deps) {
 
   // all expected dependencies are installed correctly, with the correct version
   _.each(deps, function(version, name) {
-    // presumably if this file is here we have correctly installed the package
-    assert(fs.existsSync(path.join(nodeModulesDir, name, 'LICENSE')));
+    assert(looksInstalled(nodeModulesDir, name));
 
     assert.equal(JSON.parse(
       fs.readFileSync(
         path.join(nodeModulesDir, name, "package.json"),
         'utf8')).version,
-                 version);
+                 expectedMeteorNpmShrinkwrapDependencies[name].version);
   });
 
   // all installed dependencies were expected to be found there,
@@ -99,16 +103,24 @@ var _assertCorrectBundleNpmContents = function(bundleDir, deps) {
 
   // bundle actually has the npm modules
   _.each(deps, function(version, name) {
-    // presumably if this file is here we have correctly installed the package
-    assert(fs.existsSync(path.join(bundledPackageNodeModulesDir, name, 'LICENSE')));
+    assert(looksInstalled(bundledPackageNodeModulesDir, name));
 
-    assert.equal(JSON.parse(
-      fs.readFileSync(path.join(bundledPackageNodeModulesDir, name, 'package.json'), 'utf8'))
-                 .version,
-                 version);
+    if (!/tarball/.test(version)) {
+      assert.equal(JSON.parse(
+        fs.readFileSync(path.join(bundledPackageNodeModulesDir, name, 'package.json'), 'utf8'))
+                   .version,
+                   version);
+    }
   });
 };
 
+var looksInstalled = function (nodeModulesDir, name) {
+  // All of the packages in this test have one of these two files, so presumably
+  // if one of these files is here we have correctly installed the package.
+  return fs.existsSync(path.join(nodeModulesDir, name, 'README.md')) ||
+    fs.existsSync(path.join(nodeModulesDir, name, 'LICENSE'));
+};
+
 ///
 /// TESTS
 ///
@@ -219,13 +231,15 @@ assert.doesNotThrow(function () {
   _assertCorrectBundleNpmContents(tmpOutputDir, {gcd: '0.0.0', mime: '1.2.7'});
 });
 
-
 console.log("bundle multiple apps in parallel using a meteor package dependent on an npm package");
 // this fails if we don't manage the package .npm directory correctly
 // against parallel bundling.  this happens if you are running more
 // than one app at once using a certain package and that package is
 // updated.
+// XXX This doesn't pass if it has to remove a module! eg, move the gzippo test
+//     that is below to above this one. It will fail!
 assert.doesNotThrow(function () {
+  updateTestPackage({gcd: '0.0.0', mime: '1.2.7'});
   // rm -rf .npm/node_modules, to make sure installing modules takes some time
   var nodeModulesDir = path.join(testPackageDir, ".npm", "node_modules");
   assert(fs.existsSync(path.join(nodeModulesDir)));
@@ -265,6 +279,21 @@ assert.doesNotThrow(function () {
 });
 
 
+console.log("app that uses gcd - install gzippo via tarball");
+assert.doesNotThrow(function () {
+  var deps = {gzippo: 'https://github.com/meteor/gzippo/tarball/1e4b955439abc643879ae264b28a761521818f3b'};
+  updateTestPackage(deps);
+  var tmpOutputDir = tmpDir();
+  var errors = bundler.bundle(appWithPackageDir, tmpOutputDir, {nodeModulesMode: 'skip', releaseStamp: 'none'});
+  assert.strictEqual(errors, undefined, errors && errors[0]);
+  _assertCorrectPackageNpmDir(deps);
+  _assertCorrectBundleNpmContents(tmpOutputDir, deps);
+  // Check that a string introduced by our fork is in the source.
+  assert(/clientMaxAge = 604800000/.test(
+    fs.readFileSync(
+      path.join(testPackageDir, ".npm", "node_modules", "gzippo", "lib", "staticGzip.js"), "utf8")));
+});
+
 
 
 ///