From 46dde62c54cbe04687776dd3ab153a84bd69929f Mon Sep 17 00:00:00 2001
From: Derek Cormier <derek@aspect.dev>
Date: Fri, 25 Feb 2022 11:01:11 -0800
Subject: [PATCH] feat(builtin): perform make variable substitution in
 npm_package_bin env vars

---
 docs/Built-ins.md                           |  3 ++-
 internal/node/npm_package_bin.bzl           |  9 +++++++--
 internal/node/test/BUILD.bazel              | 10 ++++++++++
 internal/node/test/expand_variables.js      | 16 ++++++++++++++--
 internal/node/test/expand_variables.spec.js | 15 +++++++++++++--
 5 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/docs/Built-ins.md b/docs/Built-ins.md
index 843903689c..6b1be56eb4 100755
--- a/docs/Built-ins.md
+++ b/docs/Built-ins.md
@@ -1907,7 +1907,8 @@ Defaults to `[]`
 
 <h4 id="npm_package_bin-env">env</h4>
 
-specifies additional environment variables to set when the target is executed
+specifies additional environment variables to set when the target is executed. The values of environment variables
+are subject to 'Make variable' substitution (see [args](#npm_package_bin-args)).
 
 Defaults to `{}`
 
diff --git a/internal/node/npm_package_bin.bzl b/internal/node/npm_package_bin.bzl
index 49d0cd80e1..3d7d0847c5 100644
--- a/internal/node/npm_package_bin.bzl
+++ b/internal/node/npm_package_bin.bzl
@@ -65,6 +65,10 @@ def _impl(ctx):
     for a in ctx.attr.args:
         args.add_all([expand_variables(ctx, e, outs = ctx.outputs.outs, output_dir = ctx.attr.output_dir) for e in _expand_locations(ctx, a)])
 
+    envs = {}
+    for k, v in ctx.attr.env.items():
+        envs[k] = " ".join([expand_variables(ctx, e, outs = ctx.outputs.outs, output_dir = ctx.attr.output_dir, attribute_name = "env") for e in _expand_locations(ctx, v)])
+
     tool_outputs = []
     if ctx.outputs.stdout:
         tool_outputs.append(ctx.outputs.stdout)
@@ -83,7 +87,7 @@ def _impl(ctx):
         arguments = [args],
         configuration_env_vars = ctx.attr.configuration_env_vars,
         chdir = expand_variables(ctx, ctx.attr.chdir),
-        env = ctx.attr.env,
+        env = envs,
         stdout = ctx.outputs.stdout,
         stderr = ctx.outputs.stderr,
         exit_code_out = ctx.outputs.exit_code_out,
@@ -213,7 +217,8 @@ def npm_package_bin(
                 args = ["/".join([".."] * _package_segments + ["$@"])],
             )
             ```
-        env: specifies additional environment variables to set when the target is executed
+        env: specifies additional environment variables to set when the target is executed. The values of environment variables
+            are subject to 'Make variable' substitution (see [args](#npm_package_bin-args)).
         **kwargs: additional undocumented keyword args
     """
     if not tool:
diff --git a/internal/node/test/BUILD.bazel b/internal/node/test/BUILD.bazel
index 79ce3dc20f..5b2195cb5c 100644
--- a/internal/node/test/BUILD.bazel
+++ b/internal/node/test/BUILD.bazel
@@ -359,6 +359,15 @@ npm_package_bin(
         "somearg$$",
         "some0arg",
     ],
+    env = {
+        "OUTFILE": "$@",
+        "COMPLATION_MODE": "$(COMPILATION_MODE)",
+        "TARGET_CPU": "$(TARGET_CPU)",
+        "BINDIR": "$(BINDIR)",
+        "SOME_TEST_ENV": "$(SOME_TEST_ENV)",
+        "SOMEARG$$": "somearg$$",
+        "SOME0ARG": "some0arg",
+    },
     tool = ":expand_variables",
 )
 
@@ -368,6 +377,7 @@ jasmine_node_test(
     data = [":expand_variables.out"],
     templated_args = [
         "$(rootpath :expand_variables.out)",
+        "$(execpath :expand_variables.out)",
         "$(COMPILATION_MODE)",
         "$(TARGET_CPU)",
         "$(BINDIR)",
diff --git a/internal/node/test/expand_variables.js b/internal/node/test/expand_variables.js
index 99754db7ca..4f11dcccbd 100644
--- a/internal/node/test/expand_variables.js
+++ b/internal/node/test/expand_variables.js
@@ -1,4 +1,16 @@
 const fs = require('fs');
 const args = process.argv.slice(2);
-const outfile = args.shift();
-fs.writeFileSync(outfile, JSON.stringify(args, null, 2), 'utf-8');
+const outfile = args[0];
+const dump = {
+    args,
+    env: {
+        OUTFILE: process.env.OUTFILE,
+        COMPLATION_MODE: process.env.COMPLATION_MODE,
+        TARGET_CPU: process.env.TARGET_CPU,
+        BINDIR: process.env.BINDIR,
+        SOME_TEST_ENV: process.env.SOME_TEST_ENV,
+        SOMEARG$$: process.env.SOMEARG$$,
+        SOME0ARG: process.env.SOME0ARG,
+    }
+}
+fs.writeFileSync(outfile, JSON.stringify(dump, null, 2), 'utf-8');
diff --git a/internal/node/test/expand_variables.spec.js b/internal/node/test/expand_variables.spec.js
index 8a0a641e51..623241ae45 100644
--- a/internal/node/test/expand_variables.spec.js
+++ b/internal/node/test/expand_variables.spec.js
@@ -20,10 +20,21 @@ const runfiles = require(process.env['BAZEL_NODE_RUNFILES_HELPER']);
 const args = process.argv.slice(2);
 const out = JSON.parse(
     require('fs').readFileSync(runfiles.resolveWorkspaceRelative(args.shift()), 'utf-8'));
-const expected = args;
+const expected_args = args;
 
 describe('nodejs_test templated_args variable expansion', function() {
   it('should match variable expansion in npm_package_bin args', function() {
-    expect(out).toEqual(expected);
+    expect(out.args).toEqual(expected_args);
+  });
+  it('should match variable expansion in npm_package_bin env vars', function() {
+    expect(out.env).toEqual({
+      OUTFILE: expected_args[0],
+      COMPLATION_MODE: expected_args[1],
+      TARGET_CPU: expected_args[2],
+      BINDIR: expected_args[3],
+      SOME_TEST_ENV: expected_args[4],
+      SOMEARG$$: expected_args[5],
+      SOME0ARG: expected_args[6],
+    })
   });
 });