incompatible_windows_native_test_wrapper: remove

Remove this flag and every occurrence of it.
It was flipped in Bazel 1.0, so nobody should be
using it in production.

Fixes #6622

RELNOTES[INC]: The --[no]incompatible_windows_native_test_wrapper flag is no longer supported. It was flipped in Bazel 1.0

PiperOrigin-RevId: 289840656

site/docs/windows.md

@@ -38,23 +38,13 @@ details.
 <a name="bazel-build-without-bash"></a>
 ### `bazel build` without Bash
 
-With **Bazel 0.26.0** and the `--incompatible_windows_native_test_wrapper` flag,
-you can **build Python and all C++ rules without Bash**. Use the
-`--shell_executable=""` flag to tell Bazel not to look for Bash.
+Bazel versions before 1.0 used to require Bash to build some rules.
 
-With **Bazel 0.25.0** and the `--incompatible_windows_native_test_wrapper` flag,
-you can **build Java and `cc_binary` rules without Bash** (but not `cc_test`).
-Use the `--shell_executable=""` flag to tell Bazel not to look for Bash.
+Starting with Bazel 1.0, you can build any rule without Bash unless it is a:
 
-With **Bazel 0.24.x and older** you need Bash to build any rule.
-
-With every Bazel version, you **still need Bash** if a rule in your build or in
-some external repository:
-
-- is a `genrule`, because genrules execute Bash commands
-- is a `sh_binary` or `sh_test` rule, because these inherently need Bash
-- is a Starlark rule that uses `ctx.actions.run_shell()` or
-  `ctx.resolve_command()`
+- `genrule`, because genrules execute Bash commands
+- `sh_binary` or `sh_test` rule, because these inherently need Bash
+- Starlark rule that uses `ctx.actions.run_shell()` or `ctx.resolve_command()`
 
 However, `genrule` is often used for simple tasks like
 [copying a file](https://github.com/bazelbuild/bazel-skylib/blob/master/rules/copy_file.bzl)
@@ -67,28 +57,22 @@ When built on Windows, **these rules do not require Bash**.
 <a name="bazel-test-without-bash"></a>
 ### `bazel test` without Bash
 
-With **Bazel 0.25.0 or newer** and the
-`--incompatible_windows_native_test_wrapper` flag, you can `bazel test` rules
-without Bash, i.e.
-`bazel test --incompatible_windows_native_test_wrapper //foo:bar_test` works
-even if there's no MSYS2 installed.
+Bazel versions before 1.0 used to require Bash to `bazel test` anything.
 
-With **Bazel 0.24.x and older** you cannot use this flag, and need Bash (MSYS2)
-to run any `bazel test`.
+Starting with Bazel 1.0, you can test any rule without Bash, except when:
 
-In Bazel 0.25.0 and Bazel 0.26.0, the
-`--incompatible_windows_native_test_wrapper` flag is **off** be default. We plan
-to enable it by default starting with Bazel 0.27.0, and plan to remove support
-for the flag in Bazel 0.28.0. Follow issue
-[#6622](https://github.com/bazelbuild/bazel/pull/6622) for updates.
+- you use `--run_under`
+- the test rule itself requires Bash (because its executable is a shell script)
 
 <a name="bazel-run-without-bash"></a>
 ### `bazel run` without Bash
 
-With Bazel 0.25.0 you still need Bash (MSYS2) to `bazel run //foo:bin` anything.
+Bazel versions before 1.0 used to require Bash to `bazel run` anything.
+
+Starting with Bazel 1.0, you can run any rule without Bash, except when:
 
-Removing this requirement is one of our top priorities. Follow issue
-[#8240](https://github.com/bazelbuild/bazel/pull/8240) for updates.
+- you use `--run_under` or `--script_path`
+- the test rule itself requires Bash (because its executable is a shell script)
 
 <a name="sh-rules-without-bash"></a>
 ### `sh_binary` and `sh_*` rules, and `ctx.actions.run_shell()` without Bash
@@ -99,7 +83,7 @@ applies not only to rules in your project, but to rules in any of the external
 repositories your project depends on (even transitively).
 
 We may explore the option to use Windows Subsystem for Linux (WSL) to build
-these rules, but as of 2019-05-07 it is not a priority for the Bazel-on-Windows
+these rules, but as of 2020-01-15 it is not a priority for the Bazel-on-Windows
 subteam.
 
 ## Setting environment variables

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

@@ -202,8 +202,7 @@ private TestParams createTestAction(int shards) {
     // 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 && testConfiguration.isUsingWindowsNativeTestWrapper();
+    final boolean isUsingTestWrapperInsteadOfTestSetupScript = isExecutedOnWindows;
 
     NestedSetBuilder<Artifact> inputsBuilder = NestedSetBuilder.stableOrder();
     inputsBuilder.addTransitive(
@@ -410,7 +409,6 @@ private TestParams createTestAction(int shards) {
                 inputs,
                 testRunfilesSupplier,
                 testActionExecutable,
-                isUsingTestWrapperInsteadOfTestSetupScript,
                 testXmlGeneratorExecutable,
                 collectCoverageScript,
                 testLog,

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

@@ -33,7 +33,6 @@
 import com.google.devtools.common.options.OptionDefinition;
 import com.google.devtools.common.options.OptionDocumentationCategory;
 import com.google.devtools.common.options.OptionEffectTag;
-import com.google.devtools.common.options.OptionMetadataTag;
 import com.google.devtools.common.options.OptionsParser;
 import com.google.devtools.common.options.OptionsParsingException;
 import com.google.devtools.common.options.TriState;
@@ -242,27 +241,6 @@ public static class TestOptions extends FragmentOptions {
     )
     public Label coverageReportGenerator;
 
-    @Option(
-        name = "incompatible_windows_native_test_wrapper",
-        // Design:
-        // https://github.com/laszlocsomor/proposals/blob/win-test-runner/designs/2018-07-18-windows-native-test-runner.md
-        documentationCategory = OptionDocumentationCategory.TESTING,
-        // Affects loading and analysis: this flag affects which target Bazel loads and creates test
-        // actions with on Windows.
-        effectTags = {
-          OptionEffectTag.LOADING_AND_ANALYSIS,
-          OptionEffectTag.TEST_RUNNER,
-        },
-        metadataTags = {
-          OptionMetadataTag.INCOMPATIBLE_CHANGE,
-          OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES,
-        },
-        defaultValue = "true",
-        help =
-            "On Windows: if true, uses the C++ test wrapper to run tests, otherwise uses "
-                + "tools/test/test-setup.sh as on other platforms. On other platforms: no-op.")
-    public boolean windowsNativeTestWrapper;
-
     @Override
     public FragmentOptions getHost() {
       TestOptions hostOptions = (TestOptions) getDefault();
@@ -337,10 +315,6 @@ public Label getCoverageReportGenerator(){
     return options.coverageReportGenerator;
   }
 
-  public boolean isUsingWindowsNativeTestWrapper() {
-    return options.windowsNativeTestWrapper;
-  }
-
   /**
    * @return number of times the given test should run. If the test doesn't match any of the
    *     filters, runs it once.

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

@@ -119,9 +119,6 @@ public class TestRunnerAction extends AbstractAction
   private final int runNumber;
   private final String workspaceName;
 
-  // Takes the value of the `--windows_native_test_wrapper` flag.
-  private final boolean useTestWrapperInsteadOfTestSetupSh;
-
   // Mutable state related to test caching. Lazily initialized: null indicates unknown.
   private Boolean unconditionalExecution;
 
@@ -159,7 +156,6 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
       NestedSet<Artifact> inputs,
       RunfilesSupplier runfilesSupplier,
       Artifact testSetupScript, // Must be in inputs
-      boolean useTestWrapperInsteadOfTestSetupSh,
       Artifact testXmlGeneratorScript, // Must be in inputs
       @Nullable Artifact collectCoverageScript, // Must be in inputs, if not null
       Artifact testLog,
@@ -184,7 +180,6 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
         configuration.getActionEnvironment());
     Preconditions.checkState((collectCoverageScript == null) == (coverageArtifact == null));
     this.testSetupScript = testSetupScript;
-    this.useTestWrapperInsteadOfTestSetupSh = useTestWrapperInsteadOfTestSetupSh;
     this.testXmlGeneratorScript = testXmlGeneratorScript;
     this.collectCoverageScript = collectCoverageScript;
     this.configuration = Preconditions.checkNotNull(configuration);
@@ -854,10 +849,6 @@ public Artifact getTestSetupScript() {
     return testSetupScript;
   }
 
-  public boolean isUsingTestWrapperInsteadOfTestSetupScript() {
-    return useTestWrapperInsteadOfTestSetupSh;
-  }
-
   public Artifact getTestXmlGeneratorScript() {
     return testXmlGeneratorScript;
   }

src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java

@@ -18,7 +18,6 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Lists;
 import com.google.common.io.ByteStreams;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
@@ -47,7 +46,6 @@
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.events.Reporter;
-import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.FileStatus;
@@ -372,29 +370,19 @@ private static BuildEventStreamProtos.TestResult.ExecutionInfo extractExecutionI
    * generate a test.xml file itself.
    */
   private Spawn createXmlGeneratingSpawn(TestRunnerAction action, SpawnResult result) {
-    List<String> args = Lists.newArrayList();
-    // TODO(ulfjack): This 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().getExecOS() == OS.WINDOWS
-    if (OS.getCurrent() == OS.WINDOWS && !action.isUsingTestWrapperInsteadOfTestSetupScript()) {
-      // TestActionBuilder constructs TestRunnerAction with a 'null' shell path only when we use the
-      // native test wrapper. Something clearly went wrong.
-      Preconditions.checkNotNull(action.getShExecutableMaybe(), "%s", action);
-      args.add(action.getShExecutableMaybe().getPathString());
-      args.add("-c");
-      args.add("$0 $*");
-    }
-    args.add(action.getTestXmlGeneratorScript().getExecPath().getCallablePathString());
-    args.add(action.getTestLog().getExecPathString());
-    args.add(action.getXmlOutputPath().getPathString());
-    args.add(Long.toString(result.getWallTime().orElse(Duration.ZERO).getSeconds()));
-    args.add(Integer.toString(result.exitCode()));
+    ImmutableList<String> args =
+        ImmutableList.of(
+            action.getTestXmlGeneratorScript().getExecPath().getCallablePathString(),
+            action.getTestLog().getExecPathString(),
+            action.getXmlOutputPath().getPathString(),
+            Long.toString(result.getWallTime().orElse(Duration.ZERO).getSeconds()),
+            Integer.toString(result.exitCode()));
 
     String testBinaryName =
         action.getExecutionSettings().getExecutable().getRootRelativePath().getCallablePathString();
     return new SimpleSpawn(
         action,
-        ImmutableList.copyOf(args),
+        args,
         ImmutableMap.of(
             "PATH", "/usr/bin:/bin",
             "TEST_SHARD_INDEX", Integer.toString(action.getShardNum()),

src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java

@@ -194,16 +194,6 @@ public static ImmutableList<String> expandedArgsFromAction(TestRunnerAction test
     // 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);
-    final boolean useTestWrapper = testAction.isUsingTestWrapperInsteadOfTestSetupScript();
-
-    if (executedOnWindows && !useTestWrapper) {
-      // TestActionBuilder constructs TestRunnerAction with a 'null' shell path only when we use the
-      // native test wrapper. Something clearly went wrong.
-      Preconditions.checkNotNull(testAction.getShExecutableMaybe(), "%s", testAction);
-      args.add(testAction.getShExecutableMaybe().getPathString());
-      args.add("-c");
-      args.add("$0 \"$@\"");
-    }
 
     Artifact testSetup = testAction.getTestSetupScript();
     args.add(testSetup.getExecPath().getCallablePathString());

src/test/py/bazel/test_wrapper_test.py

@@ -577,11 +577,8 @@ def _AssertXmlGeneratedByTestIsRetained(self, flags, split_xml=False):
       xml_contents = [line.strip() for line in f.readlines()]
     self.assertListEqual(xml_contents, ['leave this'])
 
-  # Test that the native test wrapper can run tests from external repositories.
+  # Test that we can run tests from external repositories.
   # See https://github.com/bazelbuild/bazel/issues/8088
-  # Unfortunately as of 2019-04-18 the legacy test wrapper (test-setup.sh) also
-  # has this bug, but I (@laszlocsomor) work on enabling the native test wrapper
-  # by default so fixing the legacy one seems to make little sense.
   def testRunningTestFromExternalRepo(self):
     rule_definition = [
         'load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")',
@@ -600,7 +597,6 @@ def testRunningTestFromExternalRepo(self):
         exit_code, _, stderr = self.RunBazel([
             'test',
             '-t-',
-            '--incompatible_windows_native_test_wrapper',
             '--shell_executable=',
             '--test_output=errors',
             '--verbose_failures',
@@ -611,9 +607,9 @@ def testRunningTestFromExternalRepo(self):
             exit_code, 0,
             ['flag=%s' % flag, 'target=%s' % target] + stderr)
 
-  def _RunTests(self, flags):
+  def testTestExecutionWithTestWrapperExe(self):
     self._CreateMockWorkspace()
-    flags = ['--noincompatible_windows_native_test_wrapper']
+    flags = ['--shell_executable=']
     self._AssertPassingTest(flags)
     self._AssertFailingTest(flags)
     self._AssertPrintingTest(flags)
@@ -629,13 +625,6 @@ def _RunTests(self, flags):
     self._AssertXmlGeneratedByTestIsRetained(flags, split_xml=False)
     self._AssertXmlGeneratedByTestIsRetained(flags, split_xml=True)
 
-  def testTestExecutionWithTestSetupSh(self):
-    self._RunTests(['--noincompatible_windows_native_test_wrapper'])
-
-  def testTestExecutionWithTestWrapperExe(self):
-    self._RunTests(
-        ['--incompatible_windows_native_test_wrapper', '--shell_executable='])
-
 
 if __name__ == '__main__':
   unittest.main()