From 007a8f6fe9d778d6c0a547724b1fb13d2ade49d1 Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Mon, 16 Dec 2019 16:38:01 -0800 Subject: [PATCH] feat(builtin): use linker for all generated :bin targets rather than loader.js module-resolver hacks for generated targets, we now expect that tools will use the linker to set up their node_modules tree, or will explicitly use a runfiles helper library to resolve files in the runfiles, like other languages in Bazel. BREAKING CHANGE: If you use the generated nodejs_binary or nodejs_test rules in the npm workspace, for example @npm//typescript/bin:tsc, your custom rule must now link the node_modules directory into that process. A typical way to do this is with the run_node helper. See updates to examples in this commit. --- examples/BUILD.bazel | 4 +++ examples/parcel/parcel.bzl | 9 +++++-- internal/linker/index.js | 17 +++++++++--- internal/linker/link_node_modules.ts | 15 ++++++++--- internal/node/node_patches.js | 5 +++- internal/npm_install/generate_build_file.ts | 3 ++- internal/npm_install/index.js | 3 ++- .../test-a/bin/BUILD.bazel.golden | 1 + .../golden/jasmine/bin/BUILD.bazel.golden | 1 + internal/providers/node_runtime_deps_info.bzl | 3 +-- packages/karma/src/karma.conf.js | 27 ++++++++++++------- packages/karma/src/karma_web_test.bzl | 10 +++++-- .../test/karma_typescript/user_files.spec.js | 4 +-- packages/node-patches/src/subprocess.ts | 7 ++++- packages/terser/src/index.js | 2 +- packages/terser/src/terser_minified.bzl | 7 +++-- 16 files changed, 86 insertions(+), 32 deletions(-) diff --git a/examples/BUILD.bazel b/examples/BUILD.bazel index b5228ea44e..3b798945c3 100644 --- a/examples/BUILD.bazel +++ b/examples/BUILD.bazel @@ -91,6 +91,10 @@ example_integration_test( npm_packages = { "//packages/jasmine:npm_package": "@bazel/jasmine", }, + # Parcel spawns a subprocess which requires our node-patches + # but we don't yet have a mechanism on Windows for spawned processes + # to inherit the --require script needed to install the patches + tags = ["no-bazelci-windows"], ) example_integration_test( diff --git a/examples/parcel/parcel.bzl b/examples/parcel/parcel.bzl index 458dbffd12..eab5dcced0 100644 --- a/examples/parcel/parcel.bzl +++ b/examples/parcel/parcel.bzl @@ -17,6 +17,8 @@ This is not a full-featured Parcel bazel rule, just enough to demonstrate how to write one. """ +load("@build_bazel_rules_nodejs//:providers.bzl", "run_node") + def _parcel_impl(ctx): """The "implementation function" for our rule. @@ -31,9 +33,12 @@ def _parcel_impl(ctx): args += ["--out-dir", ctx.outputs.bundle.dirname] args += ["--out-file", ctx.outputs.bundle.basename] - ctx.actions.run( + # We use the run_node helper rather than ctx.actions.run so that the npm package + # gets automatically included in the action inputs + run_node( + ctx = ctx, inputs = ctx.files.srcs + [ctx.file.entry_point], - executable = ctx.executable.parcel, + executable = "parcel", outputs = [ctx.outputs.bundle, ctx.outputs.sourcemap], arguments = args, env = {"COMPILATION_MODE": ctx.var["COMPILATION_MODE"]}, diff --git a/internal/linker/index.js b/internal/linker/index.js index e130315ad0..c6072bb217 100644 --- a/internal/linker/index.js +++ b/internal/linker/index.js @@ -143,12 +143,16 @@ class Runfiles { } resolve(modulePath) { if (this.manifest) { - return this.lookupDirectory(modulePath); + const result = this.lookupDirectory(modulePath); + if (result) + return result; } - if (exports.runfiles.dir) { + if (exports.runfiles.dir && fs.existsSync(path.join(exports.runfiles.dir, modulePath))) { return path.join(exports.runfiles.dir, modulePath); } - throw new Error(`could not resolve modulePath ${modulePath}`); + const e = new Error(`could not resolve modulePath ${modulePath}`); + e.code = 'MODULE_NOT_FOUND'; + throw e; } resolveWorkspaceRelative(modulePath) { if (!this.workspace) { @@ -345,7 +349,12 @@ function main(args, runfiles) { else { runfilesPath = `${workspace}/${runfilesPath}`; } - target = runfiles.resolve(runfilesPath) || ''; + try { + target = runfiles.resolve(runfilesPath); + } + catch (_a) { + target = ''; + } break; } yield symlink(target, m.name); diff --git a/internal/linker/link_node_modules.ts b/internal/linker/link_node_modules.ts index 7929b2561b..0c5fc42b8c 100644 --- a/internal/linker/link_node_modules.ts +++ b/internal/linker/link_node_modules.ts @@ -245,12 +245,15 @@ export class Runfiles { resolve(modulePath: string) { // Look in the runfiles first if (this.manifest) { - return this.lookupDirectory(modulePath); + const result = this.lookupDirectory(modulePath); + if (result) return result; } - if (runfiles.dir) { + if (runfiles.dir && fs.existsSync(path.join(runfiles.dir, modulePath))) { return path.join(runfiles.dir, modulePath); } - throw new Error(`could not resolve modulePath ${modulePath}`); + const e = new Error(`could not resolve modulePath ${modulePath}`); + (e as any).code = 'MODULE_NOT_FOUND'; + throw e; } resolveWorkspaceRelative(modulePath: string) { @@ -569,7 +572,11 @@ export async function main(args: string[], runfiles: Runfiles) { } else { runfilesPath = `${workspace}/${runfilesPath}`; } - target = runfiles.resolve(runfilesPath) || ''; + try { + target = runfiles.resolve(runfilesPath); + } catch { + target = ''; + } break; } diff --git a/internal/node/node_patches.js b/internal/node/node_patches.js index 3447ec22a9..2318e6cdf6 100644 --- a/internal/node/node_patches.js +++ b/internal/node/node_patches.js @@ -593,7 +593,10 @@ fi process.env.PATH = nodeDir + path.delimiter + process.env.PATH; } // fix execPath so folks use the proxy node - process.argv[0] = process.execPath = path.join(nodeDir, 'node'); + if (process.platform == 'win32') ; + else { + process.argv[0] = process.execPath = path.join(nodeDir, 'node'); + } // replace any instances of require script in execArgv with the absolute path to the script. // example: bazel-require-script.js process.execArgv.map(v => { diff --git a/internal/npm_install/generate_build_file.ts b/internal/npm_install/generate_build_file.ts index 7c8d420105..10fc077c15 100644 --- a/internal/npm_install/generate_build_file.ts +++ b/internal/npm_install/generate_build_file.ts @@ -1074,7 +1074,8 @@ nodejs_binary( name = "${name}", entry_point = "//:node_modules/${pkg._dir}/${path}", install_source_map_support = False, - data = [${data.map(p => `"${p}"`).join(', ')}],${additionalAttributes(pkg, name)} + data = [${data.map(p => `"${p}"`).join(', ')}], + templated_args = ["--nobazel_patch_module_resolver"],${additionalAttributes(pkg, name)} ) `; } diff --git a/internal/npm_install/index.js b/internal/npm_install/index.js index c1fa0da69c..71def726d6 100644 --- a/internal/npm_install/index.js +++ b/internal/npm_install/index.js @@ -645,7 +645,8 @@ nodejs_binary( name = "${name}", entry_point = "//:node_modules/${pkg._dir}/${path}", install_source_map_support = False, - data = [${data.map(p => `"${p}"`).join(', ')}],${additionalAttributes(pkg, name)} + data = [${data.map(p => `"${p}"`).join(', ')}], + templated_args = ["--nobazel_patch_module_resolver"],${additionalAttributes(pkg, name)} ) `; } diff --git a/internal/npm_install/test/golden/@gregmagolan/test-a/bin/BUILD.bazel.golden b/internal/npm_install/test/golden/@gregmagolan/test-a/bin/BUILD.bazel.golden index b999b71d1e..c49118f926 100644 --- a/internal/npm_install/test/golden/@gregmagolan/test-a/bin/BUILD.bazel.golden +++ b/internal/npm_install/test/golden/@gregmagolan/test-a/bin/BUILD.bazel.golden @@ -6,4 +6,5 @@ nodejs_binary( entry_point = "//:node_modules/@gregmagolan/test-a/@bin/test.js", install_source_map_support = False, data = ["//@gregmagolan/test-a:test-a"], + templated_args = ["--nobazel_patch_module_resolver"], ) diff --git a/internal/npm_install/test/golden/jasmine/bin/BUILD.bazel.golden b/internal/npm_install/test/golden/jasmine/bin/BUILD.bazel.golden index 0ce9a6f06e..93c29fa488 100644 --- a/internal/npm_install/test/golden/jasmine/bin/BUILD.bazel.golden +++ b/internal/npm_install/test/golden/jasmine/bin/BUILD.bazel.golden @@ -6,4 +6,5 @@ nodejs_binary( entry_point = "//:node_modules/jasmine/bin/jasmine.js", install_source_map_support = False, data = ["//jasmine:jasmine"], + templated_args = ["--nobazel_patch_module_resolver"], ) diff --git a/internal/providers/node_runtime_deps_info.bzl b/internal/providers/node_runtime_deps_info.bzl index aa08711c48..18597bdbf9 100644 --- a/internal/providers/node_runtime_deps_info.bzl +++ b/internal/providers/node_runtime_deps_info.bzl @@ -74,7 +74,6 @@ def run_node(ctx, inputs, arguments, executable, **kwargs): modules_manifest = write_node_modules_manifest(ctx, link_data) add_arg(arguments, "--bazel_node_modules_manifest=%s" % modules_manifest.path) - inputs.append(modules_manifest) # By using the run_node helper, you suggest that your program # doesn't implicitly use runfiles to require() things @@ -98,7 +97,7 @@ def run_node(ctx, inputs, arguments, executable, **kwargs): env["BAZEL_NODE_MODULES_ROOT"] = _compute_node_modules_root(ctx) ctx.actions.run( - inputs = inputs + extra_inputs, + inputs = inputs + extra_inputs + [modules_manifest], arguments = arguments, executable = exec_exec, env = env, diff --git a/packages/karma/src/karma.conf.js b/packages/karma/src/karma.conf.js index cb3f631335..54f627a95f 100644 --- a/packages/karma/src/karma.conf.js +++ b/packages/karma/src/karma.conf.js @@ -5,6 +5,7 @@ try { const path = require('path'); const tmp = require('tmp'); const child_process = require('child_process'); + const runfiles = require(process.env['BAZEL_NODE_RUNFILES_HELPER']); const VERBOSE_LOGS = !!process.env['VERBOSE_LOGS']; @@ -230,7 +231,7 @@ try { // In Windows, the runfile will probably not be symlinked. Se we need to // serve the real file through karma, and proxy calls to the expected file // location in the runfiles to the real file. - const resolvedFile = require.resolve(f); + const resolvedFile = runfiles.resolve(f); conf.files.push({pattern: resolvedFile, included: false}); // Prefixing the proxy path with '/absolute' allows karma to load local // files. This doesn't see to be an official API. @@ -249,14 +250,14 @@ try { let filePath = null; if (f.startsWith('NODE_MODULES/')) { try { - // attempt to resolve in @bazel/typescript nested node_modules first - filePath = require.resolve(f.replace(/^NODE_MODULES\//, '@bazel/karma/node_modules/')); + // attempt to resolve in nested node_modules first + filePath = runfiles.resolve(f.replace(/^NODE_MODULES\//, '@bazel/karma/node_modules/')); } catch (e) { // if that failed then attempt to resolve in root node_modules - filePath = require.resolve(f.replace(/^NODE_MODULES\//, '')); + filePath = runfiles.resolve(f.replace(/^NODE_MODULES\//, '')); } } else { - filePath = require.resolve(f); + filePath = runfiles.resolve(f); } conf.files.push(filePath); @@ -308,7 +309,7 @@ try { overrideConfigValue(conf, 'browsers', []); overrideConfigValue(conf, 'customLaunchers', null); - const webTestMetadata = require(process.env['WEB_TEST_METADATA']); + const webTestMetadata = require(runfiles.resolve(process.env['WEB_TEST_METADATA'])); log_verbose(`WEB_TEST_METADATA: ${JSON.stringify(webTestMetadata, null, 2)}`); if (webTestMetadata['environment'] === 'local') { // When a local chrome or firefox browser is chosen such as @@ -327,7 +328,11 @@ try { process.env.CHROME_BIN = extractWebArchive(extractExe, archiveFile, webTestNamedFiles['CHROMIUM']); } else { - process.env.CHROME_BIN = require.resolve(webTestNamedFiles['CHROMIUM']); + try { + process.env.CHROME_BIN = runfiles.resolve(webTestNamedFiles['CHROMIUM']); + } catch { + // Ignore; karma may still find Chrome another way + } } const browser = process.env['DISPLAY'] ? 'Chrome' : 'ChromeHeadless'; if (!supportChromeSandboxing()) { @@ -345,7 +350,11 @@ try { process.env.FIREFOX_BIN = extractWebArchive(extractExe, archiveFile, webTestNamedFiles['FIREFOX']); } else { - process.env.FIREFOX_BIN = require.resolve(webTestNamedFiles['FIREFOX']); + try { + process.env.FIREFOX_BIN = runfiles.resolve(webTestNamedFiles['FIREFOX']); + } catch { + // Ignore; karma may still find Firefox another way + } } conf.browsers.push(process.env['DISPLAY'] ? 'Firefox' : 'FirefoxHeadless'); } @@ -403,7 +412,7 @@ try { // Import the user's base karma configuration if specified if (configPath) { - const baseConf = require(configPath); + const baseConf = require(runfiles.resolve(configPath)); if (typeof baseConf !== 'function') { throw new Error( 'Invalid base karma configuration. Expected config function to be exported.'); diff --git a/packages/karma/src/karma_web_test.bzl b/packages/karma/src/karma_web_test.bzl index 6ad4c3fbf7..67ee22e56e 100644 --- a/packages/karma/src/karma_web_test.bzl +++ b/packages/karma/src/karma_web_test.bzl @@ -110,6 +110,12 @@ def _write_amd_names_shim(ctx): def _filter_js(files): return [f for f in files if f.extension == "js" or f.extension == "mjs"] +def _find_dep(ctx, suffix): + for d in ctx.files.deps: + if (d.path.endswith(suffix)): + return _to_manifest_path(ctx, d) + fail("couldn't find file %s in the deps" % suffix) + # Generates the karma configuration file for the rule def _write_karma_config(ctx, files, amd_names_shim): configuration = ctx.actions.declare_file( @@ -149,8 +155,8 @@ def _write_karma_config(ctx, files, amd_names_shim): # for a priority require of nested `@bazel/typescript/node_modules` before # looking in root node_modules. bootstrap_entries += [ - "NODE_MODULES/requirejs/require.js", - "NODE_MODULES/karma-requirejs/lib/adapter.js", + "NODE_MODULES/%s" % _find_dep(ctx, "requirejs/require.js"), + "NODE_MODULES/%s" % _find_dep(ctx, "karma-requirejs/lib/adapter.js"), "/".join([ctx.workspace_name, amd_names_shim.short_path]), ] diff --git a/packages/karma/test/karma_typescript/user_files.spec.js b/packages/karma/test/karma_typescript/user_files.spec.js index e411408594..e856981a29 100644 --- a/packages/karma/test/karma_typescript/user_files.spec.js +++ b/packages/karma/test/karma_typescript/user_files.spec.js @@ -18,8 +18,8 @@ describe('karma_web_test_suite', () => { return l.trim().slice(1, -1); }).filter(l => !!l); expect(files).toEqual([ - 'NODE_MODULES/requirejs/require.js', - 'NODE_MODULES/karma-requirejs/lib/adapter.js', + 'NODE_MODULES/npm/node_modules/requirejs/require.js', + 'NODE_MODULES/npm/node_modules/karma-requirejs/lib/adapter.js', 'build_bazel_rules_nodejs/packages/karma/test/karma_typescript/_testing_wrapped_test.amd_names_shim.js', ]); }); diff --git a/packages/node-patches/src/subprocess.ts b/packages/node-patches/src/subprocess.ts index 892a0a23b9..9715dcbebd 100644 --- a/packages/node-patches/src/subprocess.ts +++ b/packages/node-patches/src/subprocess.ts @@ -47,7 +47,12 @@ fi } // fix execPath so folks use the proxy node - process.argv[0] = process.execPath = path.join(nodeDir, 'node'); + if (process.platform == 'win32') { + // FIXME: need to make an exe, or run in a shell so we can use .bat + } else { + process.argv[0] = process.execPath = path.join(nodeDir, 'node'); + } + // replace any instances of require script in execArgv with the absolute path to the script. // example: bazel-require-script.js diff --git a/packages/terser/src/index.js b/packages/terser/src/index.js index 4b60d182c3..dd4bdde47b 100644 --- a/packages/terser/src/index.js +++ b/packages/terser/src/index.js @@ -72,7 +72,7 @@ function terserDirectory(input, output, residual, terserBinary) { ...directoryArgs(residual, inputFile, outputFile) ]; - spawn(process.execPath, args) + spawn(process.execPath, [...process.execArgv, ...args]) .then( (data) => { if (data.code) { diff --git a/packages/terser/src/terser_minified.bzl b/packages/terser/src/terser_minified.bzl index c8c4553ce1..7eb49a4e42 100644 --- a/packages/terser/src/terser_minified.bzl +++ b/packages/terser/src/terser_minified.bzl @@ -14,6 +14,8 @@ "Rule to run the terser binary under bazel" +load("@build_bazel_rules_nodejs//:providers.bzl", "run_node") + _DOC = """Run the terser minifier. Typical example: @@ -166,10 +168,11 @@ def _terser(ctx): args.add_all(["--config-file", opts.path]) args.add_all(ctx.attr.args) - ctx.actions.run( + run_node( + ctx, inputs = inputs, outputs = outputs, - executable = ctx.executable.terser_bin, + executable = "terser_bin", arguments = [args], env = {"COMPILATION_MODE": ctx.var["COMPILATION_MODE"]}, progress_message = "Minifying JavaScript %s [terser]" % (outputs[0].short_path),