From 1667d507137268bbda6dbee489fc7a65960eab1e Mon Sep 17 00:00:00 2001 From: Greg Magolan Date: Mon, 6 Apr 2020 19:49:46 -0700 Subject: [PATCH] refactor(builtin): cleanup and optimizations in nodejs_binary & linker (#1799) --- internal/linker/BUILD.bazel | 4 +- internal/linker/index.js | 31 +++++--------- internal/linker/link_node_modules.ts | 41 +++++++------------ internal/node/node.bzl | 1 - .../bazel/tools/bash/runfiles/runfiles.bash | 9 ++-- 5 files changed, 35 insertions(+), 51 deletions(-) diff --git a/internal/linker/BUILD.bazel b/internal/linker/BUILD.bazel index 08def5b3d1..90d8beb74f 100644 --- a/internal/linker/BUILD.bazel +++ b/internal/linker/BUILD.bazel @@ -4,7 +4,9 @@ load("@npm_bazel_typescript//:index.from_src.bzl", "checked_in_ts_library") # We can't bootstrap the ts_library rule using the linker itself, # because the implementation of ts_library depends on the linker so that would be a cycle. -# So we compile it to JS and check in the result as index.js +# So we compile it to JS and check in the result as index.js. +# To update index.js run: +# bazel run //internal/linker:linker_lib_check_compiled.accept checked_in_ts_library( name = "linker_lib", srcs = ["link_node_modules.ts"], diff --git a/internal/linker/index.js b/internal/linker/index.js index 2d1aadad82..6af11a4b43 100644 --- a/internal/linker/index.js +++ b/internal/linker/index.js @@ -172,10 +172,9 @@ Include as much of the build output as you can without disclosing anything confi If you want to test runfiles manifest behavior, add --spawn_strategy=standalone to the command line.`); } - const wksp = env['BAZEL_WORKSPACE']; - if (!!wksp) { - this.workspace = wksp; - } + // Bazel starts actions with pwd=execroot/my_wksp + this.workspaceDir = path.resolve('.'); + this.workspace = path.basename(this.workspaceDir); // If target is from an external workspace such as @npm//rollup/bin:rollup // resolvePackageRelative is not supported since package is in an external // workspace. @@ -238,17 +237,11 @@ Include as much of the build output as you can without disclosing anything confi throw new Error(`could not resolve modulePath ${modulePath}`); } resolveWorkspaceRelative(modulePath) { - if (!this.workspace) { - throw new Error('workspace could not be determined from the environment'); - } return this.resolve(path.posix.join(this.workspace, modulePath)); } resolvePackageRelative(modulePath) { - if (!this.workspace) { - throw new Error('workspace could not be determined from the environment'); - } if (!this.package) { - throw new Error('package could not be determined from the environment'); + throw new Error('package could not be determined from the environment; make sure BAZEL_TARGET is set'); } return this.resolve(path.posix.join(this.workspace, this.package, modulePath)); } @@ -432,17 +425,15 @@ Include as much of the build output as you can without disclosing anything confi modules = modules || {}; log_verbose('manifest file', modulesManifest); log_verbose('manifest contents', JSON.stringify({ workspace, bin, root, modules }, null, 2)); - // Bazel starts actions with pwd=execroot/my_wksp - const workspaceDir = path.resolve('.'); - // resolveRoot will change the cwd when under runfiles + // NB: resolveRoot will change the cwd when under runfiles to the runfiles root const rootDir = yield resolveRoot(root, runfiles); - log_verbose('resolved root', root, 'to', rootDir); + log_verbose('resolved node_modules root', root, 'to', rootDir); log_verbose('cwd', process.cwd()); - // Create the $pwd/node_modules directory that node will resolve from + // Create the node_modules symlink to the node_modules root that node will resolve from yield symlink(rootDir, 'node_modules'); + // Change directory to the node_modules root directory so that all subsequent + // symlinks will be created under node_modules process.chdir(rootDir); - // Symlinks to packages need to reach back to the workspace/runfiles directory - const workspaceAbs = path.resolve(workspaceDir); function linkModules(m) { return __awaiter(this, void 0, void 0, function* () { // ensure the parent directory exist @@ -454,7 +445,7 @@ Include as much of the build output as you can without disclosing anything confi switch (root) { case 'execroot': if (runfiles.execroot) { - target = path.posix.join(workspaceAbs, modulePath); + target = path.posix.join(runfiles.workspaceDir, modulePath); } else { // If under runfiles, convert from execroot path to runfiles path. @@ -470,7 +461,7 @@ Include as much of the build output as you can without disclosing anything confi if (runfilesPath.startsWith(externalPrefix)) { runfilesPath = `../${runfilesPath.slice(externalPrefix.length)}`; } - target = path.posix.join(workspaceAbs, runfilesPath); + target = path.posix.join(runfiles.workspaceDir, runfilesPath); } break; case 'runfiles': diff --git a/internal/linker/link_node_modules.ts b/internal/linker/link_node_modules.ts index 06b546a83a..6391a0e4ea 100644 --- a/internal/linker/link_node_modules.ts +++ b/internal/linker/link_node_modules.ts @@ -131,10 +131,8 @@ export class Runfiles { manifest: Map|undefined; dir: string|undefined; execroot: boolean; - /** - * If the environment gives us enough hints, we can know the workspace name - */ - workspace: string|undefined; + workspace: string; + workspaceDir: string; /** * If the environment gives us enough hints, we can know the package path */ @@ -166,10 +164,9 @@ export class Runfiles { If you want to test runfiles manifest behavior, add --spawn_strategy=standalone to the command line.`); } - const wksp = env['BAZEL_WORKSPACE']; - if (!!wksp) { - this.workspace = wksp; - } + // Bazel starts actions with pwd=execroot/my_wksp + this.workspaceDir = path.resolve('.'); + this.workspace = path.basename(this.workspaceDir); // If target is from an external workspace such as @npm//rollup/bin:rollup // resolvePackageRelative is not supported since package is in an external // workspace. @@ -239,18 +236,13 @@ export class Runfiles { } resolveWorkspaceRelative(modulePath: string) { - if (!this.workspace) { - throw new Error('workspace could not be determined from the environment'); - } return this.resolve(path.posix.join(this.workspace, modulePath)); } resolvePackageRelative(modulePath: string) { - if (!this.workspace) { - throw new Error('workspace could not be determined from the environment'); - } if (!this.package) { - throw new Error('package could not be determined from the environment'); + throw new Error( + 'package could not be determined from the environment; make sure BAZEL_TARGET is set'); } return this.resolve(path.posix.join(this.workspace, this.package, modulePath)); } @@ -493,20 +485,17 @@ export async function main(args: string[], runfiles: Runfiles) { log_verbose('manifest file', modulesManifest); log_verbose('manifest contents', JSON.stringify({workspace, bin, root, modules}, null, 2)); - // Bazel starts actions with pwd=execroot/my_wksp - const workspaceDir = path.resolve('.'); - - // resolveRoot will change the cwd when under runfiles + // NB: resolveRoot will change the cwd when under runfiles to the runfiles root const rootDir = await resolveRoot(root, runfiles); - log_verbose('resolved root', root, 'to', rootDir); + log_verbose('resolved node_modules root', root, 'to', rootDir); log_verbose('cwd', process.cwd()); - // Create the $pwd/node_modules directory that node will resolve from + // Create the node_modules symlink to the node_modules root that node will resolve from await symlink(rootDir, 'node_modules'); - process.chdir(rootDir); - // Symlinks to packages need to reach back to the workspace/runfiles directory - const workspaceAbs = path.resolve(workspaceDir); + // Change directory to the node_modules root directory so that all subsequent + // symlinks will be created under node_modules + process.chdir(rootDir); async function linkModules(m: LinkerTreeElement) { // ensure the parent directory exist @@ -519,7 +508,7 @@ export async function main(args: string[], runfiles: Runfiles) { switch (root) { case 'execroot': if (runfiles.execroot) { - target = path.posix.join(workspaceAbs, modulePath); + target = path.posix.join(runfiles.workspaceDir, modulePath); } else { // If under runfiles, convert from execroot path to runfiles path. // First strip the bin portion if it exists: @@ -533,7 +522,7 @@ export async function main(args: string[], runfiles: Runfiles) { if (runfilesPath.startsWith(externalPrefix)) { runfilesPath = `../${runfilesPath.slice(externalPrefix.length)}`; } - target = path.posix.join(workspaceAbs, runfilesPath); + target = path.posix.join(runfiles.workspaceDir, runfilesPath); } break; case 'runfiles': diff --git a/internal/node/node.bzl b/internal/node/node.bzl index 4912647dd0..3aee90702a 100644 --- a/internal/node/node.bzl +++ b/internal/node/node.bzl @@ -179,7 +179,6 @@ def _nodejs_binary_impl(ctx): _write_loader_script(ctx) env_vars = "export BAZEL_TARGET=%s\n" % ctx.label - env_vars += "export BAZEL_WORKSPACE=%s\n" % ctx.workspace_name for k in ctx.attr.configuration_env_vars + ctx.attr.default_env_vars: # Check ctx.var first & if env var not in there then check # ctx.configuration.default_shell_env. The former will contain values from --define=FOO=BAR diff --git a/third_party/github.com/bazelbuild/bazel/tools/bash/runfiles/runfiles.bash b/third_party/github.com/bazelbuild/bazel/tools/bash/runfiles/runfiles.bash index a7fbb21584..3837231381 100644 --- a/third_party/github.com/bazelbuild/bazel/tools/bash/runfiles/runfiles.bash +++ b/third_party/github.com/bazelbuild/bazel/tools/bash/runfiles/runfiles.bash @@ -135,9 +135,12 @@ function rlocation() { return 1 else # --- begin rules_nodejs custom code --- - # Normalize ${BAZEL_WORKSPACE}/$1. + # Bazel always sets the PWD to execroot/my_wksp so we derive the workspace + # from the PWD. + export bazel_workspace=$(basename $PWD) + # Normalize ${bazel_workspace}/$1. # If $1 is a $(rootpath) this will convert it to the runfiles manifest path - readonly from_rootpath=$(normpath ${BAZEL_WORKSPACE:-/dev/null}/$1) + readonly from_rootpath=$(normpath ${bazel_workspace}/$1) # --- end rules_nodejs custom code --- if [[ -e "${RUNFILES_DIR:-/dev/null}/$1" ]]; then if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then @@ -148,7 +151,7 @@ function rlocation() { # If $1 is a rootpath then check if the converted rootpath to runfiles manifest path file is found elif [[ -e "${RUNFILES_DIR:-/dev/null}/${from_rootpath}" ]]; then if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then - echo >&2 "INFO[runfiles.bash]: rlocation($1): found under RUNFILES_DIR/BAZEL_WORKSPACE ($RUNFILES_DIR/$BAZEL_WORKSPACE), return" + echo >&2 "INFO[runfiles.bash]: rlocation($1): found under RUNFILES_DIR/WKSP ($RUNFILES_DIR/$bazel_workspace), return" fi echo "${RUNFILES_DIR}/${from_rootpath}" # --- end rules_nodejs custom code ---