feat(builtin): support $(rootpath), $(execpath), predefined & custom …

…variables in templated_args

* patch runfiles.bash helper to support $(rootpath)

* add support for $(rootpath), $(execpath), predefined & custom variables to expand_location_into_runfiles helper function

This applies to `templated_args` in nodejs_binary, nodejs_test & jasmine_node_test and to `args` in params_file

Also fixes ambiguity of `$(rlocation $(location ...))` expansion as it is not clear that `$(rlocation ...)` is not meant to be expanded but rather passed down to the shell script for execution. This is now more clear with `$$(rlocation $(manifestpath ...))` which follows the Bazel "Make" variable expansion convention. For example, `templated_args = ["--node_options=--require=$$(rlocation $(manifestpath :jasmine_shared_env_bootstrap.js))",],`.

`templated_args` in nodejs_binary, nodejs_test & jasmine_node_test is now…

Subject to 'Make variable' substitution. See https://docs.bazel.build/versions/master/be/make-variables.html.

1. Predefined source/output path substitions is applied first:

Expands all `$(execpath ...)`, `$(rootpath ...)` and legacy `$(location ...)` templates in the
given string by replacing with the expanded path. Expansion only works for labels that point to direct dependencies
of this rule or that are explicitly listed in the optional argument targets.

See https://docs.bazel.build/versions/master/be/make-variables.html#predefined_label_variables.

Use `$(rootpath)` and `$(rootpaths)` to expand labels to the runfiles path that a built binary can use
to find its dependencies. This path is of the format:
- `./file`
- `path/to/file`
- `../external_repo/path/to/file`

Use `$(execpath)` and `$(execpaths)` to expand labels to the execroot (where Bazel runs build actions).
This is of the format:
- `./file`
- `path/to/file`
- `external/external_repo/path/to/file`
- `<bin_dir>/path/to/file`
- `<bin_dir>/external/external_repo/path/to/file`

The legacy `$(location)` and `$(locations)` expansion is DEPRECATED as it returns the runfiles manifest path of the
format `repo/path/to/file` which behaves differently than the built-in `$(location)` expansion in args of *_binary
and *_test rules which returns the rootpath.
See https://docs.bazel.build/versions/master/be/common-definitions.html#common-attributes-binaries.

The legacy `$(location)` and `$(locations)` expansion also differs from how the builtin `ctx.expand_location()` expansions
of `$(location)` and `$(locations)` behave as that function returns either the execpath or rootpath depending on the context.
See https://docs.bazel.build/versions/master/be/make-variables.html#predefined_label_variables.

The behavior of `$(location)` and `$(locations)` expansion may change in the future with support either being removed
entirely or the expansion changed to return the same path as `ctx.expand_location()` returns for these.

The recommended approach is to now use `$(rootpath)` where you previously used $(location).

To get from a `$(rootpath)` to the absolute path that `$$(rlocation $(location))` returned you can either use
`$$(rlocation $(rootpath))` if you are in the `templated_args` of a `nodejs_binary` or `nodejs_test`:

BUILD.bazel:
```
nodejs_test(
    name = "my_test",
    data = [":bootstrap.js"],
    templated_args = ["--node_options=--require=$$(rlocation $(rootpath :bootstrap.js))"],
)
```

or if you're in the context of a .js script you can pass the $(rootpath) as an argument to the script
and use the javascript runfiles helper to resolve to the absolute path:

BUILD.bazel:
```
nodejs_test(
    name = "my_test",
    data = [":some_file"],
    entry_point = ":my_test.js",
    templated_args = ["$(rootpath :some_file)"],
)
```

my_test.js
```
const runfiles = require(process.env['BAZEL_NODE_RUNFILES_HELPER']);
const args = process.argv.slice(2);
const some_file = runfiles.resolveWorkspaceRelative(args[0]);
```

NB: Bazel will error if it sees the single dollar sign $(rlocation path) in `templated_args` as it will try to
example `$(rlocation)` since we now expand predefined & custom "make" variables such as `$(COMPILATION_MODE)`,
`$(BINDIR)` & `$(TARGET_CPU)` using `ctx.expand_make_variables`. See https://docs.bazel.build/versions/master/be/make-variables.html.

