diff --git a/tools/build-plugin.js b/tools/build-plugin.js index f3747cb256..3578e6394a 100644 --- a/tools/build-plugin.js +++ b/tools/build-plugin.js @@ -14,6 +14,7 @@ exports.SourceProcessor = function (options) { self.archMatching = options.archMatching; self.isTemplate = !! options.isTemplate; self.factoryFunction = options.factoryFunction; + self.methodName = options.methodName; self.id = `${ options.isopack.displayName() }#${ nextId++ }`; self.userPlugin = null; }; @@ -26,24 +27,29 @@ _.extend(exports.SourceProcessor.prototype, { // defined later in the file). instantiatePlugin: function () { var self = this; + buildmessage.assertInCapture(); if (self.userPlugin) { throw Error("Called instantiatePlugin twice?"); } - // XXX BBP proper error handling --- this is running user-supplied plugin - // code, and use markBoundary too - try { - self.userPlugin = buildmessage.markBoundary(self.factoryFunction).call( - null); - // If we have a disk cache directory and the plugin wants it, use it. - if (self.isopack.pluginCacheDir && - self.userPlugin.setDiskCacheDirectory) { - const markedMethod = buildmessage.markBoundary( - self.userPlugin.setDiskCacheDirectory.bind(self.userPlugin)); - markedMethod(self.isopack.pluginCacheDir); + buildmessage.enterJob( + `running ${self.methodName} callback in package ` + + self.isopack.displayName(), + () => { + try { + self.userPlugin = buildmessage.markBoundary(self.factoryFunction) + .call(null); + // If we have a disk cache directory and the plugin wants it, use it. + if (self.isopack.pluginCacheDir && + self.userPlugin.setDiskCacheDirectory) { + const markedMethod = buildmessage.markBoundary( + self.userPlugin.setDiskCacheDirectory.bind(self.userPlugin)); + markedMethod(self.isopack.pluginCacheDir); + } + } catch (e) { + buildmessage.exception(e); + } } - } catch (e) { - buildmessage.exception(e); - } + ); }, relevantForArch: function (arch) { var self = this; diff --git a/tools/bundler.js b/tools/bundler.js index aa16260a94..67afbcc96a 100644 --- a/tools/bundler.js +++ b/tools/bundler.js @@ -687,6 +687,7 @@ _.extend(Target.prototype, { _runCompilerPlugins: Profile("Target#_runCompilerPlugins", function () { var self = this; + buildmessage.assertInJob(); var processor = new compilerPluginModule.CompilerPluginProcessor({ unibuilds: self.unibuilds, arch: self.arch, @@ -2087,6 +2088,14 @@ exports.bundle = function ({ if (! buildmessage.jobHasMessages() && shouldLint) { lintingMessages = lintBundle(projectContext, app, packageSource); } + // If while trying to lint, we got a compilation error (eg, an issue loading + // plugins in one of the linter packages), restart on any relevant change, + // and be done. + if (buildmessage.jobHasMessages()) { + serverWatchSet.merge(projectContext.getProjectAndLocalPackagesWatchSet()); + serverWatchSet.merge(app.getMergedWatchSet()); + return; + } var minifiers = null; if (! _.contains(['development', 'production'], minifyMode)) { @@ -2096,6 +2105,13 @@ exports.bundle = function ({ isopackCache: projectContext.isopackCache, isopack: app }); + // If figuring out what the minifiers are failed (eg, clashing extension + // handlers), restart on any relevant change, and be done. + if (buildmessage.jobHasMessages()) { + serverWatchSet.merge(projectContext.getProjectAndLocalPackagesWatchSet()); + serverWatchSet.merge(app.getMergedWatchSet()); + return; + } var clientTargets = []; // Client @@ -2187,16 +2203,17 @@ exports.bundle = function ({ }; }; +// Returns null if there are no lint warnings and the app has no linters +// defined. Returns an empty MessageSet if the app has a linter defined but +// there are no lint warnings (on app or packages). function lintBundle (projectContext, isopack, packageSource) { + buildmessage.assertInJob(); + const lintingMessages = new buildmessage._MessageSet(); - const appMessages = buildmessage.capture({ - title: "linting the application" - }, function () { - compiler.lint(packageSource, { - isopackCache: projectContext.isopackCache, - isopack: isopack - }); + const appMessages = compiler.lint(packageSource, { + isopack, + isopackCache: projectContext.isopackCache }); if (appMessages) { lintingMessages.merge(appMessages); @@ -2211,7 +2228,7 @@ function lintBundle (projectContext, isopack, packageSource) { // if there was no linting performed since there are no applicable // linters for neither app nor packages, just return null const appLinters = isopack.sourceProcessors.linter; - if (_.isEmpty(appLinters) && ! localPackagesMessages) { + if (appLinters.isEmpty() && ! localPackagesMessages) { return null; } diff --git a/tools/compiler-plugin.js b/tools/compiler-plugin.js index da91d0245b..c8f4357a33 100644 --- a/tools/compiler-plugin.js +++ b/tools/compiler-plugin.js @@ -396,6 +396,8 @@ _.extend(ResourceSlot.prototype, { // XXX BBP ??? var PackageSourceBatch = function (unibuild, processor) { var self = this; + buildmessage.assertInJob(); + self.unibuild = unibuild; self.processor = processor; var sourceProcessorSet = self._getSourceProcessorSet(); @@ -436,6 +438,9 @@ var PackageSourceBatch = function (unibuild, processor) { _.extend(PackageSourceBatch.prototype, { _getSourceProcessorSet: function () { var self = this; + + buildmessage.assertInJob(); + var isopack = self.unibuild.pkg; const activePluginPackages = compiler.getActivePluginPackages(isopack, { uses: self.unibuild.uses, diff --git a/tools/compiler.js b/tools/compiler.js index ca35f35164..732f36cb70 100644 --- a/tools/compiler.js +++ b/tools/compiler.js @@ -197,6 +197,14 @@ compiler.compile = Profile(function (packageSource, options) { // - isopackCache // - includeCordovaUnibuild compiler.lint = function (packageSource, options) { + // Note: the buildmessage context of compiler.lint and lintUnibuild is a + // normal error message context (eg, there might be errors from initializing + // plugins in getLinterSourceProcessorSet). We return the linter warnings as + // our return value. + buildmessage.assertInJob(); + + const warnings = new buildmessage._MessageSet; + _.each(packageSource.architectures, function (architecture) { // skip Cordova if not required if (! options.includeCordovaUnibuild @@ -204,15 +212,21 @@ compiler.lint = function (packageSource, options) { return; } - lintUnibuild({ + const unibuildWarnings = lintUnibuild({ isopack: options.isopack, isopackCache: options.isopackCache, sourceArch: architecture }); + if (unibuildWarnings) { + warnings.merge(unibuildWarnings); + } }); + return warnings.hasMessages() ? warnings : null; }; compiler.getMinifiers = function (packageSource, options) { + buildmessage.assertInJob(); + var minifiers = []; _.each(packageSource.architectures, function (architecture) { var activePluginPackages = getActivePluginPackages(options.isopack, { @@ -246,6 +260,8 @@ compiler.getMinifiers = function (packageSource, options) { }; function getLinterSourceProcessorSet({isopack, activePluginPackages}) { + buildmessage.assertInJob(); + const sourceProcessorSet = new SourceProcessorSet( isopack.displayName, { allowConflicts: true }); @@ -259,6 +275,12 @@ function getLinterSourceProcessorSet({isopack, activePluginPackages}) { } var lintUnibuild = function ({isopack, isopackCache, sourceArch}) { + // Note: the buildmessage context of compiler.lint and lintUnibuild is a + // normal error message context (eg, there might be errors from initializing + // plugins in getLinterSourceProcessorSet). We return the linter warnings as + // our return value. + buildmessage.assertInJob(); + var activePluginPackages = getActivePluginPackages( isopack, { isopackCache, @@ -267,9 +289,10 @@ var lintUnibuild = function ({isopack, isopackCache, sourceArch}) { const sourceProcessorSet = getLinterSourceProcessorSet({isopack, activePluginPackages}); - // bail out early if there is not much to run - if (sourceProcessorSet.isEmpty()) { - return; + // bail out early if we had trouble loading plugins or if we're not + // going to lint anything + if (buildmessage.jobHasMessages() || sourceProcessorSet.isEmpty()) { + return null; } const unibuild = _.findWhere(isopack.unibuilds, {arch: sourceArch.arch}); @@ -280,13 +303,16 @@ var lintUnibuild = function ({isopack, isopackCache, sourceArch}) { var sourceItems = sourceArch.getSourcesFunc( sourceProcessorSet, unibuild.watchSet); - runLinters({ - isopackCache, - sourceItems, - sourceProcessorSet, - inputSourceArch: sourceArch, - watchSet: unibuild.watchSet + const linterMessages = buildmessage.capture(() => { + runLinters({ + isopackCache, + sourceItems, + sourceProcessorSet, + inputSourceArch: sourceArch, + watchSet: unibuild.watchSet + }); }); + return linterMessages; }; // options.sourceArch is a SourceArch to compile. Process all source files @@ -322,31 +348,39 @@ var compileUnibuild = function (options) { // *** Assemble the list of source file handlers from the plugins // XXX BBP redoc - var sourceProcessorSet = new SourceProcessorSet( - isopk.displayName(), { hardcodeJs: true}); + let sourceProcessorSet, linterSourceProcessorSet; + buildmessage.enterJob("determining active plugins", () => { + sourceProcessorSet = new SourceProcessorSet( + isopk.displayName(), { hardcodeJs: true}); - activePluginPackages.forEach((otherPkg) => { - otherPkg.ensurePluginsInitialized(); + activePluginPackages.forEach((otherPkg) => { + otherPkg.ensurePluginsInitialized(); - // Note that this may log a buildmessage if there are conflicts. - sourceProcessorSet.merge(otherPkg.sourceProcessors.compiler); - }); + // Note that this may log a buildmessage if there are conflicts. + sourceProcessorSet.merge(otherPkg.sourceProcessors.compiler); + }); - // Used to excuse functions from the "undeclared static asset" check. - const linterSourceProcessorSet = getLinterSourceProcessorSet({ - activePluginPackages, - isopack: isopk + // Used to excuse functions from the "undeclared static asset" check. + linterSourceProcessorSet = getLinterSourceProcessorSet({ + activePluginPackages, + isopack: isopk + }); + if (buildmessage.jobHasMessages()) { + // Recover by not calling getSourcesFunc and pretending there are no + // items. + sourceProcessorSet = null; + } }); // *** Determine source files - // Note: sourceExtensions does not include leading dots // Note: the getSourcesFunc function isn't expected to add its // source files to watchSet; rather, the watchSet is for other // things that the getSourcesFunc consulted (such as directory // listings or, in some hypothetical universe, control files) to // determine its source files. - var sourceItems = inputSourceArch.getSourcesFunc( - sourceProcessorSet, watchSet); + const sourceItems = sourceProcessorSet + ? inputSourceArch.getSourcesFunc(sourceProcessorSet, watchSet) + : []; if (nodeModulesPath) { // If this slice has node modules, we should consider the shrinkwrap file @@ -557,6 +591,10 @@ var compileUnibuild = function (options) { function runLinters({inputSourceArch, isopackCache, sourceItems, sourceProcessorSet, watchSet}) { + // The buildmessage context here is for linter warnings only! runLinters + // should not do anything that can have a real build failure. + buildmessage.assertInCapture(); + if (sourceProcessorSet.isEmpty()) { return; } diff --git a/tools/isopack-cache.js b/tools/isopack-cache.js index 9fe484b03a..dc3198d9c0 100644 --- a/tools/isopack-cache.js +++ b/tools/isopack-cache.js @@ -303,19 +303,16 @@ _.extend(exports.IsopackCache.prototype, { // WatchSets with files used by the linter. _lintLocalPackage(packageSource, isopack) { const self = this; + buildmessage.assertInJob(); if (! self._lintLocalPackages) return; - const lintingMessages = buildmessage.capture({ - title: "linting isopack " + isopack.name - }, function () { - compiler.lint(packageSource, { - isopackCache: self, - isopack: isopack, - includeCordovaUnibuild: self._includeCordovaUnibuild - }); + const lintingMessages = compiler.lint(packageSource, { + isopackCache: self, + isopack: isopack, + includeCordovaUnibuild: self._includeCordovaUnibuild }); - if (lintingMessages.hasMessages()) { + if (lintingMessages && lintingMessages.hasMessages()) { isopack.lintingMessages = lintingMessages; } }, diff --git a/tools/isopack.js b/tools/isopack.js index 46dd115af0..46b98840d2 100644 --- a/tools/isopack.js +++ b/tools/isopack.js @@ -452,6 +452,8 @@ _.extend(Isopack.prototype, { ensurePluginsInitialized: Profile("Isopack#ensurePluginsInitialized", function () { var self = this; + buildmessage.assertInJob(); + if (self._pluginsInitialized) return; @@ -494,7 +496,6 @@ _.extend(Isopack.prototype, { // Plugin.registerCompiler({...}, function () { return new C; }); // var C = function () {...} // and so we want to wait for C to be defined. - _.each(self.sourceProcessors, (sourceProcessorSet) => { _.each(sourceProcessorSet.allSourceProcessors, (sourceProcessor) => { sourceProcessor.instantiatePlugin(); @@ -608,7 +609,8 @@ _.extend(Isopack.prototype, { filenames: filenames, archMatching: archMatching, isTemplate: isTemplate, - factoryFunction: factory + factoryFunction: factory, + methodName: methodName }); // This logs a buildmessage on conflicts. sourceProcessorSet.addSourceProcessor(sp); diff --git a/tools/run-app.js b/tools/run-app.js index d06678393d..33aed03ee7 100644 --- a/tools/run-app.js +++ b/tools/run-app.js @@ -708,16 +708,22 @@ _.extend(AppRunner.prototype, { } appProcess.start(); - if (self.projectContext.lintAppAndLocalPackages) { - var warnings = bundleResult.warnings; - - if (warnings && warnings.hasMessages()) { - runLog.logTemporary('Linting your app.'); + function maybePrintLintWarnings(bundleResult) { + if (! (self.projectContext.lintAppAndLocalPackages && + bundleResult.warnings)) { + return; + } + if (bundleResult.warnings.hasMessages()) { + const formattedMessages = bundleResult.warnings.formatMessages(); runLog.log( - 'Linted your app.\n\n' + warnings.formatMessages(), + `Linted your app.\n\n${ formattedMessages }`, { arrow: true }); + } else { + runLog.log('Linted your app. No linting errors.', + { arrow: true }); } } + maybePrintLintWarnings(bundleResult); // Start watching for changes for files if requested. There's no // hurry to do this, since clientWatchSet contains a snapshot of the @@ -771,21 +777,7 @@ _.extend(AppRunner.prototype, { return bundleResultOrRunResult.runResult; bundleResult = bundleResultOrRunResult.bundleResult; - const canAndShouldLint = - self.projectContext.lintAppAndLocalPackages && bundleResult.warnings; - - if (canAndShouldLint) { - runLog.logTemporary('Linting your app.'); - - const formattedMessages = bundleResult.warnings.formatMessages(); - if (formattedMessages) { - runLog.log( - `Linted your app.\n\n${ formattedMessages }`, - { arrow: true }); - } else { - runLog.log('Linted your app. No linting errors.'); - } - } + maybePrintLintWarnings(bundleResult); var oldFuture = self.runFuture = new Future; diff --git a/tools/tests/compiler-plugins.js b/tools/tests/compiler-plugins.js index c57ba77d5d..ef2fe95aec 100644 --- a/tools/tests/compiler-plugins.js +++ b/tools/tests/compiler-plugins.js @@ -250,7 +250,8 @@ selftest.define("compiler plugins - compiler throws", () => { const run = s.run('add', 'local-plugin'); run.matchErr('Errors while adding packages'); - run.matchErr('While building package local-plugin'); + run.matchErr( + 'While running registerCompiler callback in package local-plugin'); // XXX This is wrong! The path on disk is packages/local-plugin/plugin.js, but // at some point we switched to the servePath which is based on the *plugin*'s // "package" name. diff --git a/tools/tests/linter-plugins.js b/tools/tests/linter-plugins.js index 3e7cbc3b19..0dd839b4fa 100644 --- a/tools/tests/linter-plugins.js +++ b/tools/tests/linter-plugins.js @@ -16,7 +16,7 @@ function startRun(sandbox, ...args) { }; function matchLintingMessages(run, messages, initial) { - run.match('Linting your app.'); + run.match('Linted your app.'); messages.forEach(message => run.match(message)); if (initial) { run.match('Started your app.');