Use the execution platform instead of the host platform to decide whether to do Windows-specific things in a few places.

Note: tests based on BuildViewTestCase never set the execution platform, so after this change they effectively always act as if running on non-Windows. This should be fixed but likely has a large blast radius, so instead I've fixed the test assertions for now.
PiperOrigin-RevId: 555424062
Change-Id: Ice82412832ab79452b43e2e6e49015a87460d854

Author: tjgq

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

@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.analysis;
 
 import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.devtools.build.lib.analysis.constraints.ConstraintConstants.OS_TO_CONSTRAINTS;
 import static com.google.devtools.build.lib.analysis.test.ExecutionInfo.DEFAULT_TEST_RUNNER_EXEC_GROUP;
 import static com.google.devtools.build.lib.packages.ExecGroup.DEFAULT_EXEC_GROUP_NAME;
 
@@ -599,10 +600,8 @@ public Artifact createOutputArtifact(OutputFile out) {
    */
   public Artifact createOutputArtifactScript() {
     Target target = getTarget();
-    // TODO(laszlocsomor): Use the execution platform, not the host platform.
-    boolean isExecutedOnWindows = OS.getCurrent() == OS.WINDOWS;
 
-    String fileExtension = isExecutedOnWindows ? ".cmd" : ".sh";
+    String fileExtension = isExecutedOnWindows() ? ".cmd" : ".sh";
 
     PathFragment rootRelativePath =
         getPackageDirectory().getRelative(PathFragment.create(target.getName() + fileExtension));
@@ -1629,6 +1628,13 @@ public boolean isTestOnlyTarget() {
     return attributes().has("testonly", Type.BOOLEAN) && attributes().get("testonly", Type.BOOLEAN);
   }
 
+  /** Returns true if the execution platform is Windows. */
+  public boolean isExecutedOnWindows() {
+    return getExecutionPlatform()
+        .constraints()
+        .hasConstraintValue(OS_TO_CONSTRAINTS.get(OS.WINDOWS));
+  }
+
   /**
    * Returns true if {@code label} is visible from {@code prerequisite}.
    *

src/main/java/com/google/devtools/build/lib/analysis/actions/LauncherFileWriteAction.java

@@ -13,20 +13,20 @@
 // limitations under the License.
 package com.google.devtools.build.lib.analysis.actions;
 
+import static com.google.common.base.Preconditions.checkState;
+
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.io.ByteStreams;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
-import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.util.Fingerprint;
-import com.google.devtools.build.lib.util.OS;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import java.io.IOException;
 import java.io.InputStream;
@@ -46,37 +46,35 @@ public final class LauncherFileWriteAction extends AbstractFileWriteAction {
 
   private final LaunchInfo launchInfo;
   private final Artifact launcher;
+  private final boolean isExecutedOnWindows;
 
   /** Creates a new {@link LauncherFileWriteAction}, registering it with the {@code ruleContext}. */
   public static void createAndRegister(
       RuleContext ruleContext, Artifact output, LaunchInfo launchInfo) {
     ruleContext.registerAction(
         new LauncherFileWriteAction(
-            ruleContext.getActionOwner(),
-            output,
-            ruleContext.getPrerequisiteArtifact("$launcher"),
-            launchInfo));
+            ruleContext, output, ruleContext.getPrerequisiteArtifact("$launcher"), launchInfo));
   }
 
   /** Creates a new {@code LauncherFileWriteAction}. */
   private LauncherFileWriteAction(
-      ActionOwner owner, Artifact output, Artifact launcher, LaunchInfo launchInfo) {
+      RuleContext ruleContext, Artifact output, Artifact launcher, LaunchInfo launchInfo) {
     super(
-        owner,
+        ruleContext.getActionOwner(),
         NestedSetBuilder.create(Order.STABLE_ORDER, Preconditions.checkNotNull(launcher)),
         output,
-        /*makeExecutable=*/ true);
+        /* makeExecutable= */ true);
     this.launcher = launcher; // already null-checked in the superclass c'tor
     this.launchInfo = Preconditions.checkNotNull(launchInfo);
