github/meteor
1
1
Fork 0
mirror of https://github.com/meteor/meteor.git synced 2026-05-02 03:01:46 -04:00
Code Issues Packages Projects Releases Wiki Activity

clean up error handling around linters

Browse Source 
There were a few problems here:

- compiler.lint actually did things that could seriously fail (eg loading
  plugins) but used its buildmessage context to return *linter
  warnings*. So actual errors got lumped in with warnings and didn't
  prevent builds.  Fixed this by changing compiler.lint to return linter
  warnings as a return value and use its implicit buildmessage context
  only for build errors

- We weren't checking for errors after compiler.getMinifiers even though
  that loaded plugins and could fail

- We were using _.isEmpty(app.sourceProcessors.linter) to decide in
  lintBundle if we should say "no linter warnings" or "no linters were
  run", but this is a SourceProcessorSet now, not a dictionary, so we
  should have used the isEmpty method instead (so we were getting
  unnecessary "No linting errors" messages when there were no linters)

- compiler.compile still tried to run getSourcesFunc even if
  initializing the SourceProcessorSets failed

- Printing linter warnings in the runner looked different depending on
  whether it was right after doing a client refresh or not

- We were doing a temporary log of "Linting your app" and immediately
  logging "Linted your app". The point of temporary logs is to display
  while long processes run, but since linting is integrated, this didn't
  really make sense.  (Really we need to better integrate the progress
  bar and runlog, since progress fulfills most of the needs formerly
  done by the runlog.)
This commit is contained in:
David Glasser
2015-07-14 23:12:21 -07:00
parent 1a71752f8b
commit 95b4b8f0b0
9 changed files with 138 additions and 80 deletions
Download Patch File Download Diff File Expand all files Collapse all files

34
tools/build-plugin.js
View File

@@ -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;

33
tools/bundler.js
View File

@@ -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;
}

5
tools/compiler-plugin.js
View File

@@ -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,

86
tools/compiler.js
View File

@@ -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;
}

15
tools/isopack-cache.js
View File

@@ -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;
}
},

6
tools/isopack.js
View File

@@ -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);

34
tools/run-app.js
View File

@@ -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;

3
tools/tests/compiler-plugins.js
View File

@@ -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.

2
tools/tests/linter-plugins.js
View File

@@ -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.');