Decide which sh path to used based on the platform the action will be…

… executed on, instead of the host platform. This makes it possible to execute sh actions on platforms other than the host platform. Note that --shell_executable (Bazel only) now only affects actions configured for host. RELNOTES: Bazel now selects sh path based on execution platform instead of host platform, making it possible to execute sh actions in multiplatform builds. --shell_executable now only applies to actions configured for host. PiperOrigin-RevId: 453946322 Change-Id: Ibc811939f20c5fac7789d631f36933da6e8bcb35
bazelbuild · Jun 9, 2022 · eeb2e04 · eeb2e04
1 parent 442155f
commit eeb2e04
Show file tree

Hide file tree

Showing 16 changed files with 223 additions and 124 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ShToolchain.java b/src/main/java/com/google/devtools/build/lib/analysis/ShToolchain.java
@@ -14,48 +14,58 @@
 
 package com.google.devtools.build.lib.analysis;
 
+import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
+import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
+import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.vfs.PathFragment;
 
 /** Class to work with the shell toolchain, e.g. get the shell interpreter's path. */
 public final class ShToolchain {
 
+  private static PathFragment getHostOrDefaultPath() {
+    OS current = OS.getCurrent();
+    if (!ShellConfiguration.getShellExecutables().containsKey(current)) {
+      current = OS.UNKNOWN;
+    }
+    Preconditions.checkState(
+        ShellConfiguration.getShellExecutables().containsKey(current),
+        "shellExecutableFinder should set a value with key '%s'",
+        current);
+
+    return ShellConfiguration.getShellExecutables().get(current);
+  }
+
   /**
-   * Returns the shell executable's path, or an empty path if not set.
+   * Returns the default shell executable's path for the host OS.
    *
    * <p>This method checks the configuration's {@link ShellConfiguration} fragment.
    */
-  public static PathFragment getPath(BuildConfigurationValue config) {
-    PathFragment result = PathFragment.EMPTY_FRAGMENT;
-
-    ShellConfiguration configFragment =
-        (ShellConfiguration) config.getFragment(ShellConfiguration.class);
+  public static PathFragment getPathForHost(BuildConfigurationValue config) {
+    ShellConfiguration configFragment = config.getFragment(ShellConfiguration.class);
     if (configFragment != null) {
-      PathFragment path = configFragment.getShellExecutable();
-      if (path != null) {
-        result = path;
+      if (configFragment.getOptionsBasedDefault() != null) {
+        return configFragment.getOptionsBasedDefault();
+      } else {
+        return getHostOrDefaultPath();
       }
     }
-
-    return result;
+    return PathFragment.EMPTY_FRAGMENT;
   }
 
   /**
-   * Returns the shell executable's path, or reports a rule error if the path is empty.
-   *
-   * <p>This method checks the rule's configuration's {@link ShellConfiguration} fragment for the
-   * shell executable's path. If null or empty, the method reports an error against the rule.
+   * Returns the shell executable's path for the provided platform. If none is present, return the
+   * path for the host platform. Otherwise, return the default.
    */
-  public static PathFragment getPathOrError(RuleContext ctx) {
-    PathFragment result = getPath(ctx.getConfiguration());
-
-    if (result.isEmpty()) {
-      ctx.ruleError(
-          "This rule needs a shell interpreter. Use the --shell_executable=<path> flag to specify"
-              + " the interpreter's path, e.g. --shell_executable=/usr/local/bin/bash");
+  public static PathFragment getPathOrError(PlatformInfo platformInfo) {
+    for (OS os : ShellConfiguration.getShellExecutables().keySet()) {
+      if (platformInfo
+          .constraints()
+          .hasConstraintValue(ShellConfiguration.OS_TO_CONSTRAINTS.get(os))) {
+        return ShellConfiguration.getShellExecutables().get(os);
+      }
     }
-
-    return result;
+    return getHostOrDefaultPath();
   }
 
   private ShToolchain() {}

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ShellConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/ShellConfiguration.java
@@ -18,49 +18,100 @@
 import com.google.devtools.build.lib.analysis.config.Fragment;
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.build.lib.analysis.config.RequiresOptions;
+import com.google.devtools.build.lib.analysis.platform.ConstraintSettingInfo;
+import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.OptionsUtils.PathFragmentConverter;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.common.options.Option;
 import com.google.devtools.common.options.OptionDocumentationCategory;
 import com.google.devtools.common.options.OptionEffectTag;
 import com.google.devtools.common.options.OptionMetadataTag;
+import java.util.Map;
 import java.util.function.Function;
 
 /** A configuration fragment that tells where the shell is. */
 @RequiresOptions(options = {ShellConfiguration.Options.class})
 public class ShellConfiguration extends Fragment {
-  private static final ImmutableMap<OS, PathFragment> OS_SPECIFIC_SHELL =
-      ImmutableMap.<OS, PathFragment>builder()
-          .put(OS.WINDOWS, PathFragment.create("c:/tools/msys64/usr/bin/bash.exe"))
-          .put(OS.FREEBSD, PathFragment.create("/usr/local/bin/bash"))
-          .put(OS.OPENBSD, PathFragment.create("/usr/local/bin/bash"))
-          .buildOrThrow();
 
-  private static Function<BuildOptions, PathFragment> shellExecutableFinder;
+  private static Map<OS, PathFragment> shellExecutables;
+
+  private static final ConstraintSettingInfo OS_CONSTRAINT_SETTING =
+      ConstraintSettingInfo.create(
+          Label.parseAbsoluteUnchecked("@platforms//os:os"));
+
+  private static Function<Options, PathFragment> optionsBasedDefault;
 
   /**
-   * Injects a function for finding the shell executable path, given the current configuration's
-   * {@link BuildOptions} and whatever system-specific logic the provider wishes to use.
+   * Injects a function for retrieving the default sh path from build options, and a map for
+   * locating the correct sh executable given a set of target constraints.
    */
-  public static void injectShellExecutableFinder(Function<BuildOptions, PathFragment> finder) {
+  public static void injectShellExecutableFinder(
+      Function<Options, PathFragment> shellFromOptionsFinder, Map<OS, PathFragment> osToShellMap) {
     // It'd be nice not to have to set a global static field. But there are so many disparate calls
-    // to getShellExecutable() (in both the build's analysis phase and in the run command) that
+    // to getShellExecutables() (in both the build's analysis phase and in the run command) that
     // feeding this through instance variables is unwieldy. Fortunately this info is a function of
     // the Blaze implementation and not something that might change between builds.
-    shellExecutableFinder = finder;
+    optionsBasedDefault = shellFromOptionsFinder;
+    shellExecutables = osToShellMap;
+  }
+
+  /**
+   * Injects a map for locating the correct sh executable given a set of target constraints. Assumes
+   * no options-based default shell.
+   */
+  public static void injectShellExecutableFinder(Map<OS, PathFragment> osToShellMap) {
+    optionsBasedDefault = (options) -> null;
+    shellExecutables = osToShellMap;
   }
+  // Standard mapping between OS and the corresponding platform constraints.
+  static final ImmutableMap<OS, ConstraintValueInfo> OS_TO_CONSTRAINTS =
+      ImmutableMap.<OS, ConstraintValueInfo>builder()
+          .put(
+              OS.DARWIN,
+              ConstraintValueInfo.create(
+                  OS_CONSTRAINT_SETTING,
+                  Label.parseAbsoluteUnchecked("@platforms//os:osx")))
+          .put(
+              OS.WINDOWS,
+              ConstraintValueInfo.create(
+                  OS_CONSTRAINT_SETTING,
+                  Label.parseAbsoluteUnchecked("@platforms//os:windows")))
+          .put(
+              OS.FREEBSD,
+              ConstraintValueInfo.create(
+                  OS_CONSTRAINT_SETTING,
+                  Label.parseAbsoluteUnchecked("@platforms//os:freebsd")))
+          .put(
+              OS.OPENBSD,
+              ConstraintValueInfo.create(
+                  OS_CONSTRAINT_SETTING,
+                  Label.parseAbsoluteUnchecked("@platforms//os:openbsd")))
+          .put(
+              OS.UNKNOWN,
+              ConstraintValueInfo.create(
+                  OS_CONSTRAINT_SETTING,
+                  Label.parseAbsoluteUnchecked("@platforms//os:none")))
+          .buildOrThrow();
 
-  private final PathFragment shellExecutable;
   private final boolean useShBinaryStubScript;
 
+  private final PathFragment defaultShellExecutableFromOptions;
+
   public ShellConfiguration(BuildOptions buildOptions) {
-    this.shellExecutable = shellExecutableFinder.apply(buildOptions);
+    this.defaultShellExecutableFromOptions =
+        optionsBasedDefault.apply(buildOptions.get(Options.class));
     this.useShBinaryStubScript = buildOptions.get(Options.class).useShBinaryStubScript;
   }
 
-  public PathFragment getShellExecutable() {
-    return shellExecutable;
+  public static Map<OS, PathFragment> getShellExecutables() {
+    return shellExecutables;
+  }
+
+  /* Returns a function for retrieving the default shell from build options. */
+  public PathFragment getOptionsBasedDefault() {
+    return defaultShellExecutableFromOptions;
   }
 
   public boolean useShBinaryStubScript() {
@@ -103,22 +154,4 @@ public Options getHost() {
       return host;
     }
   }
-
-  public static PathFragment determineShellExecutable(
-      OS os, Options options, PathFragment defaultShell) {
-    if (options.shellExecutable != null) {
-      return options.shellExecutable;
-    }
-
-    // Honor BAZEL_SH env variable for backwards compatibility.
-    String path = System.getenv("BAZEL_SH");
-    if (path != null) {
-      return PathFragment.create(path);
-    }
-    // TODO(ulfjack): instead of using the OS Bazel runs on, we need to use the exec platform,
-    // which may be different for remote execution. For now, this can be overridden with
-    // --shell_executable, so at least there's a workaround.
-    PathFragment result = OS_SPECIFIC_SHELL.get(os);
-    return result != null ? result : defaultShell;
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java
@@ -48,11 +48,13 @@
 import com.google.devtools.build.lib.analysis.actions.Substitution;
 import com.google.devtools.build.lib.analysis.actions.SymlinkAction;
 import com.google.devtools.build.lib.analysis.actions.TemplateExpansionAction;
+import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
 import com.google.devtools.build.lib.collect.nestedset.Depset;
 import com.google.devtools.build.lib.collect.nestedset.Depset.TypeException;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
+import com.google.devtools.build.lib.packages.ExecGroup;
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
 import com.google.devtools.build.lib.server.FailureDetails;
@@ -440,6 +442,24 @@ private StarlarkSemantics getSemantics() {
     return context.getStarlarkSemantics();
   }
 
+  private void verifyExecGroup(Object execGroupUnchecked, RuleContext ctx) throws EvalException {
+    String execGroup = (String) execGroupUnchecked;
+    if (!StarlarkExecGroupCollection.isValidGroupName(execGroup)
+        || !ctx.hasToolchainContext(execGroup)) {
+      throw Starlark.errorf("Action declared for non-existent exec group '%s'.", execGroup);
+    }
+  }
+
+  private PlatformInfo getExecutionPlatform(Object execGroupUnchecked, RuleContext ctx)
+      throws EvalException {
+    if (execGroupUnchecked == Starlark.NONE) {
+      return ctx.getExecutionPlatform(ExecGroup.DEFAULT_EXEC_GROUP_NAME);
+    } else {
+      verifyExecGroup(execGroupUnchecked, ctx);
+      return ctx.getExecutionPlatform((String) execGroupUnchecked);
+    }
+  }
+
   @Override
   public void runShell(
       Sequence<?> outputs,
@@ -464,11 +484,12 @@ public void runShell(
     buildCommandLine(builder, arguments);
 
     if (commandUnchecked instanceof String) {
-      Map<String, String> executionInfo =
+      ImmutableMap<String, String> executionInfo =
           ImmutableMap.copyOf(TargetUtils.getExecutionInfo(ruleContext.getRule()));
       String helperScriptSuffix = String.format(".run_shell_%d.sh", runShellOutputCounter++);
       String command = (String) commandUnchecked;
-      PathFragment shExecutable = ShToolchain.getPathOrError(ruleContext);
+      PathFragment shExecutable =
+          ShToolchain.getPathOrError(getExecutionPlatform(execGroupUnchecked, ruleContext));
       BashCommandConstructor constructor =
           CommandHelper.buildBashCommandConstructor(
               executionInfo, shExecutable, helperScriptSuffix);
@@ -662,13 +683,11 @@ private void registerStarlarkAction(
       }
     }
 
-    if (execGroupUnchecked != Starlark.NONE) {
-      String execGroup = (String) execGroupUnchecked;
-      if (!StarlarkExecGroupCollection.isValidGroupName(execGroup)
-          || !ruleContext.hasToolchainContext(execGroup)) {
-        throw Starlark.errorf("Action declared for non-existent exec group '%s'.", execGroup);
-      }
-      builder.setExecGroup(execGroup);
+    if (execGroupUnchecked == Starlark.NONE) {
+      builder.setExecGroup(ExecGroup.DEFAULT_EXEC_GROUP_NAME);
+    } else {
+      verifyExecGroup(execGroupUnchecked, ruleContext);
+      builder.setExecGroup((String) execGroupUnchecked);
     }
 
     if (shadowedActionUnchecked != Starlark.NONE) {

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java
@@ -1018,7 +1018,9 @@ public Tuple resolveCommand(
                 String.class,
                 String.class,
                 "execution_requirements"));
-    PathFragment shExecutable = ShToolchain.getPathOrError(ruleContext);
+    // TODO(b/234923262): Take exec_group into consideration instead of using the default
+    // exec_group.
+    PathFragment shExecutable = ShToolchain.getPathOrError(ruleContext.getExecutionPlatform());
 
     BashCommandConstructor constructor =
         CommandHelper.buildBashCommandConstructor(

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java
@@ -398,6 +398,7 @@ private TestParams createTestAction(int shards)
                 && testConfiguration.cancelConcurrentTests();
 
         boolean splitCoveragePostProcessing = testConfiguration.splitCoveragePostProcessing();
+        // TODO(b/234923262): Take exec_group into consideration when selecting sh tools
         TestRunnerAction testRunnerAction =
             new TestRunnerAction(
                 getOwner(),
@@ -421,7 +422,7 @@ private TestParams createTestAction(int shards)
                 ruleContext.getWorkspaceName(),
                 (!isUsingTestWrapperInsteadOfTestSetupScript
                         || executionSettings.needsShell(isExecutedOnWindows))
-                    ? ShToolchain.getPathOrError(ruleContext)
+                    ? ShToolchain.getPathOrError(ruleContext.getExecutionPlatform())
                     : null,
                 cancelConcurrentTests,
                 splitCoveragePostProcessing,