+    this.isExecutedOnWindows = ruleContext.isExecutedOnWindows();
   }
 
   @Override
   public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) {
-    // TODO(laszlocsomor): make this code check for the execution platform, not the host platform,
-    // once Bazel supports distinguishing between the two.
-    // OS.getCurrent() returns the host platform, not the execution platform, which is fine in a
-    // single-machine execution environment, but problematic with remote execution.
-    Preconditions.checkState(OS.getCurrent() == OS.WINDOWS);
+    // TODO(tjgq): Move this check into createAndRegister.
+    // This requires fixing many unit tests that don't appropriately set the execution platform
+    // when running on Windows.
+    checkState(isExecutedOnWindows);
     return out -> {
       try (InputStream in = ctx.getInputPath(this.launcher).getInputStream()) {
         ByteStreams.copy(in, out);

src/main/java/com/google/devtools/build/lib/analysis/test/AnalysisTestActionBuilder.java

@@ -18,7 +18,6 @@
 import com.google.common.base.Splitter;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
-import com.google.devtools.build.lib.util.OS;
 
 /**
  * Helper for writing test actions for analysis test rules. Analysis test rules are restricted to
@@ -37,25 +36,23 @@ private AnalysisTestActionBuilder() {}
    */
   public static void writeAnalysisTestAction(
       RuleContext ruleContext, AnalysisTestResultInfo testResultInfo) {
-    // TODO(laszlocsomor): Use the execution platform, not the host platform.
-    boolean isExecutedOnWindows = OS.getCurrent() == OS.WINDOWS;
     String escapedMessage =
-        isExecutedOnWindows
+        ruleContext.isExecutedOnWindows()
             ? testResultInfo.getMessage().replace("%", "%%")
             // Prefix each character with \ (double-escaped; once in the string, once in the
             // replacement sequence, which allows backslash-escaping literal "$"). "." is put in
             // parentheses because ErrorProne is overly vigorous about objecting to "." as an
             // always-matching regex (b/201772278).
             : testResultInfo.getMessage().replaceAll("(.)", "\\\\$1");
     StringBuilder sb = new StringBuilder();
-    if (isExecutedOnWindows) {
+    if (ruleContext.isExecutedOnWindows()) {
       sb.append("@echo off\n");
     }
     for (String line : Splitter.on("\n").split(escapedMessage)) {
       sb.append("echo ").append(line).append("\n");
     }
     sb.append("exit ");
-    if (isExecutedOnWindows) {
+    if (ruleContext.isExecutedOnWindows()) {
       sb.append("/b ");
     }
     sb.append(testResultInfo.getSuccess() ? "0" : "1");

src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java

@@ -49,7 +49,6 @@
 import com.google.devtools.build.lib.packages.TestTimeout;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
-import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
@@ -224,11 +223,7 @@ private TestParams createTestAction(int shards)
     AnalysisEnvironment env = ruleContext.getAnalysisEnvironment();
     ArtifactRoot root = ruleContext.getTestLogsDirectory();
 
-    // TODO(laszlocsomor), TODO(ulfjack): `isExecutedOnWindows` should use the execution platform,
-    // not the host platform. Once Bazel can tell apart these platforms, fix the right side of this
-    // initialization.
-    final boolean isExecutedOnWindows = OS.getCurrent() == OS.WINDOWS;
-    final boolean isUsingTestWrapperInsteadOfTestSetupScript = isExecutedOnWindows;
+    final boolean isUsingTestWrapperInsteadOfTestSetupScript = ruleContext.isExecutedOnWindows();
 
     NestedSetBuilder<Artifact> inputsBuilder = NestedSetBuilder.stableOrder();
     inputsBuilder.addTransitive(
@@ -468,7 +463,7 @@ private TestParams createTestAction(int shards)
                 config,
                 ruleContext.getWorkspaceName(),
                 (!isUsingTestWrapperInsteadOfTestSetupScript
-                        || executionSettings.needsShell(isExecutedOnWindows))
+                        || executionSettings.needsShell(ruleContext.isExecutedOnWindows()))
                     ? ShToolchain.getPathForPlatform(
                         ruleContext.getConfiguration(), ruleContext.getExecutionPlatform())
                     : null,
@@ -481,7 +476,8 @@ private TestParams createTestAction(int shards)
                 MoreObjects.firstNonNull(
                     Allowlist.fetchPackageSpecificationProviderOrNull(
                         ruleContext, "external_network"),
-                    EMPTY_PACKAGES_PROVIDER));
+                    EMPTY_PACKAGES_PROVIDER),
+                ruleContext.isExecutedOnWindows());
 
         testOutputs.addAll(testRunnerAction.getSpawnOutputs());
         testOutputs.addAll(testRunnerAction.getOutputs());

src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java

@@ -144,6 +144,8 @@ public class TestRunnerAction extends AbstractAction
   private final int runNumber;
   private final String workspaceName;
 
+  private final boolean isExecutedOnWindows;
+
   /**
    * Cached test result status used to minimize disk accesses. This field is set when test status is
    * retrieved from disk or saved to disk. This field is null if it has not been set yet. This field
@@ -211,7 +213,8 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
       boolean splitCoveragePostProcessing,
       NestedSetBuilder<Artifact> lcovMergerFilesToRun,
       RunfilesSupplier lcovMergerRunfilesSupplier,
-      PackageSpecificationProvider networkAllowlist) {
+      PackageSpecificationProvider networkAllowlist,
+      boolean isExecutedOnWindows) {
     super(
         owner,
         inputs,
@@ -302,6 +305,12 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
             getUndeclaredOutputsDir(),
             undeclaredOutputsAnnotationsDir,
             baseDir.getRelative("test_attempts"));
+
+    this.isExecutedOnWindows = isExecutedOnWindows;
+  }
+
+  public boolean isExecutedOnWindows() {
+    return isExecutedOnWindows;
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/analysis/test/TestStrategy.java

@@ -43,7 +43,6 @@
 import com.google.devtools.build.lib.server.FailureDetails.TestAction;
 import com.google.devtools.build.lib.server.FailureDetails.TestAction.Code;
 import com.google.devtools.build.lib.util.Fingerprint;
-import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.io.OutErr;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -211,10 +210,6 @@ public static ImmutableList<String> getArgs(TestRunnerAction testAction)
   public static ImmutableList<String> expandedArgsFromAction(TestRunnerAction testAction)
       throws CommandLineExpansionException, InterruptedException {
     List<String> args = Lists.newArrayList();
-    // TODO(ulfjack): `executedOnWindows` is incorrect for remote execution, where we need to
-    // consider the target configuration, not the machine Bazel happens to run on. Change this to
-    // something like: testAction.getConfiguration().getTargetOS() == OS.WINDOWS
-    final boolean executedOnWindows = (OS.getCurrent() == OS.WINDOWS);
 
     Artifact testSetup = testAction.getTestSetupScript();
     args.add(testSetup.getExecPath().getCallablePathString());
@@ -227,7 +222,7 @@ public static ImmutableList<String> expandedArgsFromAction(TestRunnerAction test
 
     // Insert the command prefix specified by the "--run_under=<command-prefix>" option, if any.
     if (execSettings.getRunUnder() != null) {
-      addRunUnderArgs(testAction, args, executedOnWindows);
+      addRunUnderArgs(testAction, args);
     }
 
     // Execute the test using the alias in the runfiles tree, as mandated by the Test Encyclopedia.
@@ -236,13 +231,12 @@ public static ImmutableList<String> expandedArgsFromAction(TestRunnerAction test
     return ImmutableList.copyOf(args);
   }
 
-  private static void addRunUnderArgs(
-      TestRunnerAction testAction, List<String> args, boolean executedOnWindows) {
+  private static void addRunUnderArgs(TestRunnerAction testAction, List<String> args) {
     TestTargetExecutionSettings execSettings = testAction.getExecutionSettings();
     if (execSettings.getRunUnderExecutable() != null) {
       args.add(execSettings.getRunUnderExecutable().getRunfilesPath().getCallablePathString());
     } else {
-      if (execSettings.needsShell(executedOnWindows)) {
+      if (execSettings.needsShell(testAction.isExecutedOnWindows())) {
         // TestActionBuilder constructs TestRunnerAction with a 'null' shell only when none is
         // required. Something clearly went wrong.
         Preconditions.checkNotNull(testAction.getShExecutableMaybe(), "%s", testAction);

src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShBinary.java

@@ -30,7 +30,6 @@
 import com.google.devtools.build.lib.analysis.test.InstrumentedFilesCollector;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
-import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import javax.annotation.Nullable;
 
@@ -70,7 +69,7 @@ public ConfiguredTarget create(RuleContext ruleContext)
             ruleContext.getConfiguration().legacyExternalRunfiles());
 
     Artifact mainExecutable =
-        (OS.getCurrent() == OS.WINDOWS) ? launcherForWindows(ruleContext, symlink, src) : symlink;
+        ruleContext.isExecutedOnWindows() ? launcherForWindows(ruleContext, symlink, src) : symlink;
     if (!symlink.equals(mainExecutable)) {
       filesToBuildBuilder.add(mainExecutable);
       runfilesBuilder.addArtifact(symlink);

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

@@ -23,7 +23,6 @@ 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",
@@ -34,7 +33,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/packages",
         "//src/main/java/com/google/devtools/build/lib/util",
         "//src/main/java/com/google/devtools/build/lib/util:filetype",
-        "//src/main/java/com/google/devtools/build/lib/util:os",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//third_party:guava",
         "//third_party:jsr305",

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

@@ -15,7 +15,6 @@
 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;
@@ -50,7 +49,6 @@
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.packages.Type;
 import com.google.devtools.build.lib.util.FileTypeSet;
-import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.OnDemandString;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -95,10 +93,7 @@ enum CommandType {
   private static Pair<CommandType, String> determineCommandTypeAndAttribute(
       RuleContext ruleContext) {
     AttributeMap attributeMap = ruleContext.attributes();
-    if (ruleContext
-        .getExecutionPlatform()
-        .constraints()
-        .hasConstraintValue(OS_TO_CONSTRAINTS.get(OS.WINDOWS))) {
+    if (ruleContext.isExecutedOnWindows()) {
       if (attributeMap.isAttributeValueExplicitlySpecified("cmd_ps")) {
         return Pair.of(CommandType.WINDOWS_POWERSHELL, "cmd_ps");
       }

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

@@ -19,8 +19,10 @@ java_library(
         ["*.java"],
     ),
     deps = [
+        "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
         "//src/test/java/com/google/devtools/build/lib/analysis/util",
+        "//third_party:guava",
         "//third_party:junit4",
         "//third_party:truth",
     ],

src/test/java/com/google/devtools/build/lib/bazel/rules/sh/BazelShTestConfiguredTargetTest.java

@@ -15,30 +15,56 @@
 
 import static com.google.common.truth.Truth.assertThat;
 
+import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.test.TestRunnerAction;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
 /** Tests for sh_test configured target. */
 @RunWith(JUnit4.class)
 public class BazelShTestConfiguredTargetTest extends BuildViewTestCase {
+
+  @Before
+  public void setUp() throws Exception {
+    scratch.file("BUILD", "sh_test(name = 'test', srcs = ['test.sh'])");
+  }
+
   @Test
   public void testCoverageOutputGenerator() throws Exception {
-    scratch.file("sh/test/BUILD", "sh_test(name = 'foo_test', srcs = ['foo_test.sh'])");
     reporter.removeHandler(failFastHandler);
-    ConfiguredTarget ct = getConfiguredTarget("//sh/test:foo_test");
+    ConfiguredTarget ct = getConfiguredTarget("//:test");
     assertThat(getRuleContext(ct).getPrerequisite(":lcov_merger")).isNull();
   }
 
   @Test
   public void testCoverageOutputGeneratorCoverageMode() throws Exception {
     useConfiguration("--collect_code_coverage");
-    scratch.file("sh/test/BUILD", "sh_test(name = 'foo_test', srcs = ['foo_test.sh'])");
     reporter.removeHandler(failFastHandler);
-    ConfiguredTarget ct = getConfiguredTarget("//sh/test:foo_test");
+    ConfiguredTarget ct = getConfiguredTarget("//:test");
     assertThat(getRuleContext(ct).getPrerequisite(":lcov_merger").getLabel().toString())
         .isEqualTo("@bazel_tools//tools/test:lcov_merger");
   }
+
+  @Test
+  public void testNonWindowsWrapper() throws Exception {
+    assertThat(getTestRunnerAction("//:test").getArguments().get(0)).endsWith("test-setup.sh");
+  }
+
+  @Test
+  public void testWindowsWrapper() throws Exception {
+    scratch.file(
+        "platforms/BUILD",
+        "platform(name = 'windows', constraint_values = ['@platforms//os:windows'])");
+    useConfiguration("--host_platform=//platforms:windows");
+
+    assertThat(getTestRunnerAction("//:test").getArguments().get(0)).endsWith("test_wrapper_bin");
+  }
+
+  private TestRunnerAction getTestRunnerAction(String label) throws Exception {
+    return (TestRunnerAction) Iterables.getOnlyElement(getActions(label, TestRunnerAction.class));
+  }
 }