fix(builtin): preserve lone $ in templated_args for legacy support (#…

…1772)

This preservation may be removed in a future version so that templated_args behaves the same as the built-in *_binary and *_test args

internal/common/preserve_legacy_rlocation.bzl → internal/common/preserve_legacy_templated_args.bzl

@@ -12,12 +12,15 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-"""Helper functions to preserve legacy `$(rlocation ` usage
+"""Helper functions to preserve legacy `$` usage in templated_args
 """
 
-def preserve_legacy_rlocation(input):
-    """Converts legacy `$(rlocation ` to `$(rlocation ` which is preserved when expanding make
-    variables with ctx.expand_make_variables.
+def preserve_legacy_templated_args(input):
+    """Converts legacy uses of `$` to `$$` so that the new call to ctx.expand_make_variables
+
+    Converts any lone `$` that is not proceeded by a `(` to `$$. Also converts legacy `$(rlocation `
+    to `$$(rlocation ` as that is a commonly used bash function so we don't want to break this
+    legacy behavior.
 
     Args:
       input: String to be modified
@@ -32,5 +35,9 @@ def preserve_legacy_rlocation(input):
             if i == 0 or input[i - 1] != "$":
                 # insert an additional "$"
                 result += "$"
+        elif input[i] == "$" and (i + 1 == length or (i + 1 < length and input[i + 1] != "(" and input[i + 1] != "$")):
+            if i == 0 or input[i - 1] != "$":
+                # insert an additional "$"
+                result += "$"
         result += input[i]
     return result

internal/common/test/BUILD.bazel

@@ -3,7 +3,7 @@ load("@npm_bazel_jasmine//:index.from_src.bzl", "jasmine_node_test")
 load("//internal/common:copy_to_bin.bzl", "copy_to_bin")
 load("//internal/common:params_file.bzl", "params_file")
 load(":expand_into_runfiles_test.bzl", "expand_into_runfiles_test_suite")
-load(":preserve_legacy_rlocation_test.bzl", "preserve_legacy_rlocation_test_suite")
+load(":preserve_legacy_templated_args_test.bzl", "preserve_legacy_templated_args_test_suite")
 
 licenses(["notice"])
 
@@ -78,4 +78,4 @@ sh_test(
 
 expand_into_runfiles_test_suite()
 
-preserve_legacy_rlocation_test_suite()
+preserve_legacy_templated_args_test_suite()

internal/common/test/preserve_legacy_templated_args_test.bzl

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

internal/node/node.bzl

@@ -24,7 +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:preserve_legacy_templated_args.bzl", "preserve_legacy_templated_args")
 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")
@@ -214,7 +214,7 @@ def _nodejs_binary_impl(ctx):
 
     # 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]
+    expanded_args = [preserve_legacy_templated_args(a) for a in ctx.attr.templated_args]
 
     # First expand predefined source/output path variables:
     # $(execpath), $(rootpath) & legacy $(location)

internal/node/test/BUILD.bazel

@@ -282,6 +282,8 @@ npm_package_bin(
         "$(TARGET_CPU)",
         "$(BINDIR)",
         "$(SOME_TEST_ENV)",
+        "somearg$$",
+        "some0arg",
     ],
     tool = ":expand_variables",
 )
@@ -296,6 +298,9 @@ jasmine_node_test(
         "$(TARGET_CPU)",
         "$(BINDIR)",
         "$(SOME_TEST_ENV)",
+        # Should preserve lone $ in templated_args
+        "somearg$",
+        "some$#arg",
     ],
 )