From d70d03b30e32b5499e0cc3a5fceaf1891350588b Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 2 Dec 2022 13:59:42 +0000 Subject: [PATCH] fix(builtin): entry point from sources used when used as tool Consider a `nodejs_binary` with a definition like the followed: ```bzl js_library( name = "lib", srcs = ["entrypoint.js", "constants.js"], ) nodejs_binary( name = "bin", entry_point = "entrypoint.js", data = [":lib"], ) ``` This seems very standanrd and also works perfectly when the binary is invoked with `bazel run`. Via Bazel run, the Node Bash launcher resolves the entrypoint via the actual runfiles using `rlocation`. If this binary is used as a tool, e.g. in `npm_package_bin`- then the entry point is resolved from the execroot, landing ultimately on the *actual source file*. This is wrong and breaks resolution in RBE (where only necessary sources are in the execroot). This happens because the source entrypoint file. i.e. not the `entrypoint.js` from `bazel-bin` is also ending up being included in the runfiles of `run_node` via `NodeRuntimeDepsInfo`. This mismatch breaks resolution, and also results in an incorrect/ unnecessary file being added to the action inputs. The entry point used in `NodeRuntimeDepsInfo` should be the one derived from the `data` sources of the rule, ensuring the entry-point can access its other files of the `js_library`. i.e. entry-point should come from the `data` preferred, and if it's not found- then the source, or `File` can be directly used. This fixes RBE for angular.io which started unveiling some issues when we attmepted to enable RBE via: https://github.com/angular/angular/pull/48316 --- internal/node/node.bzl | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/internal/node/node.bzl b/internal/node/node.bzl index 18d7ce432a..3b8ce747ea 100644 --- a/internal/node/node.bzl +++ b/internal/node/node.bzl @@ -353,10 +353,22 @@ if (process.cwd() !== __dirname) { else: executable = ctx.outputs.launcher_sh - # syntax sugar: allows you to avoid repeating the entry point in data - # entry point is only needed in runfiles if it is a javascript file - if len(ctx.files.entry_point) == 1 and is_javascript_file(ctx.files.entry_point[0]): - runfiles.extend(ctx.files.entry_point) + # Note: `to_list()` is expensive and should only be called once. + sources_list = sources.to_list() + entry_point_input_short_path = _ts_to_js(_get_entry_point_file(ctx).short_path) + entry_point_script = None + + for f in sources_list: + if f.short_path == entry_point_input_short_path: + entry_point_script = f + break + + if not entry_point_script and len(ctx.files.entry_point) == 1 and is_javascript_file(ctx.files.entry_point[0]): + entry_point_script = ctx.files.entry_point[0] + + # Convenience: We add the entry point to the runfiles. This means that users would not + # need to explicitly repeat the entry point in the `data` attribute. + runfiles.append(entry_point_script) return [ DefaultInfo( @@ -371,14 +383,14 @@ if (process.cwd() !== __dirname) { # Calling the .to_list() method may have some perfs hits, # so we should be running this method only once per rule. # see: https://docs.bazel.build/versions/main/skylark/depsets.html#performance - node_modules.to_list() + sources.to_list(), + node_modules.to_list() + sources_list, collect_data = True, ), ), # TODO(alexeagle): remove sources and node_modules from the runfiles # when downstream usage is ready to rely on linker NodeRuntimeDepsInfo( - deps = depset(ctx.files.entry_point, transitive = [node_modules, sources]), + deps = depset([entry_point_script], transitive = [node_modules, sources]), pkgs = data, ), # indicates that the this binary should be instrumented by coverage