From 9ad54d74d29afa1e4ee8d49c9035239eaf502fb2 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Mon, 29 Oct 2018 21:25:00 +0100 Subject: [PATCH] build: fix ts-api-guardian does not work on windows w/ bazel (#26761) * Fixes that the `ts-api-guardian` package does not work on Windows with Bazel. This is because `ts-api-guardian` does not resolve the runfiles through theNodeJS `require` function that properly handles runfiles within Bazel. PR Close #26761 --- tools/public_api_guard/public_api_guard.bzl | 4 +- tools/ts-api-guardian/BUILD.bazel | 2 + tools/ts-api-guardian/lib/cli.ts | 31 ++++++++++++--- tools/ts-api-guardian/test/bootstrap.ts | 12 ++++-- tools/ts-api-guardian/test/cli_e2e_test.ts | 14 ++++--- tools/ts-api-guardian/test/cli_unit_test.ts | 43 ++++----------------- 6 files changed, 54 insertions(+), 52 deletions(-) diff --git a/tools/public_api_guard/public_api_guard.bzl b/tools/public_api_guard/public_api_guard.bzl index c7d538a069..56b877ae0f 100644 --- a/tools/public_api_guard/public_api_guard.bzl +++ b/tools/public_api_guard/public_api_guard.bzl @@ -16,7 +16,7 @@ def generate_targets(golden_files): [package_name, entry_point_tail] = entry_point.split("/", 1) directory_name = entry_point_tail.split("/")[-1] target_suffix = "/" + entry_point_tail if package_name != entry_point_tail else "" - actual_file = "packages/%s%s/%s.d.ts" % (package_name, target_suffix, directory_name) + actual_file = "angular/packages/%s%s/%s.d.ts" % (package_name, target_suffix, directory_name) label_name = package_name + target_suffix.replace("/", "_") ts_api_guardian_test( @@ -25,7 +25,7 @@ def generate_targets(golden_files): data = [golden_file] + [ "//packages/%s:%s" % (package_name + target_suffix, directory_name), ], - golden = "tools/public_api_guard/%s" % golden_file, + golden = "angular/tools/public_api_guard/%s" % golden_file, tags = [ "fixme-ivy-aot", # ivy no longer emits generated index file "no-ivy-jit", # we will not ship JIT compiled packages to npm diff --git a/tools/ts-api-guardian/BUILD.bazel b/tools/ts-api-guardian/BUILD.bazel index b22e829c1b..d6dbdc3f07 100644 --- a/tools/ts-api-guardian/BUILD.bazel +++ b/tools/ts-api-guardian/BUILD.bazel @@ -78,6 +78,8 @@ jasmine_node_test( ]) + [ ":ts-api-guardian", "@ts-api-guardian_deps//chalk", + # TODO: remove this once the boostrap.js workaround has been removed. + ":package.json", ], node_modules = "@ts-api-guardian_deps//typescript:typescript__typings", tags = ["local"], diff --git a/tools/ts-api-guardian/lib/cli.ts b/tools/ts-api-guardian/lib/cli.ts index 44195ab75d..6c29c51cf8 100644 --- a/tools/ts-api-guardian/lib/cli.ts +++ b/tools/ts-api-guardian/lib/cli.ts @@ -53,7 +53,7 @@ export function startCli() { if (mode === 'help') { printUsageAndExit(!!errors.length); } else { - const targets = generateFileNamePairs(argv, mode); + const targets = resolveFileNamePairs(argv, mode); if (mode === 'out') { for (const {entrypoint, goldenFile} of targets) { @@ -180,19 +180,38 @@ Options: process.exit(error ? 1 : 0); } -export function generateFileNamePairs( +/** + * Resolves a given path to the associated relative path if the current process runs within + * Bazel. We need to use the wrapped NodeJS resolve logic in order to properly handle the given + * runfiles files which are only part of the runfile manifest on Windows. + */ +function resolveBazelFilePath(fileName: string): string { + // If the CLI has been launched through the NodeJS Bazel rules, we need to resolve the + // actual file paths because otherwise this script won't work on Windows where runfiles + // are not available in the working directory. In order to resolve the real path for the + // runfile, we need to use `require.resolve` which handles runfiles properly on Windows. + if (process.env['BAZEL_TARGET']) { + return path.posix.relative(process.cwd(), require.resolve(fileName)); + } + + return fileName; +} + +function resolveFileNamePairs( argv: minimist.ParsedArgs, mode: string): {entrypoint: string, goldenFile: string}[] { if (argv[mode]) { - return [{entrypoint: argv._[0], goldenFile: argv[mode]}]; - + return [{ + entrypoint: resolveBazelFilePath(argv._[0]), + goldenFile: resolveBazelFilePath(argv[mode]), + }]; } else { // argv[mode + 'Dir'] let rootDir = argv['rootDir'] || '.'; const goldenDir = argv[mode + 'Dir']; return argv._.map((fileName: string) => { return { - entrypoint: fileName, - goldenFile: path.join(goldenDir, path.relative(rootDir, fileName)) + entrypoint: resolveBazelFilePath(fileName), + goldenFile: resolveBazelFilePath(path.join(goldenDir, path.relative(rootDir, fileName))), }; }); } diff --git a/tools/ts-api-guardian/test/bootstrap.ts b/tools/ts-api-guardian/test/bootstrap.ts index 8c6514a266..6b57f8f54c 100644 --- a/tools/ts-api-guardian/test/bootstrap.ts +++ b/tools/ts-api-guardian/test/bootstrap.ts @@ -7,6 +7,12 @@ */ import * as path from 'path'; -// Before running tests, change directories to our directory under the runfiles -// From runfiles we want to go to the angular/tools/ts-api-guardian subfolder. -process.chdir(path.join(process.env['TEST_SRCDIR'], 'angular', 'tools', 'ts-api-guardian')); + +// Resolve the path to the package.json of the ts-api-guardian. We need to resolve an actual +// path of a runfile because we want to determine the path to the directory that includes all +// test fixture runfiles. On Windows this is usually the original non-sandboxed disk location, +// otherwise this just refers to the runfile directory with all the proper symlinked files. +// TODO: remove the whole bootstrap file once the tests are Bazel and Windows compatible. +const runfilesDirectory = path.dirname(require.resolve('../package.json')); + +process.chdir(runfilesDirectory); diff --git a/tools/ts-api-guardian/test/cli_e2e_test.ts b/tools/ts-api-guardian/test/cli_e2e_test.ts index 7e102ac878..3c540b7bc2 100644 --- a/tools/ts-api-guardian/test/cli_e2e_test.ts +++ b/tools/ts-api-guardian/test/cli_e2e_test.ts @@ -12,7 +12,7 @@ import * as fs from 'fs'; import * as path from 'path'; import {assertFileEqual, unlinkRecursively} from './helpers'; -const BINARY = 'ts-api-guardian/bin/ts-api-guardian'; +const BINARY_PATH = require.resolve('../ts-api-guardian/bin/ts-api-guardian'); describe('cli: e2e test', () => { const outDir = path.join(process.env['TEST_TMPDIR'], 'tmp'); @@ -78,7 +78,7 @@ describe('cli: e2e test', () => { }); it('should generate respecting --stripExportPattern', () => { - const {stdout, status} = execute([ + const {status} = execute([ '--out', path.join(outDir, 'underscored.d.ts'), '--stripExportPattern', '^__.*', 'test/fixtures/underscored.d.ts' ]); @@ -88,7 +88,7 @@ describe('cli: e2e test', () => { }); it('should not throw for aliased stripped exports', () => { - const {stdout, status} = execute([ + const {status} = execute([ '--out', path.join(outDir, 'stripped_alias.d.ts'), '--stripExportPattern', '^__.*', 'test/fixtures/stripped_alias.d.ts' ]); @@ -121,9 +121,13 @@ function copyFile(sourceFile: string, targetFile: string) { } function execute(args: string[]): {stdout: string, stderr: string, status: number} { - const output = child_process.spawnSync(process.execPath, [path.resolve(BINARY), ...args], { + // We need to determine the directory that includes the `ts-api-guardian` npm_package that + // will be used to spawn the CLI binary. This is a workaround because technically we shouldn't + // spawn a child process that doesn't have the custom NodeJS module resolution for Bazel. + const nodePath = path.join(path.dirname(require.resolve('../lib/cli.js')), '../'); + const output = child_process.spawnSync(process.execPath, [BINARY_PATH, ...args], { env: { - 'NODE_PATH': process.cwd(), + 'NODE_PATH': nodePath, } }); chai.assert(!output.error, 'Child process failed or timed out: ' + output.error); diff --git a/tools/ts-api-guardian/test/cli_unit_test.ts b/tools/ts-api-guardian/test/cli_unit_test.ts index 278df98103..85e29cf0ff 100644 --- a/tools/ts-api-guardian/test/cli_unit_test.ts +++ b/tools/ts-api-guardian/test/cli_unit_test.ts @@ -7,45 +7,42 @@ */ import chai = require('chai'); -import * as ts from 'typescript'; -import {parseArguments, generateFileNamePairs} from '../lib/cli'; - +import {parseArguments} from '../lib/cli'; describe('cli: parseArguments', () => { it('should show usage with error when supplied with no arguments', () => { - const {argv, mode, errors} = parseArguments([]); + const {mode, errors} = parseArguments([]); chai.assert.equal(mode, 'help'); chai.assert.deepEqual(errors, ['No input file specified.']); }); it('should show usage without error when supplied with --help', () => { - const {argv, mode, errors} = parseArguments(['--help']); + const {mode, errors} = parseArguments(['--help']); chai.assert.equal(mode, 'help'); chai.assert.deepEqual(errors, []); }); it('should show usage with error when supplied with none of --out/verify[Dir]', () => { - const {argv, mode, errors} = parseArguments(['input.d.ts']); + const {mode, errors} = parseArguments(['input.d.ts']); chai.assert.equal(mode, 'help'); chai.assert.deepEqual(errors, ['Specify either --out[Dir] or --verify[Dir]']); }); it('should show usage with error when supplied with both of --out/verify[Dir]', () => { - const {argv, mode, errors} = + const {mode, errors} = parseArguments(['--out', 'out.d.ts', '--verifyDir', 'golden.d.ts', 'input.d.ts']); chai.assert.equal(mode, 'help'); chai.assert.deepEqual(errors, ['Specify either --out[Dir] or --verify[Dir]']); }); it('should show usage with error when supplied without input file', () => { - const {argv, mode, errors} = parseArguments(['--out', 'output.d.ts']); + const {mode, errors} = parseArguments(['--out', 'output.d.ts']); chai.assert.equal(mode, 'help'); chai.assert.deepEqual(errors, ['No input file specified.']); }); it('should show usage with error when supplied without input file', () => { - const {argv, mode, errors} = - parseArguments(['--out', 'output.d.ts', 'first.d.ts', 'second.d.ts']); + const {mode, errors} = parseArguments(['--out', 'output.d.ts', 'first.d.ts', 'second.d.ts']); chai.assert.equal(mode, 'help'); chai.assert.deepEqual(errors, ['More than one input specified. Use --outDir instead.']); }); @@ -66,29 +63,3 @@ describe('cli: parseArguments', () => { chai.assert.deepEqual(errors, []); }); }); - -describe('cli: generateFileNamePairs', () => { - it('should generate one file pair in one-file mode', () => { - chai.assert.deepEqual( - generateFileNamePairs({_: ['input.d.ts'], out: 'output.d.ts'}, 'out'), - [{entrypoint: 'input.d.ts', goldenFile: 'output.d.ts'}]); - }); - - it('should generate file pairs in multi-file mode according to current directory', () => { - chai.assert.deepEqual( - generateFileNamePairs({_: ['src/first.d.ts', 'src/second.d.ts'], outDir: 'bank'}, 'out'), [ - {entrypoint: 'src/first.d.ts', goldenFile: 'bank/src/first.d.ts'}, - {entrypoint: 'src/second.d.ts', goldenFile: 'bank/src/second.d.ts'} - ]); - }); - - it('should generate file pairs according to rootDir if provided', () => { - chai.assert.deepEqual( - generateFileNamePairs( - {_: ['src/first.d.ts', 'src/second.d.ts'], outDir: 'bank', rootDir: 'src'}, 'out'), - [ - {entrypoint: 'src/first.d.ts', goldenFile: 'bank/first.d.ts'}, - {entrypoint: 'src/second.d.ts', goldenFile: 'bank/second.d.ts'} - ]); - }); -});