feat(bazel): allow integration commands to resolve executables through expansion (#285)

* feat(bazel): allow integration commands to resolve executables through expansion

Gives a tool built using `nodejs_binary` or `sh_binary`, the tool cannot
be used as binary in an integration test command currently because:

* Bazel location expansion does not respect the executable "FilesToRun"
  information of the binaries, and rather sees _all_ outputs, erroring
  that `$(locations X)` need to be used instead. See e.g. bazelbuild/bazel#11820)

* The command is currently split by space and this decouples the Make
  funtion expression incorrectly into a broken expansion. e.g.
  `$(rootpath X)` will become `["$(rootpath", "X"]`).

This commit fixes both things by relying on the Bazel
`CommandHelper.java` class which handles the expansion of commands as
expected, with similar semantics of a `genrule` as per `GenRuleBase.java`.

This allows test authors to conveniently use a `nodejs_binary` or
`sh_binary` as command binary because the `CommandHelper` knows
exactly which targets provide executables or not. There is no good API
for the plain Make expansion of a command itself, so we use a little trick
(which is documented sufficiently in the code).

* fixup! feat(bazel): allow integration commands to resolve executables through expansion

Address feedback

Author: devversion

bazel/integration/index.bzl

@@ -6,6 +6,14 @@ def _serialize_file(file):
 
     return struct(path = file.path, shortPath = file.short_path)
 
+def _create_expanded_value(value, isExpanded):
+    """Creates a JSON serializable dictionary matching the `BazelExpandedValue` type in
+      the test runner."""
+    return {
+        "value": value,
+        "containsExpansion": isExpanded,
+    }
+
 def _serialize_and_expand_value(ctx, value, description):
     """Expands Bazel make variable and location expressions for the given value. Returns a JSON
       serializable dictionary matching the `BazelExpandedValue` type in the test runner."""
@@ -17,14 +25,51 @@ def _serialize_and_expand_value(ctx, value, description):
     # directly use `ctx.var` but would have switch users from e.g. `$(VAR)` to `{VAR}`.
     expanded_make_value = ctx.expand_make_variables(description, expanded_location_value, {})
 
-    return {
-        "value": expanded_make_value,
-        "containsExpansion": expanded_make_value != value,
-    }
+    return _create_expanded_value(expanded_make_value, expanded_make_value != value)
+
+def _expand_and_split_command(ctx, command):
+    """Expands a command using the Bazel command helper. The command is then split into the
+      binary and its arguments, matching the runner `[BazelExpandedValue, ...string[]]` type."""
+
+    # Instead of manually resolving the command using e.g. `ctx.expand_location`, we use
+    # the Bazel `resolve_command` helper which internally follows the semantics of a `genrule`,
+    # allowing for better expansion/resolution of tools provided in the `data` attribute.
+    # This is necessary so that e.g. executables from a `sh_binary` can be conveniently
+    # expanded through `$(rootpath <label>`). Note that the same would not work with
+    # `ctx.expand_location` as a `sh_binary` exposes multiple files causing an error like:
+    #  -->  `expression expands to more than one file, please use $(locations ...)`.
+    # The Bazel command helper utility instead (which is also used by the genrule internally),
+    # will be able to determine the executable (based on the current exec platform) and expand it.
+    # https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java;l=175-199;drc=2255ce4165f936f695111020fa664b259a875c4a.
+    inputs, resolved_bash_command, manifests = ctx.resolve_command(
+        command = command,
+        attribute = "command (%s)" % command,
+        expand_locations = True,
+        make_variables = ctx.var,
+        tools = ctx.attr.data,
+    )
+
+    # If the resolved command does not have three arguments, then there were too many arguments
+    # and Bazel extracted the command into a dedicated Bash script that we cannot read here.
+    # https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/analysis/BashCommandConstructor.java;l=40;drc=2255ce4165f936f695111020fa664b259a875c4a.
+    # https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java;l=275-282;drc=2255ce4165f936f695111020fa664b259a875c4a.
+    if len(resolved_bash_command) != 3:
+        fail("Test command too long. Please use a shorter one, or extract this " +
+             "into a dedicated script: %s" % command)
+
+    # The third argument of the resolved `bash -c` command will hold the actually expanded command.
+    # We extract the command since we were only interested in the proper expansion of tools.
+    # https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/analysis/BashCommandConstructor.java;l=40;drc=2255ce4165f936f695111020fa664b259a875c4a.
+    resolved_command = resolved_bash_command[2].split(" ")
+    resolved_binary = resolved_command[0]
+    original_binary = command.split(" ", 1)[0]
+
+    # If the resolved command binary does not match the binary from the original command, then
+    # we know a Make expression has been expanded and we capture that in a `BazelExpandedValue`.
+    if resolved_binary != original_binary:
+        return [_create_expanded_value(resolved_binary, True)] + resolved_command[1:]
 
