fix: correctly split quoted args

lib/expand_make_vars.bzl

@@ -1,5 +1,6 @@
 "Public API for expanding variables"
 
+# buildifier: disable=deprecated-function
 load("//lib/private:expand_locations.bzl", _expand_locations = "expand_locations")
 load("//lib/private:expand_variables.bzl", _expand_variables = "expand_variables")
 

lib/jq.bzl

@@ -144,7 +144,7 @@ def jq(name, srcs, filter = None, filter_file = None, args = [], out = None, dat
 
         filter_file: File containing filter expression (alternative to `filter`)
         args: Additional args to pass to jq
-        expand_args: Run bazel's location-expansion on the args.
+        expand_args: Run bazel's location and make variable expansion on the args.
         out: Name of the output json file; defaults to the rule name plus ".json"
         **kwargs: Other common named parameters such as `tags` or `visibility`
     """

lib/private/BUILD.bazel

@@ -161,7 +161,8 @@ bzl_library(
     srcs = ["jq.bzl"],
     visibility = ["//lib:__subpackages__"],
     deps = [
-        ":expand_locations",
+        ":expand_variables",
+        ":strings",
         "//lib:stamping",
     ],
 )
@@ -184,7 +185,10 @@ bzl_library(
     name = "params_file",
     srcs = ["params_file.bzl"],
     visibility = ["//lib:__subpackages__"],
-    deps = [":expand_locations"],
+    deps = [
+        ":expand_variables",
+        ":strings",
+    ],
 )
 
 bzl_library(
@@ -206,8 +210,8 @@ bzl_library(
     srcs = ["run_binary.bzl"],
     visibility = ["//lib:__subpackages__"],
     deps = [
-        ":expand_locations",
         ":expand_variables",
+        ":strings",
         "//lib:stamping",
         "@bazel_skylib//lib:dicts",
     ],

lib/private/bats.bzl

@@ -2,7 +2,6 @@
 
 load("//lib:paths.bzl", "BASH_RLOCATION_FUNCTION", "to_rlocation_path")
 load("//lib:windows_utils.bzl", "create_windows_native_launcher_script")
-load(":expand_locations.bzl", "expand_locations")
 load(":expand_variables.bzl", "expand_variables")
 
 _LAUNCHER_TMPL = """#!/usr/bin/env bash
@@ -42,7 +41,7 @@ def _bats_test_impl(ctx):
     for (key, value) in ctx.attr.env.items():
         envs.append(_ENV_SET.format(
             key = key,
-            value = " ".join([expand_variables(ctx, exp, attribute_name = "env") for exp in expand_locations(ctx, value, ctx.attr.data).split(" ")]),
+            value = expand_variables(ctx, ctx.expand_location(value, targets = ctx.attr.data), attribute_name = "env"),
         ))
 
     # See https://www.msys2.org/wiki/Porting/:

lib/private/expand_locations.bzl

@@ -32,6 +32,9 @@ def expand_locations(ctx, input, targets = []):
 
     Returns:
       The expanded path or the original path
+
+    Deprecated:
+      Use vanilla `ctx.expand_location(input, targets = targets)` instead
     """
 
     return ctx.expand_location(input, targets = targets)

lib/private/expand_template.bzl

@@ -2,16 +2,12 @@
 
 load("@bazel_skylib//lib:dicts.bzl", "dicts")
 load("//lib:stamping.bzl", "STAMP_ATTRS", "maybe_stamp")
-load(":expand_locations.bzl", _expand_locations = "expand_locations")
-load(":expand_variables.bzl", _expand_variables = "expand_variables")
+load(":expand_variables.bzl", "expand_variables")
 
 def _expand_substitutions(ctx, output, substitutions):
     result = {}
     for k, v in substitutions.items():
-        result[k] = " ".join([
-            _expand_variables(ctx, e, outs = [output], attribute_name = "substitutions")
-            for e in _expand_locations(ctx, v, ctx.attr.data).split(" ")
-        ])
+        result[k] = expand_variables(ctx, ctx.expand_location(v, targets = ctx.attr.data), outs = [output], attribute_name = "substitutions")
     return result
 
 def _expand_template_impl(ctx):

lib/private/jq.bzl

@@ -1,7 +1,8 @@
 """Implementation for jq rule"""
 
 load("//lib:stamping.bzl", "STAMP_ATTRS", "maybe_stamp")
-load(":expand_locations.bzl", "expand_locations")
+load(":expand_variables.bzl", "expand_variables")
+load(":strings.bzl", "split_args")
 
 _jq_attrs = dict({
     "srcs": attr.label_list(
@@ -23,20 +24,14 @@ _jq_attrs = dict({
     ),
 }, **STAMP_ATTRS)
 
-def _expand_locations(ctx, s):
-    # `.split(" ")` is a work-around https://github.com/bazelbuild/bazel/issues/10309
-    # TODO: If the string has intentional spaces or if one or more of the expanded file
-    # locations has a space in the name, we will incorrectly split it into multiple arguments
-    return expand_locations(ctx, s, targets = ctx.attr.data).split(" ")
-
 def _jq_impl(ctx):
     jq_bin = ctx.toolchains["@aspect_bazel_lib//lib:jq_toolchain_type"].jqinfo.bin
 
     out = ctx.outputs.out or ctx.actions.declare_file(ctx.attr.name + ".json")
     if ctx.attr.expand_args:
         args = []
         for a in ctx.attr.args:
-            args += _expand_locations(ctx, a)
+            args += split_args(expand_variables(ctx, ctx.expand_location(a, targets = ctx.attr.data), outs = [out]))
     else:
         args = ctx.attr.args
 
@@ -53,7 +48,7 @@ def _jq_impl(ctx):
         args = args + ["--null-input"]
 
     if ctx.attr.filter_file:
-        args = args + ["--from-file '%s'" % ctx.file.filter_file.path]
+        args = args + ["--from-file", ctx.file.filter_file.path]
         inputs.append(ctx.file.filter_file)
 
     stamp = maybe_stamp(ctx)
@@ -77,9 +72,16 @@ def _jq_impl(ctx):
 
         args = args + ["--slurpfile", "STAMP", stamp_json.path]
 
+    # quote args that contain spaces
+    quoted_args = []
+    for a in args:
+        if " " in a:
+            a = "'{}'".format(a)
+        quoted_args.append(a)
+
     cmd = "{jq} {args} {filter} {sources} > {out}".format(
         jq = jq_bin.path,
-        args = " ".join(args),
+        args = " ".join(quoted_args),
         filter = "'%s'" % ctx.attr.filter if ctx.attr.filter else "",
         sources = " ".join(["'%s'" % file.path for file in ctx.files.srcs]),
         out = out.path,

lib/private/params_file.bzl

@@ -1,6 +1,7 @@
 "params_file rule"
 
-load(":expand_locations.bzl", "expand_locations")
+load(":expand_variables.bzl", "expand_variables")
+load(":strings.bzl", "split_args")
 
 _ATTRS = {
     "args": attr.string_list(),
@@ -13,12 +14,6 @@ _ATTRS = {
     "_windows_constraint": attr.label(default = "@platforms//os:windows"),
 }
 
-def _expand_locations(ctx, s):
-    # `.split(" ")` is a work-around https://github.com/bazelbuild/bazel/issues/10309
-    # TODO: If the string has intentional spaces or if one or more of the expanded file
-    # locations has a space in the name, we will incorrectly split it into multiple arguments
-    return expand_locations(ctx, s, targets = ctx.attr.data).split(" ")
-
 def _params_file_impl(ctx):
     is_windows = ctx.target_platform_has_constraint(ctx.attr._windows_constraint[platform_common.ConstraintValueInfo])
 
@@ -31,12 +26,9 @@ def _params_file_impl(ctx):
 
     expanded_args = []
 
-    # First expand predefined source/output path variables
+    # Expand predefined source/output path && predefined variables & custom variables
     for a in ctx.attr.args:
-        expanded_args += _expand_locations(ctx, a)
-
-    # Next expand predefined variables & custom variables
-    expanded_args = [ctx.expand_make_variables("args", e, {}) for e in expanded_args]
+        expanded_args += split_args(expand_variables(ctx, ctx.expand_location(a, targets = ctx.attr.data), outs = [ctx.outputs.out]))
 
     # ctx.actions.write creates a FileWriteAction which uses UTF-8 encoding.
     ctx.actions.write(

lib/private/run_binary.bzl

@@ -16,8 +16,8 @@
 
 load("@bazel_skylib//lib:dicts.bzl", "dicts")
 load("//lib:stamping.bzl", "STAMP_ATTRS", "maybe_stamp")
-load(":expand_locations.bzl", "expand_locations")
 load(":expand_variables.bzl", "expand_variables")
+load(":strings.bzl", "split_args")
 
 def _run_binary_impl(ctx):
     args = ctx.actions.args()
@@ -47,15 +47,11 @@ Possible fixes:
             rule_kind = str(ctx.attr.tool.label),
         ))
 
-    # `expand_locations(...).split(" ")` is a work-around https://github.com/bazelbuild/bazel/issues/10309
-    # _expand_locations returns an array of args to support $(execpaths) expansions.
-    # TODO: If the string has intentional spaces or if one or more of the expanded file
-    # locations has a space in the name, we will incorrectly split it into multiple arguments
     for a in ctx.attr.args:
-        args.add_all([expand_variables(ctx, e, outs = outputs) for e in expand_locations(ctx, a, ctx.attr.srcs).split(" ")])
+        args.add_all(split_args(expand_variables(ctx, ctx.expand_location(a, targets = ctx.attr.srcs), outs = outputs)))
     envs = {}
     for k, v in ctx.attr.env.items():
-        envs[k] = " ".join([expand_variables(ctx, e, outs = outputs, attribute_name = "env") for e in expand_locations(ctx, v, ctx.attr.srcs).split(" ")])
+        envs[k] = expand_variables(ctx, ctx.expand_location(v, targets = ctx.attr.srcs), outs = outputs, attribute_name = "env")
 
     stamp = maybe_stamp(ctx)
     if stamp:

lib/private/strings.bzl

@@ -583,3 +583,73 @@ def hex(number):
         hex_string = "0"
 
     return "{}0x{}".format("-" if is_signed else "", hex_string)
+
+def split_args(s):
+    """Split a string into a list space separated arguments
+
+    Unlike the naive `.split(" ")`, this function takes quoted strings
+    into account.
+
+    Args:
+        s: input string
+
+    Returns:
+        list of string with each an argument found in the input string
+    """
+    args = []
+    arg = ""
+    single_quote = False
+    double_quote = False
+    escape = False
+    for c in s.elems():
+        if c == "\\":
+            escape = True
+            continue
+        if escape:
+            # this is an escaped character
+            if c == " ":
+                # a dangling escape is not an escape, put the backslack back
+                arg = arg + "\\"
+            else:
+                escape = False
+        else:
+            # not an escaped character, look for quotes & spaces
+            if c == "'":
+                # single quote char
+                if double_quote:
+                    # we're in a double quote so single quotes are just chars
+                    pass
+                elif single_quote:
+                    # end of single quote
+                    single_quote = False
+                    continue
+                else:
+                    # start of single quote
+                    single_quote = True
+                    continue
+            elif c == "\"":
+                # double quote char
+                if single_quote:
+                    # we're in a single quote so double quotes are just chars
+                    pass
+                elif double_quote:
+                    # end of double quote
+                    double_quote = False
+                    continue
+                else:
+                    # start of double quote
+                    double_quote = True
+                    continue
+        if c == " ":
+            if not single_quote and not double_quote:
+                # splitting space
+                if arg != "":
+                    args.append(arg)
+                arg = ""
+                continue
+        arg = arg + c
+
+    # final arg?
+    if arg != "":
+        args.append(arg)
+    return args

lib/tests/base64_tests.bzl

@@ -1,5 +1,6 @@
 """unit tests for base64"""
 
+load("@bazel_skylib//lib:partial.bzl", "partial")
 load("@bazel_skylib//lib:unittest.bzl", "asserts", "unittest")
 load("//lib/private:base64.bzl", "decode", "encode")
 load("//lib/private:strings.bzl", "INT_TO_CHAR")
@@ -45,5 +46,5 @@ base64_test = unittest.make(_base64_test_impl)
 def base64_test_suite():
     unittest.suite(
         "base64_tests",
-        base64_test,
+        partial.make(base64_test, timeout = "short"),
     )

lib/tests/lists_test.bzl

@@ -1,5 +1,6 @@
 """unit tests for lists"""
 
+load("@bazel_skylib//lib:partial.bzl", "partial")
 load("@bazel_skylib//lib:unittest.bzl", "asserts", "unittest")
 load("//lib/private:lists.bzl", "every", "filter", "find", "map", "once", "pick", "some", "unique")
 
@@ -82,12 +83,12 @@ unique_test = unittest.make(_unique_test_impl)
 def lists_test_suite():
     unittest.suite(
         "lists_tests",
-        every_test,
-        filter_test,
-        find_test,
-        map_test,
-        once_test,
-        pick_test,
-        some_test,
-        unique_test,
+        partial.make(every_test, timeout = "short"),
+        partial.make(filter_test, timeout = "short"),
+        partial.make(find_test, timeout = "short"),
+        partial.make(map_test, timeout = "short"),
+        partial.make(once_test, timeout = "short"),
+        partial.make(pick_test, timeout = "short"),
+        partial.make(some_test, timeout = "short"),
+        partial.make(unique_test, timeout = "short"),
     )