Skip to content

Commit

Permalink
fix(ngcc): do not crash on bad entry-point
Browse files Browse the repository at this point in the history
Previously, when an entry-point contained code that caused its compilation
to fail, ngcc would exit in the middle of processing, possibly leaving other
entry-points in a corrupt state.

This change adds a new `errorOnFailedEntryPoint` option to `mainNgcc` which
can be set to `false` to allow processing of all entry-points that can be
processed to continue.

The default of `errorOnFailedEntryPoint` is set to true for programmatic
triggering of ngcc, such as the sync integration with the CLI, since in that
case it is targeting an entry-point that will actually be used in the current
project so we do want ngcc to exit with an error at that point.

The default of the command line ngcc tool will default to not error and
continue processing as much as possible, since in post-install hooks and
async CLI integration we do not have as much control over which entry-points
should be processed.
  • Loading branch information
petebacondarwin committed Mar 16, 2020
1 parent 400b0bc commit 5abfd11
Show file tree
Hide file tree
Showing 10 changed files with 186 additions and 22 deletions.
11 changes: 9 additions & 2 deletions packages/compiler-cli/ngcc/main-ngcc.ts
Expand Up @@ -83,6 +83,13 @@ if (require.main === module) {
type: 'boolean',
default: false,
})
.option('error-on-failed-entry-point', {
describe:
'If this is set then ngcc will exit and stop processing if an entry-point fails compilation.\n' +
'Otherwise ngcc will continue to attempt to process the rest of the entry-points.',
type: 'boolean',
default: false
})
.strict()
.help()
.parse(args);
Expand All @@ -103,7 +110,7 @@ if (require.main === module) {
const logLevel = options['l'] as keyof typeof LogLevel | undefined;
const enableI18nLegacyMessageIdFormat = options['legacy-message-ids'];
const invalidateEntryPointManifest = options['invalidate-entry-point-manifest'];

const errorOnFailedEntryPoint = options['error-on-failed-entry-point'];
(async() => {
try {
const logger = logLevel && new ConsoleLogger(LogLevel[logLevel]);
Expand All @@ -116,7 +123,7 @@ if (require.main === module) {
createNewEntryPointFormats,
logger,
enableI18nLegacyMessageIdFormat,
async: options['async'], invalidateEntryPointManifest,
async: options['async'], invalidateEntryPointManifest, errorOnFailedEntryPoint,
});

if (logger) {
Expand Down
7 changes: 7 additions & 0 deletions packages/compiler-cli/ngcc/src/execution/api.ts
Expand Up @@ -96,6 +96,13 @@ export interface TaskQueue {
/** Whether all tasks have been completed. */
allTasksCompleted: boolean;

/**
* Mark a task as failed.
*
* Do not process the tasks that depend upon the given task.
*/
markAsFailed(task: Task): void;

/**
* Get the next task whose processing can start (if any).
*
Expand Down
Expand Up @@ -5,7 +5,7 @@
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

import {Logger} from '../../logging/logger';
import {PartiallyOrderedTasks, Task, TaskQueue} from '../api';
import {stringifyTask} from '../utils';

Expand All @@ -19,10 +19,40 @@ export abstract class BaseTaskQueue implements TaskQueue {
}
protected inProgressTasks = new Set<Task>();

/**
* A map of tasks that should be skipped, mapped to the task that caused them to be skipped.
*/
private tasksToSkip = new Map<Task, Task>();

constructor(
protected tasks: PartiallyOrderedTasks, protected dependentTasks: Map<Task, Set<Task>>) {}
protected logger: Logger, protected tasks: PartiallyOrderedTasks,
protected dependentTasks: Map<Task, Set<Task>>) {}

protected abstract getNextTaskWorker(): Task|null;

getNextTask(): Task|null {
let nextTask = this.getNextTaskWorker();
while (nextTask !== null) {
if (!this.tasksToSkip.has(nextTask)) {
break;
}
// We are skipping this task so mark it as complete
this.markTaskCompleted(nextTask);
const failedTask = this.tasksToSkip.get(nextTask) !;
this.logger.warn(
`Skipping processing of ${nextTask.entryPoint.name} because its dependency ${failedTask.entryPoint.name} failed to compile.`);
nextTask = this.getNextTaskWorker();
}
return nextTask;
}

abstract getNextTask(): Task|null;
markAsFailed(task: Task) {
if (this.dependentTasks.has(task)) {
for (const dependentTask of this.dependentTasks.get(task) !) {
this.tasksToSkip.set(dependentTask, task);
}
}
}

markTaskCompleted(task: Task): void {
if (!this.inProgressTasks.has(task)) {
Expand Down
Expand Up @@ -5,6 +5,7 @@
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {Logger} from '../../logging/logger';
import {PartiallyOrderedTasks, Task} from '../api';
import {TaskDependencies, stringifyTask} from '../utils';
import {BaseTaskQueue} from './base_task_queue';
Expand All @@ -15,12 +16,13 @@ import {BaseTaskQueue} from './base_task_queue';
*/
export class ParallelTaskQueue extends BaseTaskQueue {
private blockedTasks: Map<Task, Set<Task>>;
constructor(tasks: PartiallyOrderedTasks, {dependencies, dependents}: TaskDependencies) {
super(sortTasksByPriority(tasks, dependencies), dependents);
constructor(
logger: Logger, tasks: PartiallyOrderedTasks, {dependencies, dependents}: TaskDependencies) {
super(logger, sortTasksByPriority(tasks, dependencies), dependents);
this.blockedTasks = dependencies;
}

getNextTask(): Task|null {
getNextTaskWorker(): Task|null {
// Look for the first available (i.e. not blocked) task.
// (NOTE: Since tasks are sorted by priority, the first available one is the best choice.)
const nextTaskIdx = this.tasks.findIndex(task => !this.blockedTasks.has(task));
Expand Down
Expand Up @@ -17,7 +17,7 @@ import {BaseTaskQueue} from './base_task_queue';
* before requesting the next one.
*/
export class SerialTaskQueue extends BaseTaskQueue {
getNextTask(): Task|null {
getNextTaskWorker(): Task|null {
const nextTask = this.tasks.shift() || null;

if (nextTask) {
Expand Down
17 changes: 16 additions & 1 deletion packages/compiler-cli/ngcc/src/execution/utils.ts
Expand Up @@ -7,10 +7,11 @@
*/
import {DepGraph} from 'dependency-graph';
import {FileSystem, resolve} from '../../../src/ngtsc/file_system';
import {Logger} from '../logging/logger';
import {markAsProcessed} from '../packages/build_marker';
import {EntryPoint, PackageJsonFormatProperties, getEntryPointFormat} from '../packages/entry_point';
import {PackageJsonUpdater} from '../writing/package_json_updater';
import {PartiallyOrderedTasks, Task, TaskCompletedCallback, TaskProcessingOutcome} from './api';
import {PartiallyOrderedTasks, Task, TaskCompletedCallback, TaskProcessingOutcome, TaskQueue} from './api';

/**
* Creates a task completed callback that calls one of those in a hash of callback based on the
Expand Down Expand Up @@ -63,6 +64,20 @@ export function createThrowErrorCallback(fs: FileSystem): TaskCompletedCallback
};
}

/**
* Create a callback that can be used to log an error and skip dependent tasks when a task fails.
*/
export function createLogErrorCallback(
logger: Logger, fs: FileSystem, taskQueue: TaskQueue): TaskCompletedCallback {
return (task: Task, _outcome: TaskProcessingOutcome, message: string | null): void => {
taskQueue.markAsFailed(task);
const format = getEntryPointFormat(fs, task.entryPoint, task.formatProperty);
logger.error(
`Failed to compile entry-point ${task.entryPoint.name} (${task.formatProperty} as ${format})` +
(message !== null ? ` due to ${message}` : ''));
};
}

/** Stringify a task for debugging purposes. */
export const stringifyTask = (task: Task): string =>
`{entryPoint: ${task.entryPoint.name}, formatProperty: ${task.formatProperty}, processDts: ${task.processDts}}`;
Expand Down
35 changes: 25 additions & 10 deletions packages/compiler-cli/ngcc/src/main.ts
Expand Up @@ -30,7 +30,7 @@ import {ClusterPackageJsonUpdater} from './execution/cluster/package_json_update
import {SingleProcessExecutorAsync, SingleProcessExecutorSync} from './execution/single_process_executor';
import {ParallelTaskQueue} from './execution/task_selection/parallel_task_queue';
import {SerialTaskQueue} from './execution/task_selection/serial_task_queue';
import {composeTaskCompletedCallbacks, computeTaskDependencies, createMarkAsProcessedCallback, createThrowErrorCallback} from './execution/utils';
import {composeTaskCompletedCallbacks, computeTaskDependencies, createLogErrorCallback, createMarkAsProcessedCallback, createThrowErrorCallback} from './execution/utils';
import {AsyncLocker} from './locking/async_locker';
import {LockFileWithChildProcess} from './locking/lock_file_with_child_process';
import {SyncLocker} from './locking/sync_locker';
Expand Down Expand Up @@ -106,6 +106,15 @@ export interface SyncNgccOptions {
*/
async?: false;

/**
* Whether processing should terminate with an error code if an entry-point fails to be processed.
*
* Defaults to true. Set to false if you want ngcc to continue to process entry-points after a
* failure. In this case it will clean the whole package that errored and resume processing other
* entry-points.
*/
errorOnFailedEntryPoint?: boolean;

/**
* Render `$localize` messages with legacy format ids.
*
Expand Down Expand Up @@ -153,7 +162,7 @@ export function mainNgcc({basePath, targetEntryPointPath,
propertiesToConsider = SUPPORTED_FORMAT_PROPERTIES,
compileAllFormats = true, createNewEntryPointFormats = false,
logger = new ConsoleLogger(LogLevel.info), pathMappings, async = false,
enableI18nLegacyMessageIdFormat = true,
errorOnFailedEntryPoint = true, enableI18nLegacyMessageIdFormat = true,
invalidateEntryPointManifest = false}: NgccOptions): void|Promise<void> {
// Execute in parallel, if async execution is acceptable and there are more than 1 CPU cores.
const inParallel = async && (os.cpus().length > 1);
Expand Down Expand Up @@ -251,7 +260,7 @@ export function mainNgcc({basePath, targetEntryPointPath,
`Analyzed ${entryPoints.length} entry-points in ${duration}s. ` +
`(Total tasks: ${tasks.length})`);

return getTaskQueue(inParallel, tasks, graph);
return getTaskQueue(logger, inParallel, tasks, graph);
};

// The function for creating the `compile()` function.
Expand Down Expand Up @@ -296,6 +305,7 @@ export function mainNgcc({basePath, targetEntryPointPath,
const errors = replaceTsWithNgInErrors(
ts.formatDiagnosticsWithColorAndContext(result.diagnostics, bundle.src.host));
onTaskCompleted(task, TaskProcessingOutcome.Failed, `compilation errors:\n${errors}`);
return;
}

logger.debug(` Successfully compiled ${entryPoint.name} : ${formatProperty}`);
Expand All @@ -305,7 +315,8 @@ export function mainNgcc({basePath, targetEntryPointPath,
};

// The executor for actually planning and getting the work done.
const createTaskCompletedCallback = getCreateTaskCompletedCallback(pkgJsonUpdater, fileSystem);
const createTaskCompletedCallback =
getCreateTaskCompletedCallback(pkgJsonUpdater, errorOnFailedEntryPoint, logger, fileSystem);
const executor = getExecutor(
async, inParallel, logger, pkgJsonUpdater, fileSystem, createTaskCompletedCallback);

Expand Down Expand Up @@ -347,17 +358,21 @@ function getFileWriter(
}

function getTaskQueue(
inParallel: boolean, tasks: PartiallyOrderedTasks, graph: DepGraph<EntryPoint>): TaskQueue {
logger: Logger, inParallel: boolean, tasks: PartiallyOrderedTasks,
graph: DepGraph<EntryPoint>): TaskQueue {
const dependencies = computeTaskDependencies(tasks, graph);
return inParallel ? new ParallelTaskQueue(tasks, dependencies) :
new SerialTaskQueue(tasks, dependencies.dependents);
return inParallel ? new ParallelTaskQueue(logger, tasks, dependencies) :
new SerialTaskQueue(logger, tasks, dependencies.dependents);
}

function getCreateTaskCompletedCallback(
pkgJsonUpdater: PackageJsonUpdater, fileSystem: FileSystem): CreateTaskCompletedCallback {
return _taskQueue => composeTaskCompletedCallbacks({
pkgJsonUpdater: PackageJsonUpdater, errorOnFailedEntryPoint: boolean, logger: Logger,
fileSystem: FileSystem): CreateTaskCompletedCallback {
return taskQueue => composeTaskCompletedCallbacks({
[TaskProcessingOutcome.Processed]: createMarkAsProcessedCallback(pkgJsonUpdater),
[TaskProcessingOutcome.Failed]: createThrowErrorCallback(fileSystem),
[TaskProcessingOutcome.Failed]:
errorOnFailedEntryPoint ? createThrowErrorCallback(fileSystem) :
createLogErrorCallback(logger, fileSystem, taskQueue),
});
}

Expand Down
Expand Up @@ -8,6 +8,7 @@
import {PartiallyOrderedTasks, TaskQueue} from '../../../src/execution/api';
import {ParallelTaskQueue} from '../../../src/execution/task_selection/parallel_task_queue';
import {computeTaskDependencies} from '../../../src/execution/utils';
import {MockLogger} from '../../helpers/mock_logger';
import {createTasksAndGraph} from '../helpers';

describe('ParallelTaskQueue', () => {
Expand Down Expand Up @@ -35,7 +36,7 @@ describe('ParallelTaskQueue', () => {
const {tasks, graph} =
createTasksAndGraph(entryPointCount, tasksPerEntryPointCount, entryPointDeps);
const dependencies = computeTaskDependencies(tasks, graph);
return {tasks, queue: new ParallelTaskQueue(tasks.slice(), dependencies)};
return {tasks, queue: new ParallelTaskQueue(new MockLogger(), tasks.slice(), dependencies)};
}

/**
Expand Down
Expand Up @@ -10,6 +10,7 @@ import {DepGraph} from 'dependency-graph';
import {PartiallyOrderedTasks, Task, TaskQueue} from '../../../src/execution/api';
import {SerialTaskQueue} from '../../../src/execution/task_selection/serial_task_queue';
import {EntryPoint} from '../../../src/packages/entry_point';
import {MockLogger} from '../../helpers/mock_logger';

describe('SerialTaskQueue', () => {
// Helpers
Expand All @@ -36,7 +37,10 @@ describe('SerialTaskQueue', () => {
graph.addNode(entryPoint.path);
}
const dependencies = computeTaskDependencies(tasks, graph);
return {tasks, queue: new SerialTaskQueue(tasks.slice(), dependencies.dependents)};
return {
tasks,
queue: new SerialTaskQueue(new MockLogger(), tasks.slice(), dependencies.dependents)
};
};

/**
Expand Down
83 changes: 83 additions & 0 deletions packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts
Expand Up @@ -1060,6 +1060,89 @@ runInEachFileSystem(() => {
expect(e.message).toContain('component is missing a template');
}
});

it('should not fail but log an error with formatted diagnostics when an error diagnostic is produced (and errorOnFailedEntryPoint is false)',
() => {
loadTestFiles([
{
name: _('/node_modules/fatal-error/package.json'),
contents:
'{"name": "fatal-error", "es2015": "./index.js", "typings": "./index.d.ts"}',
},
{name: _('/node_modules/fatal-error/index.metadata.json'), contents: 'DUMMY DATA'},
{
name: _('/node_modules/fatal-error/index.js'),
contents: `
import {Component} from '@angular/core';
export class FatalError {}
FatalError.decorators = [
{type: Component, args: [{selector: 'fatal-error'}]}
];`,
},
{
name: _('/node_modules/fatal-error/index.d.ts'),
contents: `export declare class FatalError {}`,
},
{
name: _('/node_modules/dependent/package.json'),
contents: '{"name": "dependent", "es2015": "./index.js", "typings": "./index.d.ts"}',
},
{name: _('/node_modules/dependent/index.metadata.json'), contents: 'DUMMY DATA'},
{
name: _('/node_modules/dependent/index.js'),
contents: `
import {Component} from '@angular/core';
import {FatalError} from 'fatal-error';
export class Dependent {}
Dependent.decorators = [
{type: Component, args: [{selector: 'dependent', template: ''}]}
];`,
},
{
name: _('/node_modules/dependent/index.d.ts'),
contents: `export declare class Dependent {}`,
},
{
name: _('/node_modules/independent/package.json'),
contents:
'{"name": "independent", "es2015": "./index.js", "typings": "./index.d.ts"}',
},
{name: _('/node_modules/independent/index.metadata.json'), contents: 'DUMMY DATA'},
{
name: _('/node_modules/independent/index.js'),
contents: `
import {Component} from '@angular/core';
export class Independent {}
Independent.decorators = [
{type: Component, args: [{selector: 'independent', template: ''}]}
];`,
},
{
name: _('/node_modules/independent/index.d.ts'),
contents: `export declare class Independent {}`,
},
]);

const logger = new MockLogger();
mainNgcc({
basePath: '/node_modules',
propertiesToConsider: ['es2015'],
errorOnFailedEntryPoint: false, logger,
});
expect(logger.logs.error.length).toEqual(1);
const message = logger.logs.error[0][0];
expect(message).toContain(
'Failed to compile entry-point fatal-error (es2015 as esm2015) due to compilation errors:');
expect(message).toContain('NG2001');
expect(message).toContain('component is missing a template');

expect(hasBeenProcessed(loadPackage('fatal-error', _('/node_modules')), 'es2015'))
.toBe(false);
expect(hasBeenProcessed(loadPackage('dependent', _('/node_modules')), 'es2015'))
.toBe(false);
expect(hasBeenProcessed(loadPackage('independent', _('/node_modules')), 'es2015'))
.toBe(true);
});
});

describe('logger', () => {
Expand Down

0 comments on commit 5abfd11

Please sign in to comment.