Improve linting of packages

glasser · glasser · commit 640ab28544a5 · 2015-07-09T16:39:57.000-07:00
- Only lint packages when we're actually going to use the output
- The linter adds the files it uses to the corresponding unibuild WatchSets
diff --git a/tools/bundler.js b/tools/bundler.js
@@ -1929,7 +1929,6 @@ Find out more about Meteor at meteor.com.
  *   fallback option for Windows). For isopacks (meteor packages), link to the
  *   location of the locally stored isopack build (e.g.
  *   ~/.meteor/packages/package/version/npm)
- * - lint: specifies if linting is required
  *
  * - buildOptions: may include
  *   - minify: string, type of minification for the CSS and JS assets
@@ -1971,12 +1970,11 @@ exports.bundle = function ({
   outputPath,
   includeNodeModules,
   buildOptions,
-  lint: shouldLint,
   previousBuilders,
   hasCachedBundle
 }) {
   buildOptions = buildOptions || {};
-  shouldLint = shouldLint || false;
+  const shouldLint = !!projectContext.lintAppAndLocalPackages;
 
   var appDir = projectContext.projectDir;
 
diff --git a/tools/commands.js b/tools/commands.js
@@ -275,7 +275,8 @@ function doRunCommand (options) {
 
   var projectContext = new projectContextModule.ProjectContext({
     projectDir: options.appDir,
-    allowIncompatibleUpdate: options['allow-incompatible-update']
+    allowIncompatibleUpdate: options['allow-incompatible-update'],
+    lintAppAndLocalPackages: !options['no-lint']
   });
 
   main.captureAndExit("=> Errors while initializing project:", function () {
@@ -400,7 +401,6 @@ function doRunCommand (options) {
     appHost: appHost,
     debugPort: options['debug-port'],
     settingsFile: options.settings,
-    lint: ! options['no-lint'],
     buildOptions: {
       minify: options.production ? 'production' : 'development',
       includeDebug: ! options.production
@@ -983,7 +983,8 @@ main.registerCommand({
   var projectContext = new projectContextModule.ProjectContext({
     projectDir: options.appDir,
     serverArchitectures: [archinfo.host()],
-    allowIncompatibleUpdate: options['allow-incompatible-update']
+    allowIncompatibleUpdate: options['allow-incompatible-update'],
+    lintAppAndLocalPackages: true
   });
   var messages = buildmessage.capture(function () {
     projectContext.prepareProjectForBuild();
@@ -998,7 +999,6 @@ main.registerCommand({
   var bundle = bundler.bundle({
     projectContext: projectContext,
     outputPath: null,
-    lint: true,
     buildOptions: {
       minify: 'development'
     }
@@ -1445,7 +1445,8 @@ main.registerCommand({
     projectDirForLocalPackages: options.appDir,
     explicitlyAddedLocalPackageDirs: packagesByPath,
     serverArchitectures: serverArchitectures,
-    allowIncompatibleUpdate: options['allow-incompatible-update']
+    allowIncompatibleUpdate: options['allow-incompatible-update'],
+    lintAppAndLocalPackages: !options['no-lint']
   });
 
   main.captureAndExit("=> Errors while setting up tests:", function () {
@@ -1608,8 +1609,7 @@ var getTestPackageNames = function (projectContext, packageNames) {
 var runTestAppForPackages = function (projectContext, options) {
   var buildOptions = {
     minify: options.production ? 'production' : 'development',
-    includeDebug: ! options.production,
-    lint: ! options['no-lint']
+    includeDebug: ! options.production
   };
 
   if (options.deploy) {
diff --git a/tools/compiler.js b/tools/compiler.js
@@ -175,8 +175,8 @@ compiler.lint = function (packageSource, options) {
     // skip Cordova if not required
     if (! options.includeCordovaUnibuild
         && architecture.arch === 'web.cordova') {
-          return;
-        }
+      return;
+    }
 
     lintUnibuild({
       isopack: options.isopack,
@@ -246,14 +246,20 @@ var lintUnibuild = function ({isopack, isopackCache, sourceArch}) {
     return;
   }
 
-  var watchSet = new watch.WatchSet;
-  var sourceItems = sourceArch.getSourcesFunc(sourceProcessorSet, watchSet);
+  const unibuild = _.findWhere(isopack.unibuilds, {arch: sourceArch.arch});
+  if (! unibuild) {
+    throw Error(`No ${ sourceArch.arch } unibuild for ${ isopack.name }!`);
+  }
+
+  var sourceItems = sourceArch.getSourcesFunc(
+    sourceProcessorSet, unibuild.watchSet);
 
   runLinters({
-    inputSourceArch: sourceArch,
     isopackCache,
     sourceItems,
-    sourceProcessorSet
+    sourceProcessorSet,
+    inputSourceArch: sourceArch,
+    watchSet: unibuild.watchSet
   });
 };
 
@@ -525,7 +531,7 @@ var compileUnibuild = function (options) {
 };
 
 function runLinters({inputSourceArch, isopackCache, sourceItems,
-                     sourceProcessorSet}) {
+                     sourceProcessorSet, watchSet}) {
   if (sourceProcessorSet.isEmpty()) {
     return;
   }
@@ -591,7 +597,6 @@ function runLinters({inputSourceArch, isopackCache, sourceItems,
     }
 
     // Read the file and add it to the WatchSet.
-    // XXX BBP there is no watchSet yet!!!!!!!
     const {hash, contents} = watch.readAndWatchFileWithHash(
       watchSet,
       files.pathResolve(inputSourceArch.pkg.sourceRoot, relPath));
diff --git a/tools/isopack-cache.js b/tools/isopack-cache.js
@@ -50,6 +50,8 @@ exports.IsopackCache = function (options) {
 
   self._noLineNumbers = !! options.noLineNumbers;
 
+  self._lintLocalPackages = !! options.lintLocalPackages;
+
   self.allLoadedLocalPackagesWatchSet = new watch.WatchSet;
 };
 
@@ -226,6 +228,8 @@ _.extend(exports.IsopackCache.prototype, {
       var isopack;
       if (previousIsopack && self._checkUpToDatePreloaded(previousIsopack)) {
         isopack = previousIsopack;
+        // We don't need to call self._lintLocalPackage here, because
+        // lintingMessages is saved on the isopack.
       } else {
         var pluginCacheDir;
         if (self._pluginCacheDirRoot) {
@@ -241,8 +245,7 @@ _.extend(exports.IsopackCache.prototype, {
           // Reuse existing plugin cache dir
           pluginCacheDir && files.mkdir_p(pluginCacheDir);
 
-          var Isopack = isopackModule.Isopack;
-          isopack = new Isopack();
+          isopack = new isopackModule.Isopack();
           isopack.initFromPath(name, self._isopackDir(name), {
             isopackBuildInfoJson: isopackBuildInfoJson,
             pluginCacheDir: pluginCacheDir
@@ -255,6 +258,10 @@ _.extend(exports.IsopackCache.prototype, {
           isopack.setPluginProviderPackageMap(
             self._packageMap.makeSubsetMap(
               _.keys(isopackBuildInfoJson.pluginProviderPackageMap)));
+          // Because we don't save linter messages to disk, we have to relint
+          // this package.
+          // XXX save linter messages to disk?
+          self._lintLocalPackage(packageInfo.packageSource, isopack);
         } else {
           // Nope! Compile it again. Give it a fresh plugin cache.
           if (pluginCacheDir) {
@@ -272,37 +279,45 @@ _.extend(exports.IsopackCache.prototype, {
           // Accept the compiler's result, even if there were errors (since it
           // at least will have a useful WatchSet and will allow us to keep
           // going and compile other packages that depend on this one). However,
-          // only save it to disk if there were no errors.
-          if (self.cacheDir && ! buildmessage.jobHasMessages()) {
-            // Save to disk, for next time!
-            isopack.saveToPath(self._isopackDir(name), {
-              includeIsopackBuildInfo: true
-            });
+          // only lint it and save it to disk if there were no errors.
+          if (! buildmessage.jobHasMessages()) {
+            // Lint the package. We do this before saving so that the linter can
+            // augment the saved-to-disk WatchSet with linter-specific files.
+            self._lintLocalPackage(packageInfo.packageSource, isopack);
+            if (self.cacheDir) {
+              // Save to disk, for next time!
+              isopack.saveToPath(self._isopackDir(name), {
+                includeIsopackBuildInfo: true
+              });
+            }
           }
         }
       }
 
       self.allLoadedLocalPackagesWatchSet.merge(isopack.getMergedWatchSet());
       self._isopacks[name] = isopack;
+    });
+  },
 
-      // lint isopack if compilation succeeded or if we loaded it from cache
-      if (! buildmessage.jobHasMessages()) {
-        var lintingMessages = buildmessage.capture({
-          title: "linting isopack " + name
-        }, function () {
-          compiler.lint(packageInfo.packageSource, {
-            isopackCache: self,
-            isopack: isopack,
-            includeCordovaUnibuild: self._includeCordovaUnibuild
-          });
-        });
-        if (! lintingMessages.hasMessages()) {
-          lintingMessages = null;
-        }
+  // Runs appropriate linters on a package. It also augments their unibuilds'
+  // WatchSets with files used by the linter.
+  _lintLocalPackage(packageSource, isopack) {
+    const self = this;
+    if (! self._lintLocalPackages)
+      return;
 
-        isopack.lintingMessages = lintingMessages;
-      }
+    const lintingMessages = buildmessage.capture({
+      title: "linting isopack " + isopack.name
+    }, function () {
+      compiler.lint(packageSource, {
+        isopackCache: self,
+        isopack: isopack,
+        includeCordovaUnibuild: self._includeCordovaUnibuild
+      });
     });
+    if (lintingMessages.hasMessages()) {
+      isopack.lintingMessages = lintingMessages;
+    }
   },
 
   _checkUpToDate: function (isopackBuildInfoJson) {
@@ -405,7 +420,6 @@ _.extend(exports.IsopackCache.prototype, {
 
     var messages = new buildmessage._MessageSet();
     self._packageMap.eachPackage(function (name, packageInfo) {
-      var packageInfo = self._packageMap.getInfo(name);
       var isopack = self._isopacks[name];
       if (packageInfo.kind === 'local') {
         var isopackMessages = isopack.lintingMessages;
diff --git a/tools/project-context.js b/tools/project-context.js
@@ -159,6 +159,11 @@ _.extend(ProjectContext.prototype, {
     // them or change their major version).
     self._allowIncompatibleUpdate = options.allowIncompatibleUpdate;
 
+    // If set, we run the linter on the app and local packages.  Set by 'meteor
+    // lint', and the runner commands (run/test-packages/debug) when --no-lint
+    // is not passed.
+    self.lintAppAndLocalPackages = options.lintAppAndLocalPackages;
+
     // Initialized by readProjectMetadata.
     self.releaseFile = null;
     self.projectConstraintsFile = null;
@@ -711,7 +716,8 @@ _.extend(ProjectContext.prototype, {
       cacheDir: self.getProjectLocalDirectory('isopacks'),
       pluginCacheDirRoot: self.getProjectLocalDirectory('plugin-cache'),
       tropohouse: self.tropohouse,
-      previousIsopackCache: self._previousIsopackCache
+      previousIsopackCache: self._previousIsopackCache,
+      lintLocalPackages: self.lintAppAndLocalPackages
     });
 
     if (self._forceRebuildPackages) {
diff --git a/tools/run-app.js b/tools/run-app.js
@@ -70,7 +70,6 @@ var AppProcess = function (options) {
   self.proc = null;
   self.madeExitCallback = false;
   self.ipcPipe = options.ipcPipe;
-  self.lint = options.lint;
 };
 
 _.extend(AppProcess.prototype, {
@@ -354,7 +353,6 @@ var AppRunner = function (options) {
   self.mobileServerUrl = options.mobileServerUrl;
   self.settingsFile = options.settingsFile;
   self.debugPort = options.debugPort;
-  self.lint = options.lint;
   self.proxy = options.proxy;
   self.watchForChanges =
     options.watchForChanges === undefined ? true : options.watchForChanges;
@@ -562,7 +560,6 @@ _.extend(AppRunner.prototype, {
           includeNodeModules: includeNodeModules,
           buildOptions: self.buildOptions,
           hasCachedBundle: !! cachedServerWatchSet,
-          lint: self.lint,
           previousBuilders: builders
         });
 
@@ -711,7 +708,7 @@ _.extend(AppRunner.prototype, {
     }
 
     appProcess.start();
-    if (self.lint) {
+    if (self.projectContext.lintAppAndLocalPackages) {
       var warnings = bundleResult.warnings;
 
       if (warnings && warnings.hasMessages()) {
@@ -772,7 +769,8 @@ _.extend(AppRunner.prototype, {
         if (bundleResultOrRunResult.runResult)
           return bundleResultOrRunResult.runResult;
         bundleResult = bundleResultOrRunResult.bundleResult;
-        if (self.lint && bundleResult.warnings) {
+        if (self.projectContext.lintAppAndLocalPackages &&
+            bundleResult.warnings) {
           runLog.log(
             'Linting your app.\n\n' +
               bundleResult.warnings.formatMessages(), { arrow: true });
