Use the execution platform to determine which genrule.cmd* to run.

Currently the host platform is used, which fails when using a Windows remote executor from a non-Windows host.

Also change GenRuleWindowsConfiguredTargetTest to run on every host platform.

Fixes #18584.

Closes #18590.

PiperOrigin-RevId: 538174757
Change-Id: I305caa706ce8dd0b9a2bee6eb9addcb2ff519f70

Author: tjgq

src/main/java/com/google/devtools/build/lib/analysis/BUILD

@@ -2181,6 +2181,12 @@ java_library(
 java_library(
     name = "constraints/constraint_constants",
     srcs = ["constraints/ConstraintConstants.java"],
+    deps = [
+        "//src/main/java/com/google/devtools/build/lib/analysis/platform",
+        "//src/main/java/com/google/devtools/build/lib/cmdline",
+        "//src/main/java/com/google/devtools/build/lib/util:os",
+        "//third_party:guava",
+    ],
 )
 
 java_library(

src/main/java/com/google/devtools/build/lib/analysis/ShToolchain.java

@@ -14,6 +14,8 @@
 
 package com.google.devtools.build.lib.analysis;
 
+import static com.google.devtools.build.lib.analysis.constraints.ConstraintConstants.OS_TO_CONSTRAINTS;
+
 import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
 import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
@@ -63,9 +65,7 @@ public static PathFragment getPathForPlatform(
 
     if (platformInfo != null) {
       for (OS os : ShellConfiguration.getShellExecutables().keySet()) {
-        if (platformInfo
-            .constraints()
-            .hasConstraintValue(ShellConfiguration.OS_TO_CONSTRAINTS.get(os))) {
+        if (platformInfo.constraints().hasConstraintValue(OS_TO_CONSTRAINTS.get(os))) {
           return ShellConfiguration.getShellExecutables().get(os);
         }
       }

src/main/java/com/google/devtools/build/lib/analysis/ShellConfiguration.java

@@ -13,14 +13,10 @@
 // limitations under the License.
 package com.google.devtools.build.lib.analysis;
 
-import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 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;
@@ -37,10 +33,6 @@ public class ShellConfiguration extends Fragment {
 
   private static Map<OS, PathFragment> shellExecutables;
 
-  private static final ConstraintSettingInfo OS_CONSTRAINT_SETTING =
-      ConstraintSettingInfo.create(
-          Label.parseCanonicalUnchecked("@platforms//os:os"));
-
   private static Function<Options, PathFragment> optionsBasedDefault;
 
   /**
@@ -65,35 +57,6 @@ public static void injectShellExecutableFinder(Map<OS, PathFragment> osToShellMa
     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.parseCanonicalUnchecked("@platforms//os:osx")))
-          .put(
-              OS.WINDOWS,
-              ConstraintValueInfo.create(
-                  OS_CONSTRAINT_SETTING,
-                  Label.parseCanonicalUnchecked("@platforms//os:windows")))
-          .put(
-              OS.FREEBSD,
-              ConstraintValueInfo.create(
-                  OS_CONSTRAINT_SETTING,
-                  Label.parseCanonicalUnchecked("@platforms//os:freebsd")))
-          .put(
-              OS.OPENBSD,
-              ConstraintValueInfo.create(
-                  OS_CONSTRAINT_SETTING,
-                  Label.parseCanonicalUnchecked("@platforms//os:openbsd")))
-          .put(
-              OS.UNKNOWN,
-              ConstraintValueInfo.create(
-                  OS_CONSTRAINT_SETTING,
-                  Label.parseCanonicalUnchecked("@platforms//os:none")))
-          .buildOrThrow();
 
   private final boolean useShBinaryStubScript;
 

src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintConstants.java

@@ -13,11 +13,51 @@
 // limitations under the License.
 package com.google.devtools.build.lib.analysis.constraints;
 
+import com.google.common.collect.ImmutableMap;
+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;
+
 /** Constants needed for use of the constraints system. */
 public final class ConstraintConstants {
 
   public static final String ENVIRONMENT_RULE = "environment";
 
+  private static final ConstraintSettingInfo OS_CONSTRAINT_SETTING =
+      ConstraintSettingInfo.create(
+          Label.parseCanonicalUnchecked("@platforms//os:os"));
+
+  // Standard mapping between OS and the corresponding platform constraints.
+  public static final ImmutableMap<OS, ConstraintValueInfo> OS_TO_CONSTRAINTS =
+      ImmutableMap.<OS, ConstraintValueInfo>builder()
+          .put(
+              OS.DARWIN,
+              ConstraintValueInfo.create(
+                  OS_CONSTRAINT_SETTING,
+                  Label.parseCanonicalUnchecked("@platforms//os:osx")))
+          .put(
+              OS.WINDOWS,
+              ConstraintValueInfo.create(
+                  OS_CONSTRAINT_SETTING,
+                  Label.parseCanonicalUnchecked("@platforms//os:windows")))
+          .put(
+              OS.FREEBSD,
+              ConstraintValueInfo.create(
+                  OS_CONSTRAINT_SETTING,
+                  Label.parseCanonicalUnchecked("@platforms//os:freebsd")))
+          .put(
+              OS.OPENBSD,
+              ConstraintValueInfo.create(
+                  OS_CONSTRAINT_SETTING,
+                  Label.parseCanonicalUnchecked("@platforms//os:openbsd")))
+          .put(
+              OS.UNKNOWN,
+              ConstraintValueInfo.create(
+                  OS_CONSTRAINT_SETTING,
+                  Label.parseCanonicalUnchecked("@platforms//os:none")))
+          .buildOrThrow();
+
   // No-op constructor to keep this from being instantiated.
   private ConstraintConstants() {}
 }

src/main/java/com/google/devtools/build/lib/rules/genrule/BUILD

@@ -22,6 +22,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/execution_transition_factory",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
+        "//src/main/java/com/google/devtools/build/lib/analysis:constraints/constraint_constants",
         "//src/main/java/com/google/devtools/build/lib/analysis:file_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis:make_variable_supplier",
         "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",

src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java

@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.rules.genrule;
 
 import static com.google.common.collect.ImmutableMap.toImmutableMap;
+import static com.google.devtools.build.lib.analysis.constraints.ConstraintConstants.OS_TO_CONSTRAINTS;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -94,8 +95,10 @@ enum CommandType {
   private static Pair<CommandType, String> determineCommandTypeAndAttribute(
       RuleContext ruleContext) {
     AttributeMap attributeMap = ruleContext.attributes();
-    // TODO(pcloudy): This should match the execution platform instead of using OS.getCurrent()
-    if (OS.getCurrent() == OS.WINDOWS) {
+    if (ruleContext
+        .getExecutionPlatform()
+        .constraints()
+        .hasConstraintValue(OS_TO_CONSTRAINTS.get(OS.WINDOWS))) {
       if (attributeMap.isAttributeValueExplicitlySpecified("cmd_ps")) {
         return Pair.of(CommandType.WINDOWS_POWERSHELL, "cmd_ps");
       }

src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/BUILD

@@ -15,15 +15,7 @@ filegroup(
 
 java_library(
     name = "GenruleTests_lib",
-    srcs = glob(
-               ["*.java"],
-               exclude = ["GenRuleWindowsConfiguredTargetTest.java"],
-           ) +
-           # If we are on windows add back the test
-           select({
-               "//conditions:default": [],
-               "//src/conditions:windows": ["GenRuleWindowsConfiguredTargetTest.java"],
-           }),
+    srcs = glob(["*.java"]),
     deps = [
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",

src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleWindowsConfiguredTargetTest.java

@@ -15,14 +15,14 @@
 package com.google.devtools.build.lib.bazel.rules.genrule;
 
 import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assume.assumeTrue;
 
 import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.analysis.ShToolchain;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
-import com.google.devtools.build.lib.util.OS;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -58,6 +58,20 @@ private static String getWindowsPath(Artifact artifact) {
     return artifact.getExecPathString().replace('/', '\\');
   }
 
+  @Before
+  public void assumeBazel() throws Exception {
+    // The cmd_{bash,bat,ps} attributes don't exist in Blaze.
+    assumeTrue(analysisMock.isThisBazel());
+  }
+
+  @Before
+  public void createWindowsPlatform() throws Exception {
+    scratch.file(
+        "platforms/BUILD",
+        "platform(name = 'windows', constraint_values = ['@platforms//os:windows'])");
+    useConfiguration("--host_platform=//platforms:windows");
+  }
+
   @Test
   public void testCmdBatchIsPreferred() throws Exception {
     scratch.file(
@@ -163,8 +177,7 @@ public void testCmdBashIsPreferred() throws Exception {
     assertThat(shellAction.getOutputs()).containsExactly(messageArtifact);
 
     String expected = "echo \"Hello, Bash cmd.\" >" + messageArtifact.getExecPathString();
-    assertThat(shellAction.getArguments().get(0))
-        .isEqualTo(ShToolchain.getPathForHost(targetConfig).getPathString());
+    assertThat(shellAction.getArguments().get(0)).isEqualTo("c:/tools/msys64/usr/bin/bash.exe");
     assertThat(shellAction.getArguments().get(1)).isEqualTo("-c");
     assertBashCommandEquals(expected, shellAction.getArguments().get(2));
   }
@@ -181,9 +194,11 @@ public void testMissingCmdAttributeError() throws Exception {
 
   @Test
   public void testMissingCmdAttributeErrorOnNonWindowsPlatform() throws Exception {
-    if (OS.getCurrent() == OS.WINDOWS) {
-      return;
-    }
+    scratch.overwriteFile(
+        "platforms/BUILD",
+        "platform(name = 'nonwindows', constraint_values = ['@platforms//os:linux'])");
+    useConfiguration("--host_platform=//platforms:nonwindows");
+
     checkError(
         "foo",
         "bar",