To prevent expansion of `$(rlocation)` write it as `$$(rlocation)`. Bazel understands `$$` to be
the string literal `$` and the expansion results in `$(rlocation)` being passed as an arg instead
of being expanded. `$(rlocation)` is then evaluated by the bash node launcher script and it calls
the `rlocation` function in the runfiles.bash helper. For example, the templated arg
`$$(rlocation $(rootpath //:some_file))` is expanded by Bazel to `$(rlocation ./some_file)` which
is then converted in bash to the absolute path of `//:some_file` in runfiles by the runfiles.bash helper
before being passed as an argument to the program

2. Predefined variables & Custom variables are expanded second:

Predefined "Make" variables such as $(COMPILATION_MODE) and $(TARGET_CPU) are expanded.
See https://docs.bazel.build/versions/master/be/make-variables.html#predefined_variables.

Custom variables are also expanded including variables set through the Bazel CLI with --define=SOME_VAR=SOME_VALUE.
See https://docs.bazel.build/versions/master/be/make-variables.html#custom_variables.

Predefined genrule variables are not supported in this context.

e2e/jasmine/BUILD.bazel

@@ -10,7 +10,7 @@ jasmine_node_test(
     srcs = ["jasmine_shared_env_test.spec.js"],
     data = ["jasmine_shared_env_bootstrap.js"],
     templated_args = [
-        "--node_options=--require=$(rlocation $(location :jasmine_shared_env_bootstrap.js))",
+        "--node_options=--require=$$(rlocation $(rootpath :jasmine_shared_env_bootstrap.js))",
     ],
     deps = [
         "@npm//jasmine",

examples/angular/src/BUILD.bazel

@@ -209,7 +209,7 @@ history_server(
     #   /        => src/prodapp
     templated_args = [
         "-a",
-        "$(rlocation examples_angular/src/prodapp)",
+        "$$(rlocation examples_angular/src/prodapp)",
     ],
 )
 

examples/angular_view_engine/src/BUILD.bazel

@@ -217,6 +217,6 @@ history_server(
     #   /        => src/prodapp
     templated_args = [
         "-a",
-        "$(rlocation examples_angular_view_engine/src/prodapp)",
+        "$$(rlocation examples_angular_view_engine/src/prodapp)",
     ],
 )

examples/jest/jest.bzl

@@ -4,16 +4,18 @@ load("@npm//jest-cli:index.bzl", _jest_test = "jest_test")
 
 def jest_test(name, srcs, deps, jest_config, **kwargs):
     "A macro around the autogenerated jest_test rule"
-    args = [
+    templated_args = [
         "--no-cache",
         "--no-watchman",
         "--ci",
     ]
-    args.extend(["--config", "$(location %s)" % jest_config])
+    templated_args.extend(["--config", "$(rootpath %s)" % jest_config])
+    for src in srcs:
+        templated_args.extend(["--runTestsByPath", "$(rootpath %s)" % src])
 
     _jest_test(
         name = name,
         data = [jest_config] + srcs + deps,
-        args = args,
+        templated_args = templated_args,
         **kwargs
     )

internal/common/expand_into_runfiles.bzl

@@ -15,48 +15,62 @@
 """Helper functions to expand paths into runfiles
 """
 
-# Expand $(location) and $(locations) to runfiles manifest path
-def _expand_mlocations(ctx, input, targets):
+# Expand $(rootpath) and $(rootpaths) to runfiles manifest path.
+# Runfiles manifest path is of the form:
+# - repo/path/to/file
+def _expand_rootpath_to_manifest_path(ctx, input, targets):
     paths = ctx.expand_location(input, targets)
-    return " ".join([_short_path_to_runfiles_manifest_path(ctx, p, targets) for p in paths.split(" ")])
+    return " ".join([_rootpath_to_runfiles_manifest_path(ctx, p, targets) for p in paths.split(" ")])
 
-# Convert a short_path in the execroot to the runfiles manifest path
-def _short_path_to_runfiles_manifest_path(ctx, path, targets):
+# Convert an runfiles rootpath to a runfiles manifestpath.
+# Runfiles rootpath is returned from ctx.expand_location $(rootpath) and $(rootpaths):
+# - ./file
+# - path/to/file
+# - ../external_repo/path/to/file
+# This is converted to the runfiles manifest path of:
+# - repo/path/to/file
+def _rootpath_to_runfiles_manifest_path(ctx, path, targets):
     if path.startswith("../"):
         return path[len("../"):]
     if path.startswith("./"):
         path = path[len("./"):]
-    elif path.startswith(ctx.bin_dir.path):
-        path = path[len(ctx.bin_dir.path + "/"):]
-    elif path.startswith(ctx.genfiles_dir.path):
-        path = path[len(ctx.genfiles_dir.path + "/"):]
     return ctx.workspace_name + "/" + path
 
-# Expand $(location) and $(locations) to runfiles short path
-def _expand_locations(ctx, input, targets):
-    paths = ctx.expand_location(input, targets)
-    return " ".join([_short_path_to_runfiles_short_path(ctx, p, targets) for p in paths.split(" ")])
-
-# Convert a short_path in the execroot to the runfiles short path
-def _short_path_to_runfiles_short_path(ctx, path, targets):
-    path = path.replace(ctx.bin_dir.path + "/external/", "../", 1)
-    path = path.replace(ctx.bin_dir.path + "/", "", 1)
-    path = path.replace(ctx.genfiles_dir.path + "/external/", "../", 1)
-    path = path.replace(ctx.genfiles_dir.path + "/", "", 1)
-    return path
-
 def expand_location_into_runfiles(ctx, input, targets = []):
-    """Expands all $(location ...) templates in the given string by replacing $(location //x) with the path
-    in runfiles of the output file of target //x. Expansion only works for labels that point to direct dependencies
+    """Expands all `$(execpath ...)`, `$(rootpath ...)` and legacy `$(location ...)` templates in the
+    given string by replacing with the expanded path. Expansion only works for labels that point to direct dependencies
     of this rule or that are explicitly listed in the optional argument targets.
 
-    Path is returned in runfiles manifest path format such as `repo/path/to/file`. This differs from how $(location)
-    and $(locations) expansion behaves in expansion the `args` attribute of a *_binary or *_test which returns
-    the runfiles short path of the format `./path/to/file` for user repo and `../external_repo/path/to/file` for external
-    repositories.
+    See https://docs.bazel.build/versions/master/be/make-variables.html#predefined_label_variables.
+
+    Use `$(rootpath)` and `$(rootpaths)` to expand labels to the runfiles path that a built binary can use
+    to find its dependencies. This path is of the format:
+    - `./file`
+    - `path/to/file`
+    - `../external_repo/path/to/file`
+
+    Use `$(execpath)` and `$(execpaths)` to expand labels to the execroot (where Bazel runs build actions).
+    This is of the format:
+    - `./file`
+    - `path/to/file`
+    - `external/external_repo/path/to/file`
+    - `<bin_dir>/path/to/file`
+    - `<bin_dir>/external/external_repo/path/to/file`
 
-    This will be fixed in a future release major release as well as adding support for $(execpath) and $(rootpath)
-    substitions: https://docs.bazel.build/versions/master/be/make-variables.html#predefined_label_variables.
+    The legacy `$(location)` and `$(locations)` expansion is DEPRECATED as it returns the runfiles manifest path of the
+    format `repo/path/to/file` which behaves differently than the built-in `$(location)` expansion in args of *_binary
+    and *_test rules which returns the rootpath.
+    See https://docs.bazel.build/versions/master/be/common-definitions.html#common-attributes-binaries.
+
+    The legacy `$(location)` and `$(locations)` expansion also differs from how the builtin `ctx.expand_location()` expansions
+    of `$(location)` and `$(locations)` behave as that function returns either the execpath or rootpath depending on the context.
+    See https://docs.bazel.build/versions/master/be/make-variables.html#predefined_label_variables.
+
+    The behavior of `$(location)` and `$(locations)` expansion will be fixed in a future major release to match the
+    to default Bazel behavior and return the same path as `ctx.expand_location()` returns for these.
+
+    The recommended approach is to now use `$(rootpath)` where you previously used $(location). See the docstrings
+    of `nodejs_binary` or `params_file` for examples of how to use `$(rootpath)` in `templated_args` and `args` respectively.
 
     Args:
       ctx: context
@@ -68,28 +82,32 @@ def expand_location_into_runfiles(ctx, input, targets = []):
     """
     target = "@%s//%s:%s" % (ctx.workspace_name, "/".join(ctx.build_file_path.split("/")[:-1]), ctx.attr.name)
 
-    # Loop through input an expand all $(location) and $(locations) using _expand_to_mlocation()
+    # Loop through input an expand all predefined source/output path variables
+    # See https://docs.bazel.build/versions/master/be/make-variables.html#predefined_label_variables.
     path = ""
     length = len(input)
     last = 0
     for i in range(length):
-        if (input[i:i + 12] == "$(mlocation ") or (input[i:i + 13] == "$(mlocations "):
+        # Support legacy $(location) and $(locations) expansions which return the runfiles manifest path
+        # in the format `repo/path/to/file`. This expansion is DEPRECATED. See docstring above.
+        # TODO: Change location to behave the same as the built-in $(location) expansion for args of *_binary
+        #       and *_test rules. This would be a BREAKING CHANGE.
+        if input[i:].startswith("$(location ") or input[i:].startswith("$(locations "):
             j = input.find(")", i) + 1
             if (j == 0):
-                fail("invalid $(mlocation) expansion in string \"%s\" part of target %s" % (input, target))
+                fail("invalid \"%s\" expansion in string \"%s\" part of target %s" % (input[i:j], input, target))
             path += input[last:i]
-            path += _expand_mlocations(ctx, "$(" + input[i + 3:j], targets)
+            path += _expand_rootpath_to_manifest_path(ctx, "$(rootpath" + input[i + 10:j], targets)
             last = j
             i = j
-        if (input[i:i + 11] == "$(location ") or (input[i:i + 12] == "$(locations "):
+
+        # Expand $(execpath) $(execpaths) $(rootpath) $(rootpaths) with plain ctx.expand_location()
+        if input[i:].startswith("$(execpath ") or input[i:].startswith("$(execpaths ") or input[i:].startswith("$(rootpath ") or input[i:].startswith("$(rootpaths "):
             j = input.find(")", i) + 1
             if (j == 0):
-                fail("invalid $(location) expansion in string \"%s\" part of target %s" % (input, target))
+                fail("invalid \"%s\" expansion in string \"%s\" part of target %s" % (input[i:j], input, target))
             path += input[last:i]
-
-            # TODO(gmagolan): flip to _expand_locations in the future so $(location) expands to runfiles short
-            # path which is more Bazel idiomatic and $(mlocation) can be used for runfiles manifest path
-            path += _expand_mlocations(ctx, input[i:j], targets)
+            path += ctx.expand_location(input[i:j], targets)
             last = j
             i = j
     path += input[last:]

internal/common/params_file.bzl

@@ -47,7 +47,7 @@ def _impl(ctx):
 
     expanded_args = []
 
-    # First expand $(location) args
+    # First expand predefined source/output path variables
     for a in ctx.attr.args:
         expanded_args += _expand_location_into_runfiles(ctx, a)
 
@@ -87,17 +87,27 @@ def params_file(
         out: Path of the output file, relative to this package.
         args: Arguments to concatenate into a params file.
 
-            1. Subject to $(location) substitutions.
+            Subject to 'Make variable' substitution. See https://docs.bazel.build/versions/master/be/make-variables.html.
 
-            NB: This substition returns the manifest file path which differs from the *_binary & *_test
+            1. Subject to predefined source/output path variables substitutions.
+
+            The predefined variables `execpath`, `execpaths`, `rootpath`, `rootpaths`, `location`, and `locations` take
+            label parameters (e.g. `$(execpath //foo:bar)`) and substitute the file paths denoted by that label.
+
+            See https://docs.bazel.build/versions/master/be/make-variables.html#predefined_label_variables for more info.
+
+            NB: This $(location) substition returns the manifest file path which differs from the *_binary & *_test
             args and genrule bazel substitions. This will be fixed in a future major release.
-            See docs string of `expand_location_into_runfiles` macro in
-            `internal/common/expand_into_runfiles.bzl` for more info.
+            See docs string of `expand_location_into_runfiles` macro in `internal/common/expand_into_runfiles.bzl`
+            for more info.
 
             2. Subject to predefined variables & custom variable substitutions.
 
-            See https://docs.bazel.build/versions/master/be/make-variables.html#predefined_variables
-            and https://docs.bazel.build/versions/master/be/make-variables.html#custom_variables.
+            Predefined "Make" variables such as $(COMPILATION_MODE) and $(TARGET_CPU) are expanded.
+            See https://docs.bazel.build/versions/master/be/make-variables.html#predefined_variables.
+
+            Custom variables are also expanded including variables set through the Bazel CLI with --define=SOME_VAR=SOME_VALUE.
+            See https://docs.bazel.build/versions/master/be/make-variables.html#custom_variables.
 
             Predefined genrule variables are not supported in this context.
 

internal/common/test/BUILD.bazel

@@ -36,8 +36,12 @@ params_file(
     out = ":params_file.out",
     args = [
         "$(SOME_TEST_ENV)",
-        "$(location //:package.json)",
+        "$(execpaths :locations_in)",
+        "$(execpath //:package.json)",
+        "$(rootpaths :locations_in)",
+        "$(rootpath //:package.json)",
         "$(locations :locations_in)",
+        "$(location //:package.json)",
     ],
     data = [
         ":locations_in",
@@ -49,6 +53,10 @@ jasmine_node_test(
     name = "params_file_test",
     srcs = [":params_file.spec.js"],
     data = [":params_file.out"],
+    templated_args = [
+        "$(TARGET_CPU)",
+        "$(COMPILATION_MODE)",
+    ],
 )
 
 nodejs_binary(
@@ -65,3 +73,7 @@ sh_test(
     ],
     deps = ["@bazel_tools//tools/bash/runfiles"],
 )
+
+load(":expand_into_runfiles_test.bzl", "expand_into_runfiles_test_suite")
+
+expand_into_runfiles_test_suite()

internal/common/test/expand_into_runfiles_test.bzl

@@ -0,0 +1,43 @@
+"Unit tests for //internal/common:expand_into_runfiles.bzl"
+
+load("@bazel_skylib//lib:unittest.bzl", "asserts", "unittest")
+load("//internal/common:expand_into_runfiles.bzl", "expand_location_into_runfiles")
+
+def _impl(ctx):
+    env = unittest.begin(ctx)
+
+    conversions = {
+        "$(location //:package.json)": "build_bazel_rules_nodejs/package.json",
+        "$(location :a)": "build_bazel_rules_nodejs/internal/common/test/foo/bar/a.txt",
+        "$(location params_file.spec.js)": "build_bazel_rules_nodejs/internal/common/test/params_file.spec.js",
+        "$(locations :locations_in)": "build_bazel_rules_nodejs/package.json build_bazel_rules_nodejs/internal/common/test/foo/bar/a.txt build_bazel_rules_nodejs/internal/common/test/params_file.spec.js",
+        "$(rootpath //:package.json)": "./package.json",
+        "$(rootpath :a)": "internal/common/test/foo/bar/a.txt",
+        "$(rootpath params_file.spec.js)": "internal/common/test/params_file.spec.js",
+        "$(rootpaths :locations_in)": "./package.json internal/common/test/foo/bar/a.txt internal/common/test/params_file.spec.js",
+    }
+
+    for key in conversions:
+        asserts.equals(env, "%s" % conversions[key], expand_location_into_runfiles(ctx, "%s" % key))
+        asserts.equals(env, " %s " % conversions[key], expand_location_into_runfiles(ctx, " %s " % key))
+        asserts.equals(env, "%s%s" % (conversions[key], conversions[key]), expand_location_into_runfiles(ctx, "%s%s" % (key, key)))
+        asserts.equals(env, "%s %s" % (conversions[key], conversions[key]), expand_location_into_runfiles(ctx, "%s %s" % (key, key)))
+        asserts.equals(env, " %s %s " % (conversions[key], conversions[key]), expand_location_into_runfiles(ctx, " %s %s " % (key, key)))
+        asserts.equals(env, "a%sb%sc" % (conversions[key], conversions[key]), expand_location_into_runfiles(ctx, "a%sb%sc" % (key, key)))
+
+    return unittest.end(env)
+
+expand_into_runfiles_test = unittest.make(
+    impl = _impl,
+    attrs = {
+        "deps": attr.label_list(default = [
+            "//:package.json",
+            "params_file.spec.js",
+            ":a",
+            ":locations_in",
+        ], allow_files = True),
+    },
+)
+
+def expand_into_runfiles_test_suite():
+    unittest.suite("expand_into_runfiles_tests", expand_into_runfiles_test)

internal/common/test/params_file.spec.js

@@ -18,6 +18,10 @@
 
 const runfiles = require(process.env['BAZEL_NODE_RUNFILES_HELPER']);
 
+// The arguments are passed via a params file
+const TARGET_CPU = process.argv[2];
+const COMPILATION_MODE = process.argv[3];
+
 // The arguments that were outputted to the params file
 const actual = require('fs')
                    .readFileSync(runfiles.resolvePackageRelative('params_file.out'), 'utf-8')
@@ -26,12 +30,24 @@ const actual = require('fs')
 // The argument we expect to find in the params file
 const expected = [
   'some_value',
-  // $location (expands to runfiles manifest path of format repo/path/to/file)
-  'build_bazel_rules_nodejs/package.json',
+  // $execpaths
+  './package.json',
+  `bazel-out/${TARGET_CPU}-${COMPILATION_MODE}/bin/internal/common/test/foo/bar/a.txt`,
+  'internal/common/test/params_file.spec.js',
+  // $execpath
+  './package.json',
+  // $rootpaths
+  './package.json',
+  'internal/common/test/foo/bar/a.txt',
+  'internal/common/test/params_file.spec.js',
+  // $rootpath
+  './package.json',
   // $locations (expands to runfiles manifest path of format repo/path/to/file)
   'build_bazel_rules_nodejs/package.json',
   'build_bazel_rules_nodejs/internal/common/test/foo/bar/a.txt',
   'build_bazel_rules_nodejs/internal/common/test/params_file.spec.js',
+  // $location (expands to runfiles manifest path of format repo/path/to/file)
+  'build_bazel_rules_nodejs/package.json',
 ];
 
 describe('params_file', function() {

internal/golden_file_test/bin.js

@@ -1,18 +1,20 @@
 const fs = require('fs');
 const path = require('path');
+const runfiles = require(process.env['BAZEL_NODE_RUNFILES_HELPER']);
 
 function main(args) {
   const [mode, golden_no_debug, golden_debug, actual] = args;
-  const actualPath = require.resolve(actual);
+  const actualPath = runfiles.resolveWorkspaceRelative(actual);
   const debugBuild = /\/bazel-out\/[^/\s]*-dbg\//.test(actualPath);
   const golden = debugBuild ? golden_debug : golden_no_debug;
   const actualContents = fs.readFileSync(actualPath, 'utf-8').replace(/\r\n/g, '\n');
-  const goldenContents = fs.readFileSync(require.resolve(golden), 'utf-8').replace(/\r\n/g, '\n');
+  const goldenContents =
+      fs.readFileSync(runfiles.resolveWorkspaceRelative(golden), 'utf-8').replace(/\r\n/g, '\n');
 
   if (actualContents !== goldenContents) {
     if (mode === '--out') {
       // Write to golden file
-      fs.writeFileSync(require.resolve(golden), actualContents);
+      fs.writeFileSync(runfiles.resolveWorkspaceRelative(golden), actualContents);
       console.error(`Replaced ${path.join(process.cwd(), golden)}`);
     } else if (mode === '--verify') {
       const unidiff = require('unidiff');

internal/golden_file_test/golden_file_test.bzl

@@ -15,7 +15,7 @@ Use `golden_debug` if the actual output changes when DEBUG is set.
     else:
         golden_debug = golden
 
-    loc = "$(location %s)"
+    loc = "$(rootpath %s)"
     nodejs_test(
         name = name,
         entry_point = "@build_bazel_rules_nodejs//internal/golden_file_test:bin.js",