fix(compiler-cli): ensure file_system handles mixed Windows drives (#37959)

The `fs.relative()` method assumed that the file-system is a single tree,
which is not the case in Windows, where you can have multiple drives,
e.g. `C:`, `D:` etc.

This commit changes `fs.relative()` so that it no longer forces the result
to be a `PathSegment` and then flows that refactoring through the rest of
the compiler-cli (and ngcc).  The main difference is that now, in some cases,
we needed to check whether the result is "rooted", i.e an `AbsoluteFsPath`,
rather than a `PathSegment`, before using it.

Fixes #36777

PR Close #37959
This commit is contained in:
Pete Bacon Darwin 2020-07-07 15:40:14 +01:00 committed by Andrew Scott
parent 66947cf9fa
commit 6b311552f0
22 changed files with 103 additions and 77 deletions

View File

@ -5,12 +5,12 @@
* 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 {AbsoluteFsPath, relative} from '../../../src/ngtsc/file_system';
import {AbsoluteFsPath, isLocalRelativePath, relative} from '../../../src/ngtsc/file_system';
import {DependencyTracker} from '../../../src/ngtsc/incremental/api';
export function isWithinPackage(packagePath: AbsoluteFsPath, filePath: AbsoluteFsPath): boolean {
const relativePath = relative(packagePath, filePath);
return !relativePath.startsWith('..') && !relativePath.startsWith('node_modules/');
return isLocalRelativePath(relativePath) && !relativePath.startsWith('node_modules/');
}
class NoopDependencyTracker implements DependencyTracker {

View File

@ -228,11 +228,13 @@ export class TargetedEntryPointFinder extends TracingEntryPointFinder {
/**
* Split the given `path` into path segments using an FS independent algorithm.
*/
private splitPath(path: PathSegment) {
private splitPath(path: PathSegment|AbsoluteFsPath) {
const segments = [];
while (path !== '.') {
let container = this.fs.dirname(path);
while (path !== container) {
segments.unshift(this.fs.basename(path));
path = this.fs.dirname(path);
path = container;
container = this.fs.dirname(container);
}
return segments;
}

View File

@ -9,7 +9,7 @@ import {Statement} from '@angular/compiler';
import MagicString from 'magic-string';
import * as ts from 'typescript';
import {absoluteFromSourceFile, AbsoluteFsPath, dirname, relative} from '../../../src/ngtsc/file_system';
import {absoluteFromSourceFile, AbsoluteFsPath, dirname, relative, toRelativeImport} from '../../../src/ngtsc/file_system';
import {NOOP_DEFAULT_IMPORT_RECORDER, Reexport} from '../../../src/ngtsc/imports';
import {Import, ImportManager, translateStatement} from '../../../src/ngtsc/translator';
import {isDtsPath} from '../../../src/ngtsc/util/src/typescript';
@ -57,8 +57,9 @@ export class EsmRenderingFormatter implements RenderingFormatter {
if (from) {
const basePath = stripExtension(from);
const relativePath = './' + relative(dirname(entryPointBasePath), basePath);
exportFrom = entryPointBasePath !== basePath ? ` from '${relativePath}'` : '';
const relativePath = relative(dirname(entryPointBasePath), basePath);
const relativeImport = toRelativeImport(relativePath);
exportFrom = entryPointBasePath !== basePath ? ` from '${relativeImport}'` : '';
}
const exportStr = `\nexport {${e.identifier}}${exportFrom};`;
@ -197,10 +198,10 @@ export class EsmRenderingFormatter implements RenderingFormatter {
const ngModuleName = info.ngModule.node.name.text;
const declarationFile = absoluteFromSourceFile(info.declaration.getSourceFile());
const ngModuleFile = absoluteFromSourceFile(info.ngModule.node.getSourceFile());
const relativePath = relative(dirname(declarationFile), ngModuleFile);
const relativeImport = toRelativeImport(relativePath);
const importPath = info.ngModule.ownedByModuleGuess ||
(declarationFile !== ngModuleFile ?
stripExtension(`./${relative(dirname(declarationFile), ngModuleFile)}`) :
null);
(declarationFile !== ngModuleFile ? stripExtension(relativeImport) : null);
const ngModule = generateImportString(importManager, importPath, ngModuleName);
if (info.declaration.type) {

View File

@ -6,7 +6,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 {absoluteFromSourceFile, AbsoluteFsPath, dirname, FileSystem, join, relative} from '../../../src/ngtsc/file_system';
import {absoluteFromSourceFile, AbsoluteFsPath, dirname, FileSystem, isLocalRelativePath, join, relative, resolve} from '../../../src/ngtsc/file_system';
import {Logger} from '../../../src/ngtsc/logging';
import {isDtsPath} from '../../../src/ngtsc/util/src/typescript';
import {EntryPoint, EntryPointJsonProperty} from '../packages/entry_point';
@ -70,9 +70,9 @@ export class NewEntryPointFileWriter extends InPlaceFileWriter {
bundle: EntryPointBundle, packagePath: AbsoluteFsPath, ngccFolder: AbsoluteFsPath) {
bundle.src.program.getSourceFiles().forEach(sourceFile => {
const relativePath = relative(packagePath, absoluteFromSourceFile(sourceFile));
const isOutsidePackage = relativePath.startsWith('..');
if (!sourceFile.isDeclarationFile && !isOutsidePackage) {
const newFilePath = join(ngccFolder, relativePath);
const isInsidePackage = isLocalRelativePath(relativePath);
if (!sourceFile.isDeclarationFile && isInsidePackage) {
const newFilePath = resolve(ngccFolder, relativePath);
this.fs.ensureDir(dirname(newFilePath));
this.fs.copyFile(absoluteFromSourceFile(sourceFile), newFilePath);
}
@ -86,7 +86,7 @@ export class NewEntryPointFileWriter extends InPlaceFileWriter {
super.writeFileAndBackup(file);
} else {
const relativePath = relative(packagePath, file.path);
const newFilePath = join(ngccFolder, relativePath);
const newFilePath = resolve(ngccFolder, relativePath);
this.fs.ensureDir(dirname(newFilePath));
this.fs.writeFile(newFilePath, file.contents);
}
@ -98,7 +98,7 @@ export class NewEntryPointFileWriter extends InPlaceFileWriter {
super.revertFileAndBackup(filePath);
} else if (this.fs.exists(filePath)) {
const relativePath = relative(packagePath, filePath);
const newFilePath = join(packagePath, NGCC_DIRECTORY, relativePath);
const newFilePath = resolve(packagePath, NGCC_DIRECTORY, relativePath);
this.fs.removeFile(newFilePath);
}
}
@ -117,8 +117,9 @@ export class NewEntryPointFileWriter extends InPlaceFileWriter {
// All format properties point to the same format-path.
const oldFormatProp = formatProperties[0]!;
const oldFormatPath = packageJson[oldFormatProp]!;
const oldAbsFormatPath = join(entryPoint.path, oldFormatPath);
const newAbsFormatPath = join(ngccFolder, relative(entryPoint.packagePath, oldAbsFormatPath));
const oldAbsFormatPath = resolve(entryPoint.path, oldFormatPath);
const newAbsFormatPath =
resolve(ngccFolder, relative(entryPoint.packagePath, oldAbsFormatPath));
const newFormatPath = relative(entryPoint.path, newAbsFormatPath);
// Update all properties in `package.json` (both in memory and on disk).

View File

@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/
export {NgtscCompilerHost} from './src/compiler_host';
export {absoluteFrom, absoluteFromSourceFile, basename, dirname, getFileSystem, isRoot, isRooted, join, relative, relativeFrom, resolve, setFileSystem} from './src/helpers';
export {absoluteFrom, absoluteFromSourceFile, basename, dirname, getFileSystem, isLocalRelativePath, isRoot, isRooted, join, relative, relativeFrom, resolve, setFileSystem, toRelativeImport} from './src/helpers';
export {LogicalFileSystem, LogicalProjectPath} from './src/logical';
export {NodeJSFileSystem} from './src/node_js_file_system';
export {AbsoluteFsPath, FileStats, FileSystem, PathSegment, PathString} from './src/types';

View File

@ -83,7 +83,7 @@ export function isRooted(path: string): boolean {
/**
* Static access to `relative`.
*/
export function relative<T extends PathString>(from: T, to: T): PathSegment {
export function relative<T extends PathString>(from: T, to: T): PathSegment|AbsoluteFsPath {
return fs.relative(from, to);
}
@ -93,3 +93,23 @@ export function relative<T extends PathString>(from: T, to: T): PathSegment {
export function basename(filePath: PathString, extension?: string): PathSegment {
return fs.basename(filePath, extension) as PathSegment;
}
/**
* Returns true if the given path is locally relative.
*
* This is used to work out if the given path is relative (i.e. not absolute) but also is not
* escaping the current directory.
*/
export function isLocalRelativePath(relativePath: string): boolean {
return !isRooted(relativePath) && !relativePath.startsWith('..');
}
/**
* Converts a path to a form suitable for use as a relative module import specifier.
*
* In other words it adds the `./` to the path if it is locally relative.
*/
export function toRelativeImport(relativePath: PathSegment|AbsoluteFsPath): PathSegment|
AbsoluteFsPath {
return isLocalRelativePath(relativePath) ? `./${relativePath}` as PathSegment : relativePath;
}

View File

@ -82,7 +82,7 @@ export class InvalidFileSystem implements FileSystem {
isRooted(path: string): boolean {
throw makeError();
}
relative<T extends PathString>(from: T, to: T): PathSegment {
relative<T extends PathString>(from: T, to: T): PathSegment|AbsoluteFsPath {
throw makeError();
}
basename(filePath: string, extension?: string): PathSegment {

View File

@ -7,7 +7,7 @@
*/
import * as ts from 'typescript';
import {absoluteFrom, dirname, relative, resolve} from './helpers';
import {absoluteFrom, dirname, isLocalRelativePath, relative, resolve, toRelativeImport} from './helpers';
import {AbsoluteFsPath, BrandedPath, PathSegment} from './types';
import {stripExtension} from './util';
@ -29,11 +29,8 @@ export const LogicalProjectPath = {
* importing from `to`.
*/
relativePathBetween: function(from: LogicalProjectPath, to: LogicalProjectPath): PathSegment {
let relativePath = relative(dirname(resolve(from)), resolve(to));
if (!relativePath.startsWith('../')) {
relativePath = ('./' + relativePath) as PathSegment;
}
return relativePath as PathSegment;
const relativePath = relative(dirname(resolve(from)), resolve(to));
return toRelativeImport(relativePath) as PathSegment;
},
};
@ -122,5 +119,5 @@ export class LogicalFileSystem {
* E.g. `foo/bar/zee` is within `foo/bar` but not within `foo/car`.
*/
function isWithinBasePath(base: AbsoluteFsPath, path: AbsoluteFsPath): boolean {
return !relative(base, path).startsWith('..');
return isLocalRelativePath(relative(base, path));
}

View File

@ -9,7 +9,7 @@
import * as fs from 'fs';
import * as fsExtra from 'fs-extra';
import * as p from 'path';
import {absoluteFrom, relativeFrom} from './helpers';
import {absoluteFrom} from './helpers';
import {AbsoluteFsPath, FileStats, FileSystem, PathSegment, PathString} from './types';
/**
@ -93,8 +93,8 @@ export class NodeJSFileSystem implements FileSystem {
isRooted(path: string): boolean {
return p.isAbsolute(path);
}
relative<T extends PathString>(from: T, to: T): PathSegment {
return relativeFrom(this.normalize(p.relative(from, to)));
relative<T extends PathString>(from: T, to: T): PathSegment|AbsoluteFsPath {
return this.normalize(p.relative(from, to)) as PathSegment | AbsoluteFsPath;
}
basename(filePath: string, extension?: string): PathSegment {
return p.basename(filePath, extension) as PathSegment;

View File

@ -57,7 +57,14 @@ export interface FileSystem {
resolve(...paths: string[]): AbsoluteFsPath;
dirname<T extends PathString>(file: T): T;
join<T extends PathString>(basePath: T, ...paths: string[]): T;
relative<T extends PathString>(from: T, to: T): PathSegment;
/**
* Compute the relative path between `from` and `to`.
*
* In file-systems that can have multiple file trees the returned path may not actually be
* "relative" (i.e. `PathSegment`). For example, Windows can have multiple drives :
* `relative('c:/a/b', 'd:/a/c')` would be `d:/a/c'.
*/
relative<T extends PathString>(from: T, to: T): PathSegment|AbsoluteFsPath;
basename(filePath: string, extension?: string): PathSegment;
realpath(filePath: AbsoluteFsPath): AbsoluteFsPath;
getDefaultLibLocation(): AbsoluteFsPath;

View File

@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/
import * as ts from 'typescript';
import {AbsoluteFsPath} from './types';
import {AbsoluteFsPath, PathString} from './types';
const TS_DTS_JS_EXTENSION = /(?:\.d)?\.ts$|\.js$/;
@ -21,8 +21,8 @@ export function normalizeSeparators(path: string): string {
/**
* Remove a .ts, .d.ts, or .js extension from a file name.
*/
export function stripExtension(path: string): string {
return path.replace(TS_DTS_JS_EXTENSION, '');
export function stripExtension<T extends PathString>(path: T): T {
return path.replace(TS_DTS_JS_EXTENSION, '') as T;
}
export function getSourceFileOrError(program: ts.Program, fileName: AbsoluteFsPath): ts.SourceFile {

View File

@ -7,6 +7,7 @@
*/
import * as realFs from 'fs';
import * as fsExtra from 'fs-extra';
import * as os from 'os';
import {absoluteFrom, dirname, relativeFrom, setFileSystem} from '../src/helpers';
import {NodeJSFileSystem} from '../src/node_js_file_system';
import {AbsoluteFsPath} from '../src/types';
@ -245,4 +246,13 @@ describe('NodeJSFileSystem', () => {
expect(fs.isCaseSensitive()).toEqual(isCaseSensitive);
});
});
if (os.platform() === 'win32') {
// Only relevant on Windows
describe('relative', () => {
it('should handle Windows paths on different drives', () => {
expect(fs.relative('C:\\a\\b\\c', 'D:\\a\\b\\d')).toEqual(absoluteFrom('D:\\a\\b\\d'));
});
});
}
});

View File

@ -214,7 +214,7 @@ export abstract class MockFileSystem implements FileSystem {
abstract resolve(...paths: string[]): AbsoluteFsPath;
abstract dirname<T extends string>(file: T): T;
abstract join<T extends string>(basePath: T, ...paths: string[]): T;
abstract relative<T extends PathString>(from: T, to: T): PathSegment;
abstract relative<T extends PathString>(from: T, to: T): PathSegment|AbsoluteFsPath;
abstract basename(filePath: string, extension?: string): PathSegment;
abstract isRooted(path: string): boolean;
abstract normalize<T extends PathString>(path: T): T;

View File

@ -30,7 +30,7 @@ export class MockFileSystemNative extends MockFileSystem {
join<T extends string>(basePath: T, ...paths: string[]): T {
return NodeJSFileSystem.prototype.join.call(this, basePath, ...paths) as T;
}
relative<T extends PathString>(from: T, to: T): PathSegment {
relative<T extends PathString>(from: T, to: T): PathSegment|AbsoluteFsPath {
return NodeJSFileSystem.prototype.relative.call(this, from, to);
}

View File

@ -25,8 +25,8 @@ export class MockFileSystemPosix extends MockFileSystem {
return this.normalize(p.posix.join(basePath, ...paths)) as T;
}
relative<T extends PathString>(from: T, to: T): PathSegment {
return this.normalize(p.posix.relative(from, to)) as PathSegment;
relative<T extends PathString>(from: T, to: T): PathSegment|AbsoluteFsPath {
return this.normalize(p.posix.relative(from, to)) as PathSegment | AbsoluteFsPath;
}
basename(filePath: string, extension?: string): PathSegment {

View File

@ -25,8 +25,8 @@ export class MockFileSystemWindows extends MockFileSystem {
return this.normalize(p.win32.join(basePath, ...paths)) as T;
}
relative<T extends PathString>(from: T, to: T): PathSegment {
return this.normalize(p.win32.relative(from, to)) as PathSegment;
relative<T extends PathString>(from: T, to: T): PathSegment|AbsoluteFsPath {
return this.normalize(p.win32.relative(from, to)) as PathSegment | AbsoluteFsPath;
}
basename(filePath: string, extension?: string): PathSegment {

View File

@ -9,7 +9,7 @@ import {Expression, ExternalExpr, ExternalReference, WrappedNodeExpr} from '@ang
import * as ts from 'typescript';
import {UnifiedModulesHost} from '../../core/api';
import {absoluteFromSourceFile, dirname, LogicalFileSystem, LogicalProjectPath, PathSegment, relative} from '../../file_system';
import {absoluteFromSourceFile, dirname, LogicalFileSystem, LogicalProjectPath, relative, toRelativeImport} from '../../file_system';
import {stripExtension} from '../../file_system/src/util';
import {ReflectionHost} from '../../reflection';
import {getSourceFile, isDeclaration, isTypeDeclaration, nodeNameForError} from '../../util/src/typescript';
@ -269,11 +269,9 @@ export class RelativePathStrategy implements ReferenceEmitStrategy {
emit(ref: Reference<ts.Node>, context: ts.SourceFile): Expression|null {
const destSf = getSourceFile(ref.node);
let moduleName = stripExtension(
relative(dirname(absoluteFromSourceFile(context)), absoluteFromSourceFile(destSf)));
if (!moduleName.startsWith('../')) {
moduleName = ('./' + moduleName) as PathSegment;
}
const relativePath =
relative(dirname(absoluteFromSourceFile(context)), absoluteFromSourceFile(destSf));
const moduleName = toRelativeImport(stripExtension(relativePath));
const name = findExportedNameOfNode(ref.node, destSf, this.reflector);
return new ExternalExpr({moduleName, name});

View File

@ -5,23 +5,12 @@
* 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 {dirname, relative, resolve} from '../../file_system';
const TS_DTS_JS_EXTENSION = /(?:\.d)?\.ts$|\.js$/;
import {dirname, relative, resolve, toRelativeImport} from '../../file_system';
import {stripExtension} from '../../file_system/src/util';
export function relativePathBetween(from: string, to: string): string|null {
let relativePath = relative(dirname(resolve(from)), resolve(to)).replace(TS_DTS_JS_EXTENSION, '');
if (relativePath === '') {
return null;
}
// path.relative() does not include the leading './'.
if (!relativePath.startsWith('.')) {
relativePath = `./${relativePath}`;
}
return relativePath;
const relativePath = stripExtension(relative(dirname(resolve(from)), resolve(to)));
return relativePath !== '' ? toRelativeImport(relativePath) : null;
}
export function normalizeSeparators(path: string): string {

View File

@ -12,7 +12,7 @@ import * as ts from 'typescript';
import {createCompilerHost, createProgram} from '../../index';
import {main, mainDiagnosticsForTest, readNgcCommandLineAndConfiguration} from '../../src/main';
import {absoluteFrom, AbsoluteFsPath, FileSystem, getFileSystem, NgtscCompilerHost} from '../../src/ngtsc/file_system';
import {absoluteFrom, AbsoluteFsPath, FileSystem, getFileSystem, NgtscCompilerHost, relativeFrom} from '../../src/ngtsc/file_system';
import {Folder, MockFileSystem} from '../../src/ngtsc/file_system/testing';
import {IndexedComponent} from '../../src/ngtsc/indexer';
import {NgtscProgram} from '../../src/ngtsc/program';
@ -274,7 +274,8 @@ const ROOT_PREFIX = 'root/';
class FileNameToModuleNameHost extends AugmentedCompilerHost {
fileNameToModuleName(importedFilePath: string): string {
const relativeFilePath = this.fs.relative(this.fs.pwd(), this.fs.resolve(importedFilePath));
const relativeFilePath =
relativeFrom(this.fs.relative(this.fs.pwd(), this.fs.resolve(importedFilePath)));
const rootedPath = this.fs.join('root', relativeFilePath);
return rootedPath.replace(/(\.d)?.ts$/, '');
}

View File

@ -17,14 +17,14 @@ import {TranslationBundle, TranslationHandler} from '../translator';
export class AssetTranslationHandler implements TranslationHandler {
constructor(private fs: FileSystem) {}
canTranslate(_relativeFilePath: PathSegment, _contents: Buffer): boolean {
canTranslate(_relativeFilePath: PathSegment|AbsoluteFsPath, _contents: Buffer): boolean {
return true;
}
translate(
diagnostics: Diagnostics, _sourceRoot: AbsoluteFsPath, relativeFilePath: PathSegment,
contents: Buffer, outputPathFn: OutputPathFn, translations: TranslationBundle[],
sourceLocale?: string): void {
diagnostics: Diagnostics, _sourceRoot: AbsoluteFsPath,
relativeFilePath: PathSegment|AbsoluteFsPath, contents: Buffer, outputPathFn: OutputPathFn,
translations: TranslationBundle[], sourceLocale?: string): void {
for (const translation of translations) {
this.writeAssetFile(
diagnostics, outputPathFn, translation.locale, relativeFilePath, contents);
@ -36,7 +36,7 @@ export class AssetTranslationHandler implements TranslationHandler {
private writeAssetFile(
diagnostics: Diagnostics, outputPathFn: OutputPathFn, locale: string,
relativeFilePath: PathSegment, contents: Buffer): void {
relativeFilePath: PathSegment|AbsoluteFsPath, contents: Buffer): void {
try {
const outputPath = absoluteFrom(outputPathFn(locale, relativeFilePath));
this.fs.ensureDir(this.fs.dirname(outputPath));

View File

@ -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 {absoluteFrom, AbsoluteFsPath, FileSystem, PathSegment, relativeFrom} from '@angular/compiler-cli/src/ngtsc/file_system';
import {absoluteFrom, AbsoluteFsPath, FileSystem, PathSegment} from '@angular/compiler-cli/src/ngtsc/file_system';
import {parseSync, transformFromAstSync} from '@babel/core';
import {File, Program} from '@babel/types';
@ -27,8 +27,8 @@ export class SourceFileTranslationHandler implements TranslationHandler {
TranslatePluginOptions = {...this.translationOptions, missingTranslation: 'ignore'};
constructor(private fs: FileSystem, private translationOptions: TranslatePluginOptions = {}) {}
canTranslate(relativeFilePath: PathSegment, _contents: Buffer): boolean {
return this.fs.extname(relativeFrom(relativeFilePath)) === '.js';
canTranslate(relativeFilePath: PathSegment|AbsoluteFsPath, _contents: Buffer): boolean {
return this.fs.extname(relativeFilePath) === '.js';
}
translate(

View File

@ -35,7 +35,7 @@ export interface TranslationHandler {
* @param relativeFilePath A relative path from the sourceRoot to the resource file to handle.
* @param contents The contents of the file to handle.
*/
canTranslate(relativeFilePath: PathSegment, contents: Buffer): boolean;
canTranslate(relativeFilePath: PathSegment|AbsoluteFsPath, contents: Buffer): boolean;
/**
* Translate the file at `relativeFilePath` containing `contents`, using the given `translations`,
@ -53,9 +53,9 @@ export interface TranslationHandler {
* stripped out.
*/
translate(
diagnostics: Diagnostics, sourceRoot: AbsoluteFsPath, relativeFilePath: PathSegment,
contents: Buffer, outputPathFn: OutputPathFn, translations: TranslationBundle[],
sourceLocale?: string): void;
diagnostics: Diagnostics, sourceRoot: AbsoluteFsPath,
relativeFilePath: PathSegment|AbsoluteFsPath, contents: Buffer, outputPathFn: OutputPathFn,
translations: TranslationBundle[], sourceLocale?: string): void;
}
/**