From c9f554cda7f9d22cb64e2b2b0e423d18469ddd39 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Tue, 17 Mar 2020 12:32:34 +0000 Subject: [PATCH] fix(ngcc): do not crash on overlapping entry-points (#36083) When two entry-points overlap, ngcc may attempt to process some files twice. Previously, when this occured ngcc would just exit with an error preventing any other entry-points from being processed. This commit changes ngcc so that if `errorOnFailedEntryPoint` is false, it will simply log an error and continue to process entry-points. This is useful when ngcc is processing the entire node_modules folder and there are some invalid entry-points that the project doesn't actually use. PR Close #36083 --- packages/compiler-cli/ngcc/src/main.ts | 12 +++-- .../ngcc/src/writing/in_place_file_writer.ts | 26 +++++++--- .../writing/new_entry_point_file_writer.ts | 7 ++- .../test/writing/in_place_file_writer_spec.ts | 49 ++++++++++++++----- .../new_entry_point_file_writer_spec.ts | 19 ++++--- 5 files changed, 79 insertions(+), 34 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/main.ts b/packages/compiler-cli/ngcc/src/main.ts index d332866d54..c22839a539 100644 --- a/packages/compiler-cli/ngcc/src/main.ts +++ b/packages/compiler-cli/ngcc/src/main.ts @@ -277,7 +277,8 @@ export function mainNgcc({basePath, targetEntryPointPath, // The function for creating the `compile()` function. const createCompileFn: CreateCompileFn = onTaskCompleted => { - const fileWriter = getFileWriter(fileSystem, pkgJsonUpdater, createNewEntryPointFormats); + const fileWriter = getFileWriter( + fileSystem, logger, pkgJsonUpdater, createNewEntryPointFormats, errorOnFailedEntryPoint); const transformer = new Transformer(fileSystem, logger); return (task: Task) => { @@ -362,10 +363,11 @@ function getPackageJsonUpdater(inParallel: boolean, fs: FileSystem): PackageJson } function getFileWriter( - fs: FileSystem, pkgJsonUpdater: PackageJsonUpdater, - createNewEntryPointFormats: boolean): FileWriter { - return createNewEntryPointFormats ? new NewEntryPointFileWriter(fs, pkgJsonUpdater) : - new InPlaceFileWriter(fs); + fs: FileSystem, logger: Logger, pkgJsonUpdater: PackageJsonUpdater, + createNewEntryPointFormats: boolean, errorOnFailedEntryPoint: boolean): FileWriter { + return createNewEntryPointFormats ? + new NewEntryPointFileWriter(fs, logger, errorOnFailedEntryPoint, pkgJsonUpdater) : + new InPlaceFileWriter(fs, logger, errorOnFailedEntryPoint); } function getTaskQueue( diff --git a/packages/compiler-cli/ngcc/src/writing/in_place_file_writer.ts b/packages/compiler-cli/ngcc/src/writing/in_place_file_writer.ts index 8619b49c57..ab390f2c6f 100644 --- a/packages/compiler-cli/ngcc/src/writing/in_place_file_writer.ts +++ b/packages/compiler-cli/ngcc/src/writing/in_place_file_writer.ts @@ -1,4 +1,3 @@ - /** * @license * Copyright Google Inc. All Rights Reserved. @@ -7,6 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ import {FileSystem, absoluteFrom, dirname} from '../../../src/ngtsc/file_system'; +import {Logger} from '../logging/logger'; import {EntryPointJsonProperty} from '../packages/entry_point'; import {EntryPointBundle} from '../packages/entry_point_bundle'; import {FileToWrite} from '../rendering/utils'; @@ -18,7 +18,9 @@ export const NGCC_BACKUP_EXTENSION = '.__ivy_ngcc_bak'; * a back-up of the original file with an extra `.__ivy_ngcc_bak` extension. */ export class InPlaceFileWriter implements FileWriter { - constructor(protected fs: FileSystem) {} + constructor( + protected fs: FileSystem, protected logger: Logger, + protected errorOnFailedEntryPoint: boolean) {} writeBundle( _bundle: EntryPointBundle, transformedFiles: FileToWrite[], @@ -30,12 +32,20 @@ export class InPlaceFileWriter implements FileWriter { this.fs.ensureDir(dirname(file.path)); const backPath = absoluteFrom(`${file.path}${NGCC_BACKUP_EXTENSION}`); if (this.fs.exists(backPath)) { - throw new Error( - `Tried to overwrite ${backPath} with an ngcc back up file, which is disallowed.`); + if (this.errorOnFailedEntryPoint) { + throw new Error( + `Tried to overwrite ${backPath} with an ngcc back up file, which is disallowed.`); + } else { + this.logger.error( + `Tried to write ${backPath} with an ngcc back up file but it already exists so not writing, nor backing up, ${file.path}.\n` + + `This error may be because two or more entry-points overlap and ngcc has been asked to process some files more than once.\n` + + `You should check other entry-points in this package and set up a config to ignore any that you are not using.`); + } + } else { + if (this.fs.exists(file.path)) { + this.fs.moveFile(file.path, backPath); + } + this.fs.writeFile(file.path, file.contents); } - if (this.fs.exists(file.path)) { - this.fs.moveFile(file.path, backPath); - } - this.fs.writeFile(file.path, file.contents); } } diff --git a/packages/compiler-cli/ngcc/src/writing/new_entry_point_file_writer.ts b/packages/compiler-cli/ngcc/src/writing/new_entry_point_file_writer.ts index 2c9b0570c8..0d78265f3a 100644 --- a/packages/compiler-cli/ngcc/src/writing/new_entry_point_file_writer.ts +++ b/packages/compiler-cli/ngcc/src/writing/new_entry_point_file_writer.ts @@ -8,6 +8,7 @@ */ import {AbsoluteFsPath, FileSystem, absoluteFromSourceFile, dirname, join, relative} from '../../../src/ngtsc/file_system'; import {isDtsPath} from '../../../src/ngtsc/util/src/typescript'; +import {Logger} from '../logging/logger'; import {EntryPoint, EntryPointJsonProperty} from '../packages/entry_point'; import {EntryPointBundle} from '../packages/entry_point_bundle'; import {FileToWrite} from '../rendering/utils'; @@ -27,7 +28,11 @@ export const NGCC_PROPERTY_EXTENSION = '_ivy_ngcc'; * `InPlaceFileWriter`). */ export class NewEntryPointFileWriter extends InPlaceFileWriter { - constructor(fs: FileSystem, private pkgJsonUpdater: PackageJsonUpdater) { super(fs); } + constructor( + fs: FileSystem, logger: Logger, errorOnFailedEntryPoint: boolean, + private pkgJsonUpdater: PackageJsonUpdater) { + super(fs, logger, errorOnFailedEntryPoint); + } writeBundle( bundle: EntryPointBundle, transformedFiles: FileToWrite[], diff --git a/packages/compiler-cli/ngcc/test/writing/in_place_file_writer_spec.ts b/packages/compiler-cli/ngcc/test/writing/in_place_file_writer_spec.ts index 646f2f9bb1..95eeae821f 100644 --- a/packages/compiler-cli/ngcc/test/writing/in_place_file_writer_spec.ts +++ b/packages/compiler-cli/ngcc/test/writing/in_place_file_writer_spec.ts @@ -10,6 +10,7 @@ import {runInEachFileSystem} from '../../../src/ngtsc/file_system/testing'; import {loadTestFiles} from '../../../test/helpers'; import {EntryPointBundle} from '../../src/packages/entry_point_bundle'; import {InPlaceFileWriter} from '../../src/writing/in_place_file_writer'; +import {MockLogger} from '../helpers/mock_logger'; runInEachFileSystem(() => { describe('InPlaceFileWriter', () => { @@ -24,13 +25,15 @@ runInEachFileSystem(() => { {name: _('/package/path/folder-1/file-2.js'), contents: 'ORIGINAL FILE 2'}, {name: _('/package/path/folder-2/file-3.js'), contents: 'ORIGINAL FILE 3'}, {name: _('/package/path/folder-2/file-4.js'), contents: 'ORIGINAL FILE 4'}, + {name: _('/package/path/already-backed-up.js'), contents: 'ORIGINAL ALREADY BACKED UP'}, {name: _('/package/path/already-backed-up.js.__ivy_ngcc_bak'), contents: 'BACKED UP'}, ]); }); it('should write all the FileInfo to the disk', () => { const fs = getFileSystem(); - const fileWriter = new InPlaceFileWriter(fs); + const logger = new MockLogger(); + const fileWriter = new InPlaceFileWriter(fs, logger, /* errorOnFailedEntryPoint */ true); fileWriter.writeBundle({} as EntryPointBundle, [ {path: _('/package/path/top-level.js'), contents: 'MODIFIED TOP LEVEL'}, {path: _('/package/path/folder-1/file-1.js'), contents: 'MODIFIED FILE 1'}, @@ -47,7 +50,8 @@ runInEachFileSystem(() => { it('should create backups of all files that previously existed', () => { const fs = getFileSystem(); - const fileWriter = new InPlaceFileWriter(fs); + const logger = new MockLogger(); + const fileWriter = new InPlaceFileWriter(fs, logger, /* errorOnFailedEntryPoint */ true); fileWriter.writeBundle({} as EntryPointBundle, [ {path: _('/package/path/top-level.js'), contents: 'MODIFIED TOP LEVEL'}, {path: _('/package/path/folder-1/file-1.js'), contents: 'MODIFIED FILE 1'}, @@ -65,15 +69,36 @@ runInEachFileSystem(() => { expect(fs.exists(_('/package/path/folder-3/file-5.js.__ivy_ngcc_bak'))).toBe(false); }); - it('should error if the backup file already exists', () => { - const fs = getFileSystem(); - const fileWriter = new InPlaceFileWriter(fs); - const absoluteBackupPath = _('/package/path/already-backed-up.js'); - expect( - () => fileWriter.writeBundle( - {} as EntryPointBundle, [{path: absoluteBackupPath, contents: 'MODIFIED BACKED UP'}])) - .toThrowError( - `Tried to overwrite ${absoluteBackupPath}.__ivy_ngcc_bak with an ngcc back up file, which is disallowed.`); - }); + it('should throw an error if the backup file already exists and errorOnFailedEntryPoint is true', + () => { + const fs = getFileSystem(); + const logger = new MockLogger(); + const fileWriter = new InPlaceFileWriter(fs, logger, /* errorOnFailedEntryPoint */ true); + const absoluteBackupPath = _('/package/path/already-backed-up.js'); + expect( + () => fileWriter.writeBundle( + {} as EntryPointBundle, + [{path: absoluteBackupPath, contents: 'MODIFIED BACKED UP'}])) + .toThrowError( + `Tried to overwrite ${absoluteBackupPath}.__ivy_ngcc_bak with an ngcc back up file, which is disallowed.`); + }); + + it('should log an error, and skip writing the file, if the backup file already exists and errorOnFailedEntryPoint is false', + () => { + const fs = getFileSystem(); + const logger = new MockLogger(); + const fileWriter = new InPlaceFileWriter(fs, logger, /* errorOnFailedEntryPoint */ false); + const absoluteBackupPath = _('/package/path/already-backed-up.js'); + fileWriter.writeBundle( + {} as EntryPointBundle, [{path: absoluteBackupPath, contents: 'MODIFIED BACKED UP'}]); + // Should not have written the new file nor overwritten the backup file. + expect(fs.readFile(absoluteBackupPath)).toEqual('ORIGINAL ALREADY BACKED UP'); + expect(fs.readFile(_(absoluteBackupPath + '.__ivy_ngcc_bak'))).toEqual('BACKED UP'); + expect(logger.logs.error).toEqual([[ + `Tried to write ${absoluteBackupPath}.__ivy_ngcc_bak with an ngcc back up file but it already exists so not writing, nor backing up, ${absoluteBackupPath}.\n` + + `This error may be because two or more entry-points overlap and ngcc has been asked to process some files more than once.\n` + + `You should check other entry-points in this package and set up a config to ignore any that you are not using.` + ]]); + }); }); }); diff --git a/packages/compiler-cli/ngcc/test/writing/new_entry_point_file_writer_spec.ts b/packages/compiler-cli/ngcc/test/writing/new_entry_point_file_writer_spec.ts index 89a0e22d7d..f8e1e77d42 100644 --- a/packages/compiler-cli/ngcc/test/writing/new_entry_point_file_writer_spec.ts +++ b/packages/compiler-cli/ngcc/test/writing/new_entry_point_file_writer_spec.ts @@ -23,12 +23,15 @@ runInEachFileSystem(() => { let _: typeof absoluteFrom; let fs: FileSystem; let fileWriter: FileWriter; + let logger: MockLogger; let entryPoint: EntryPoint; let esm5bundle: EntryPointBundle; let esm2015bundle: EntryPointBundle; beforeEach(() => { _ = absoluteFrom; + fs = getFileSystem(); + logger = new MockLogger(); loadTestFiles([ { @@ -100,11 +103,11 @@ runInEachFileSystem(() => { describe('writeBundle() [primary entry-point]', () => { beforeEach(() => { - fs = getFileSystem(); - fileWriter = new NewEntryPointFileWriter(fs, new DirectPackageJsonUpdater(fs)); + fileWriter = new NewEntryPointFileWriter( + fs, logger, /* errorOnFailedEntryPoint */ true, new DirectPackageJsonUpdater(fs)); const config = new NgccConfiguration(fs, _('/')); const result = getEntryPointInfo( - fs, config, new MockLogger(), _('/node_modules/test'), _('/node_modules/test')) !; + fs, config, logger, _('/node_modules/test'), _('/node_modules/test')) !; if (result === NO_ENTRY_POINT || result === INVALID_ENTRY_POINT) { return fail(`Expected an entry point but got ${result}`); } @@ -240,11 +243,11 @@ runInEachFileSystem(() => { describe('writeBundle() [secondary entry-point]', () => { beforeEach(() => { - fs = getFileSystem(); - fileWriter = new NewEntryPointFileWriter(fs, new DirectPackageJsonUpdater(fs)); + fileWriter = new NewEntryPointFileWriter( + fs, logger, /* errorOnFailedEntryPoint */ true, new DirectPackageJsonUpdater(fs)); const config = new NgccConfiguration(fs, _('/')); const result = getEntryPointInfo( - fs, config, new MockLogger(), _('/node_modules/test'), _('/node_modules/test/a')) !; + fs, config, logger, _('/node_modules/test'), _('/node_modules/test/a')) !; if (result === NO_ENTRY_POINT || result === INVALID_ENTRY_POINT) { return fail(`Expected an entry point but got ${result}`); } @@ -369,8 +372,8 @@ runInEachFileSystem(() => { describe('writeBundle() [entry-point (with files placed outside entry-point folder)]', () => { beforeEach(() => { - fs = getFileSystem(); - fileWriter = new NewEntryPointFileWriter(fs, new DirectPackageJsonUpdater(fs)); + fileWriter = new NewEntryPointFileWriter( + fs, logger, /* errorOnFailedEntryPoint */ true, new DirectPackageJsonUpdater(fs)); const config = new NgccConfiguration(fs, _('/')); const result = getEntryPointInfo( fs, config, new MockLogger(), _('/node_modules/test'), _('/node_modules/test/b')) !;