-def _split_and_expand_command(ctx, command):
-    """Splits a command into the binary and its arguments. Bazel make expression are expanded."""
-    return [_serialize_and_expand_value(ctx, v, "command") for v in command.split(" ", 1)]
+    return [_create_expanded_value(resolved_binary, False)] + resolved_command[1:]
 
 def _serialize_and_expand_environment(ctx, environment_dict):
     """Converts the given environment dictionary into a JSON-serializable dictionary
@@ -75,7 +120,7 @@ def _integration_test_config_impl(ctx):
     config = struct(
         testPackage = ctx.label.package,
         testFiles = [_serialize_file(f) for f in ctx.files.srcs],
-        commands = [_split_and_expand_command(ctx, c) for c in ctx.attr.commands],
+        commands = [_expand_and_split_command(ctx, c) for c in ctx.attr.commands],
         environment = _serialize_and_expand_environment(ctx, ctx.attr.environment),
         npmPackageMappings = npmPackageMappings,
         toolMappings = toolMappings,

bazel/integration/test_runner/main.ts

@@ -20,7 +20,7 @@ interface TestConfig {
   testFiles: BazelFileInfo[];
   npmPackageMappings: Record<string, BazelFileInfo>;
   toolMappings: Record<string, BazelFileInfo>;
-  commands: [[binary: BazelExpandedValue, ...args: BazelExpandedValue[]]];
+  commands: [[binary: BazelExpandedValue, ...args: string[]]];
   environment: Record<string, BazelExpandedValue>;
 }
 

bazel/integration/test_runner/runner.ts

@@ -44,7 +44,7 @@ export class TestRunner {
     private readonly testPackage: string,
     private readonly toolMappings: Record<string, BazelFileInfo>,
     private readonly npmPackageMappings: Record<string, BazelFileInfo>,
-    private readonly commands: [[binary: BazelExpandedValue, ...args: BazelExpandedValue[]]],
+    private readonly commands: [[binary: BazelExpandedValue, ...args: string[]]],
     private readonly environment: Record<string, BazelExpandedValue>,
   ) {}
 
@@ -218,10 +218,7 @@ export class TestRunner {
       const resolvedBinary = binary.containsExpansion
         ? await resolveBinaryWithRunfiles(binary.value)
         : binary.value;
-      const evaluatedArgs = expandEnvironmentVariableSubstitutions(
-        args.map((v) => v.value),
-        commandEnv,
-      );
+      const evaluatedArgs = expandEnvironmentVariableSubstitutions(args, commandEnv);
       const success = await runCommandInChildProcess(
         resolvedBinary,
         evaluatedArgs,
@@ -231,7 +228,7 @@ export class TestRunner {
 
       if (!success) {
         throw Error(
-          `Integration test command: \`${binary.value} ${evaluatedArgs.join(' ')}\` failed. ` +
+          `Integration test command: \`${resolvedBinary} ${evaluatedArgs.join(' ')}\` failed. ` +
             `See error output above for details.`,
         );
       }

bazel/integration/tests/external_command_script/BUILD.bazel

@@ -0,0 +1,15 @@
+load("//bazel/integration:index.bzl", "integration_test")
+
+sh_binary(
+    name = "external_script",
+    srcs = ["external_script.sh"],
+)
+
+integration_test(
+    name = "test",
+    srcs = [],
+    commands = [
+        "$(rootpath :external_script) First Second",
+    ],
+    data = [":external_script"],
+)

bazel/integration/tests/external_command_script/external_script.sh

@@ -0,0 +1,11 @@
+#!/usr/bin/env bash
+
+if [[ "$1" != "First" ]]; then
+  echo "First argument not matching: $1"
+  exit 1
+fi
+
+if [[ "$2" != "Second" ]]; then
+  echo "Second argument not matching: $2"
+  exit 1
+fi