refactor: preserve legacy "$(rlocation)" usage to make custom variabl…

…e expansion non-breaking

internal/common/preserve_legacy_rlocation.bzl

@@ -0,0 +1,36 @@
+# Copyright 2017 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Helper functions to preserve legacy `$(rlocation ` usage
+"""
+
+def preserve_legacy_rlocation(input):
+    """Converts legacy `$(rlocation ` to `$(rlocation ` which is preserved when expanding make
+    variables with ctx.expand_make_variables.
+
+    Args:
+      input: String to be modified
+
+    Returns:
+      The modified string
+    """
+    result = ""
+    length = len(input)
+    for i in range(length):
+        if input[i:].startswith("$(rlocation "):
+            if i == 0 or input[i - 1] != "$":
+                # insert an additional "$"
+                result += "$"
+        result += input[i]
+    return result

internal/common/test/BUILD.bazel

@@ -77,3 +77,7 @@ sh_test(
 load(":expand_into_runfiles_test.bzl", "expand_into_runfiles_test_suite")
 
 expand_into_runfiles_test_suite()
+
+load(":preserve_legacy_rlocation_test.bzl", "preserve_legacy_rlocation_test_suite")
+
+preserve_legacy_rlocation_test_suite()

internal/common/test/preserve_legacy_rlocation_test.bzl

@@ -0,0 +1,31 @@
+"Unit tests for //internal/common:preserve_legacy_rlocation.bzl"
+
+load("@bazel_skylib//lib:unittest.bzl", "asserts", "unittest")
+load("//internal/common:preserve_legacy_rlocation.bzl", "preserve_legacy_rlocation")
+
+def _impl(ctx):
+    env = unittest.begin(ctx)
+
+    conversions = {
+        "$$(rlocation foobar)": "$$(rlocation foobar)",
+        "$(rlocation foobar)": "$$(rlocation foobar)",
+        "$(rlocation! foobar)": "$(rlocation! foobar)",
+    }
+
+    for key in conversions:
+        asserts.equals(env, "%s" % conversions[key], preserve_legacy_rlocation("%s" % key))
+        asserts.equals(env, " %s " % conversions[key], preserve_legacy_rlocation(" %s " % key))
+        asserts.equals(env, "%s%s" % (conversions[key], conversions[key]), preserve_legacy_rlocation("%s%s" % (key, key)))
+        asserts.equals(env, "%s %s" % (conversions[key], conversions[key]), preserve_legacy_rlocation("%s %s" % (key, key)))
+        asserts.equals(env, " %s %s " % (conversions[key], conversions[key]), preserve_legacy_rlocation(" %s %s " % (key, key)))
+        asserts.equals(env, "a%sb%sc" % (conversions[key], conversions[key]), preserve_legacy_rlocation("a%sb%sc" % (key, key)))
+
+    return unittest.end(env)
+
+preserve_legacy_rlocation_test = unittest.make(
+    impl = _impl,
+    attrs = {},
+)
+
+def preserve_legacy_rlocation_test_suite():
+    unittest.suite("preserve_legacy_rlocation_tests", preserve_legacy_rlocation_test)

internal/node/node.bzl

@@ -24,6 +24,7 @@ load("//:providers.bzl", "JSNamedModuleInfo", "NodeRuntimeDepsInfo", "NpmPackage
 load("//internal/common:expand_into_runfiles.bzl", "expand_location_into_runfiles")
 load("//internal/common:module_mappings.bzl", "module_mappings_runtime_aspect")
 load("//internal/common:path_utils.bzl", "strip_external")
+load("//internal/common:preserve_legacy_rlocation.bzl", "preserve_legacy_rlocation")
 load("//internal/common:windows_utils.bzl", "create_windows_native_launcher_script", "is_windows")
 load("//internal/linker:link_node_modules.bzl", "module_mappings_aspect", "write_node_modules_manifest")
 load("//internal/node:node_repositories.bzl", "BUILT_IN_NODE_PLATFORMS")
@@ -211,9 +212,13 @@ def _nodejs_binary_impl(ctx):
 
     is_builtin = ctx.attr._node.label.workspace_name in ["nodejs_%s" % p for p in BUILT_IN_NODE_PLATFORMS]
 
+    # First replace any instances of "$(rlocation " with "$$(rlocation " to preserve
+    # legacy uses of "$(rlocation"
+    expanded_args = [preserve_legacy_rlocation(a) for a in ctx.attr.templated_args]
+
     # First expand predefined source/output path variables:
     # $(execpath), $(rootpath) & legacy $(location)
-    expanded_args = [expand_location_into_runfiles(ctx, a, ctx.attr.data) for a in ctx.attr.templated_args]
+    expanded_args = [expand_location_into_runfiles(ctx, a, ctx.attr.data) for a in expanded_args]
 
     # Next expand predefined variables & custom variables
     expanded_args = [ctx.expand_make_variables("templated_args", e, {}) for e in expanded_args]
@@ -510,7 +515,10 @@ of being expanded. `$(rlocation)` is then evaluated by the bash node launcher sc
 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
+before being passed as an argument to the program.
+
+NB: nodejs_binary and nodejs_test will preserve the legacy behavior of `$(rlocation)` so users don't
+need to update to `$$(rlocation)`. This may be changed in the future.
 
 2. Subject to predefined variables & custom variable substitutions.
 

internal/node/test/BUILD.bazel

@@ -111,7 +111,9 @@ nodejs_test(
         "@npm//tmp",
     ],
     entry_point = ":bootstrap.spec.js",
-    templated_args = ["--node_options=--require=$$(rlocation $(location :bootstrap.js))"],
+    # Ensure that legacy "$(rlocation //:foobar)" usage is preserved so that users
+    # don't have to upgrade to "$$(rlocation //:foobar)"
+    templated_args = ["--node_options=--require=$(rlocation $(location :bootstrap.js))"],
 )
 
 nodejs_test(