feat(builtin): add configuration_env_vars to npm_package_bin

.bazelrc

@@ -24,6 +24,7 @@ build --noincompatible_no_support_tools_in_action_inputs
 # Define environment value used by some tests such as //internal/npm_install/test:bazel_bin_test
 build --define=SOME_TEST_ENV=some_value
 test --define=SOME_TEST_ENV=some_value
+build --action_env=SOME_OTHER_ENV=some_other_value
 
 ###############################
 # Remote Build Execution support

.circleci/config.yml

@@ -10,7 +10,7 @@
 ## IMPORTANT
 # If you change the `default_docker_image` version, also change the `cache_key` version
 var_1: &default_docker_image circleci/node:10.16
-var_3: &cache_key node-0.16-{{ checksum "yarn.lock" }}
+var_3: &cache_key node-0.16-{{ checksum "yarn.lock" }}-v2
 
 var_4: &init_environment
   run:

internal/node/node.bzl

@@ -178,8 +178,13 @@ def _nodejs_binary_impl(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
+        # and latter will contain values from --action_env=FOO=BAR (but not from --action_env=FOO).
         if k in ctx.var.keys():
             env_vars += "export %s=\"%s\"\n" % (k, ctx.var[k])
+        elif k in ctx.configuration.default_shell_env.keys():
+            env_vars += "export %s=\"%s\"\n" % (k, ctx.configuration.default_shell_env[k])
 
     expected_exit_code = 0
     if hasattr(ctx.attr, "expected_exit_code"):

internal/node/npm_package_bin.bzl

@@ -9,6 +9,7 @@ load("//internal/linker:link_node_modules.bzl", "module_mappings_aspect")
 _ATTRS = {
     "outs": attr.output_list(),
     "args": attr.string_list(mandatory = True),
+    "configuration_env_vars": attr.string_list(default = []),
     "data": attr.label_list(allow_files = True, aspects = [module_mappings_aspect, node_modules_aspect]),
     "output_dir": attr.bool(),
     "tool": attr.label(
@@ -57,6 +58,7 @@ def _impl(ctx):
         inputs = inputs,
         outputs = outputs,
         arguments = [args],
+        configuration_env_vars = ctx.attr.configuration_env_vars,
     )
     return [DefaultInfo(files = depset(outputs))]
 

internal/node/test/BUILD.bazel

@@ -70,7 +70,10 @@ nodejs_binary(
 
 nodejs_test(
     name = "define_var",
-    configuration_env_vars = ["SOME_TEST_ENV"],
+    configuration_env_vars = [
+        "SOME_TEST_ENV",
+        "SOME_OTHER_ENV",
+    ],
     data = glob(["*.spec.js"]),
     entry_point = ":define.spec.js",
 )
@@ -271,6 +274,7 @@ npm_package_bin(
         "$@",
         "$(SOME_TEST_ENV)",
     ],
+    configuration_env_vars = ["SOME_OTHER_ENV"],
     tool = ":expand_variables",
 )
 

internal/node/test/define.spec.js

@@ -6,3 +6,11 @@ if (process.env['SOME_TEST_ENV'] !== 'some_value') {
   console.error('should accept vars; must be run with --define=SOME_TEST_ENV=some_value');
   process.exitCode = 1;
 }
+
+// Similar to above but testing another env variable that is expected to be specified with
+// `--action_env=SOME_OTHER_ENV=some_other_value`.
+if (process.env['SOME_OTHER_ENV'] !== 'some_other_value') {
+  console.error(
+      'should accept vars; must be run with --action_env=SOME_OTHER_ENV=some_other_value');
+  process.exitCode = 1;
+}

internal/node/test/expand_variables.golden

@@ -1,3 +1,4 @@
 [
-  "some_value"
+  "some_value",
+  "some_other_value"
 ]

internal/node/test/expand_variables.js

@@ -1,4 +1,5 @@
 const fs = require('fs');
 const args = process.argv.slice(2);
 const outfile = args.shift();
+args.push(process.env['SOME_OTHER_ENV']);
 fs.writeFileSync(outfile, JSON.stringify(args, null, 2), 'utf-8');

internal/npm_install/test/bazel_bin_test.sh

@@ -12,8 +12,9 @@ source "${RUNFILES_DIR:-/dev/null}/$f" 2>/dev/null || \
 # --- end runfiles.bash initialization v2 ---
 
 readonly OUT=$($(rlocation "npm/testy/bin/testy.sh"))
+readonly EXPECTED="Hello some_value && some_other_value"
 
-if [ "$OUT" != "Hello some_value" ]; then
-  echo "Expected output 'Hello world' but was '$OUT'"
+if [ "${OUT}" != "${EXPECTED}" ]; then
+  echo "Expected output '${EXPECTED}' but was '${OUT}'"
   exit 1
 fi

internal/providers/node_runtime_deps_info.bzl

@@ -63,10 +63,20 @@ def run_node(ctx, inputs, arguments, executable, **kwargs):
     # To access runfiles, you must use a runfiles helper in the program instead
     add_arg(arguments, "--nobazel_patch_module_resolver")
 
-    # Forward the COMPILATION_MODE to node process as an environment variable
     env = kwargs.pop("env", {})
-    if "COMPILATION_MODE" not in env.keys():
-        env["COMPILATION_MODE"] = ctx.var["COMPILATION_MODE"]
+
+    # Always forward the COMPILATION_MODE to node process as an environment variable
+    configuration_env_vars = kwargs.pop("configuration_env_vars", []) + ["COMPILATION_MODE"]
+    for var in configuration_env_vars:
+        if var not in env.keys():
+            # If env is not explicitely specified, 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 and latter will contain values from --action_env=FOO=BAR
+            # (but not from --action_env=FOO).
+            if var in ctx.var.keys():
+                env[var] = ctx.var[var]
+            elif var in ctx.configuration.default_shell_env.keys():
+                env[var] = ctx.configuration.default_shell_env[var]
 
     ctx.actions.run(
         inputs = inputs + extra_inputs,

tools/npm_packages/testy/index.js

@@ -1 +1 @@
-console.log(`Hello ${process.env['SOME_TEST_ENV']}`);
+console.log(`Hello ${process.env['SOME_TEST_ENV']} && ${process.env['SOME_OTHER_ENV']}`);

tools/npm_packages/testy/package.json

@@ -7,7 +7,7 @@
   "bazelBin": {
     "testy": {
       "additionalAttributes": {
-        "configuration_env_vars": "[\"SOME_TEST_ENV\"]"
+        "configuration_env_vars": "[\"SOME_TEST_ENV\", \"SOME_OTHER_ENV\"]"
       }
     }
   }