Automated rollback of commit 17057fd.

*** Reason for rollback ***

Breaks internally

PiperOrigin-RevId: 697508217
Change-Id: I4cb398208a7de2977152d280b44a3d50d814817a

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

@@ -358,7 +358,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:template_expansion_exception",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_artifacts_known_event",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_report_action_factory",
-        "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration",
         "//src/main/java/com/google/devtools/build/lib/analysis:top_level_artifact_context",
         "//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_collection",
         "//src/main/java/com/google/devtools/build/lib/analysis:view_creation_failed_exception",

src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java

@@ -27,7 +27,6 @@
 import com.google.devtools.build.lib.actions.CommandLineLimits;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.PlatformOptions;
-import com.google.devtools.build.lib.analysis.test.TestConfiguration.TestOptions;
 import com.google.devtools.build.lib.buildeventstream.BuildEvent;
 import com.google.devtools.build.lib.buildeventstream.BuildEventIdUtil;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
@@ -166,13 +165,9 @@ public void reportInvalidOptions(EventHandler reporter) {
    * be inherited from the client environment.
    */
   private ActionEnvironment setupTestEnvironment() {
-    if (!buildOptions.contains(TestOptions.class)) {
-      // TestOptions have been trimmed.
-      return ActionEnvironment.EMPTY;
-    }
-    // Order doesn't matter here as ActionEnvironment sorts by key.
+    // We make a copy first to remove duplicate entries; last one wins.
     Map<String, String> testEnv = new HashMap<>();
-    for (Map.Entry<String, String> entry : buildOptions.get(TestOptions.class).testEnvironment) {
+    for (Map.Entry<String, String> entry : options.testEnvironment) {
       testEnv.put(entry.getKey(), entry.getValue());
     }
     return ActionEnvironment.split(testEnv);

src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java

@@ -34,9 +34,11 @@
 import com.google.devtools.common.options.OptionMetadataTag;
 import com.google.devtools.common.options.OptionsParsingException;
 import com.google.devtools.common.options.TriState;
+import java.util.AbstractMap.SimpleEntry;
 import java.util.ArrayList;
 import java.util.Comparator;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.TreeSet;
@@ -418,6 +420,24 @@ public ExecConfigurationDistinguisherSchemeConverter() {
       help = "Specifies a suffix to be added to the configuration directory.")
   public String platformSuffix;
 
+  // TODO(bazel-team): The test environment is actually computed in BlazeRuntime and this option
+  // is not read anywhere else. Thus, it should be in a different options class, preferably one
+  // specific to the "test" command or maybe in its own configuration fragment.
+  @Option(
+      name = "test_env",
+      converter = Converters.OptionalAssignmentConverter.class,
+      allowMultiple = true,
+      defaultValue = "null",
+      documentationCategory = OptionDocumentationCategory.TESTING,
+      effectTags = {OptionEffectTag.TEST_RUNNER},
+      help =
+          "Specifies additional environment variables to be injected into the test runner "
+              + "environment. Variables can be either specified by name, in which case its value "
+              + "will be read from the Bazel client environment, or by the name=value pair. "
+              + "This option can be used multiple times to specify several variables. "
+              + "Used only by the 'bazel test' command.")
+  public List<Map.Entry<String, String>> testEnvironment;
+
   // TODO(bazel-team): The set of available variables from the client environment for actions
   // is computed independently in CommandEnvironment to inject a more restricted client
   // environment to skyframe.
@@ -965,6 +985,20 @@ public ImmutableMap<String, String> getNormalizedCommandLineBuildVariables() {
         .collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue));
   }
 
+  // Normalizes list of map entries by keeping only the last entry for each key.
+  private static List<Map.Entry<String, String>> normalizeEntries(
+      List<Map.Entry<String, String>> entries) {
+    LinkedHashMap<String, String> normalizedEntries = new LinkedHashMap<>();
+    for (Map.Entry<String, String> entry : entries) {
+      normalizedEntries.put(entry.getKey(), entry.getValue());
+    }
+    // If we made no changes, return the same instance we got to reduce churn.
+    if (normalizedEntries.size() == entries.size()) {
+      return entries;
+    }
+    return normalizedEntries.entrySet().stream().map(SimpleEntry::new).collect(toImmutableList());
+  }
+
   // Sort the map entries by key.
   private static List<Map.Entry<String, String>> sortEntries(
       List<Map.Entry<String, String>> entries) {
@@ -1022,6 +1056,7 @@ public CoreOptions getNormalized() {
 
     result.actionEnvironment = normalizeEntries(actionEnvironment);
     result.hostActionEnvironment = normalizeEntries(hostActionEnvironment);
+    result.testEnvironment = normalizeEntries(testEnvironment);
     result.commandLineFlagAliases = sortEntries(normalizeEntries(commandLineFlagAliases));
 
     return result;

src/main/java/com/google/devtools/build/lib/analysis/config/FragmentOptions.java

@@ -20,10 +20,7 @@
 import com.google.devtools.common.options.Option;
 import com.google.devtools.common.options.Options;
 import com.google.devtools.common.options.OptionsBase;
-import java.util.AbstractMap;
-import java.util.LinkedHashMap;
 import java.util.List;
-import java.util.Map;
 import javax.annotation.Nullable;
 
 /** Command-line build options for a Blaze module. */
@@ -92,25 +89,6 @@ protected static ImmutableList<String> dedupAndSort(@Nullable List<String> value
     return result.equals(values) ? ImmutableList.copyOf(values) : result;
   }
 
-  /**
-   * Helper method for subclasses to normalize list of map entries by keeping only the last entry
-   * for each key. The order of the entries is preserved.
-   */
-  protected static List<Map.Entry<String, String>> normalizeEntries(
-      List<Map.Entry<String, String>> entries) {
-    LinkedHashMap<String, String> normalizedEntries = new LinkedHashMap<>();
-    for (Map.Entry<String, String> entry : entries) {
-      normalizedEntries.put(entry.getKey(), entry.getValue());
-    }
-    // If we made no changes, return the same instance we got to reduce churn.
-    if (normalizedEntries.size() == entries.size()) {
-      return entries;
-    }
-    return normalizedEntries.entrySet().stream()
-        .map(AbstractMap.SimpleEntry::new)
-        .collect(toImmutableList());
-  }
-
   /** Tracks limitations on referring to an option in a {@code config_setting}. */
   // TODO(bazel-team): There will likely also be a need to customize whether or not an option is
   // visible to users for setting on the command line (or perhaps even in a test of a Starlark

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

@@ -34,7 +34,6 @@
 import com.google.devtools.build.lib.packages.TestTimeout;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.RegexFilter;
-import com.google.devtools.common.options.Converters;
 import com.google.devtools.common.options.Option;
 import com.google.devtools.common.options.OptionDefinition;
 import com.google.devtools.common.options.OptionDocumentationCategory;
@@ -85,21 +84,6 @@ public static class TestOptions extends FragmentOptions {
             OptionsParser.getOptionDefinitionByName(
                 TestOptions.class, "experimental_retain_test_configuration_across_testonly"));
 
-    @Option(
-        name = "test_env",
-        converter = Converters.OptionalAssignmentConverter.class,
-        allowMultiple = true,
-        defaultValue = "null",
-        documentationCategory = OptionDocumentationCategory.TESTING,
-        effectTags = {OptionEffectTag.TEST_RUNNER},
-        help =
-            "Specifies additional environment variables to be injected into the test runner "
-                + "environment. Variables can be either specified by name, in which case its value "
-                + "will be read from the Bazel client environment, or by the name=value pair. "
-                + "This option can be used multiple times to specify several variables. "
-                + "Used only by the 'bazel test' command.")
-    public List<Map.Entry<String, String>> testEnvironment;
-
     @Option(
         name = "test_timeout",
         defaultValue = "-1",
@@ -362,13 +346,6 @@ public static class TestOptions extends FragmentOptions {
         effectTags = {OptionEffectTag.EXECUTION},
         help = "If true, Bazel will allow local tests to run.")
     public boolean allowLocalTests;
-
-    @Override
-    public TestOptions getNormalized() {
-      TestOptions result = (TestOptions) clone();
-      result.testEnvironment = normalizeEntries(testEnvironment);
-      return result;
-    }
   }
 
   private final TestOptions options;

src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java

@@ -32,7 +32,6 @@
 import com.google.devtools.build.lib.analysis.BuildInfoEvent;
 import com.google.devtools.build.lib.analysis.config.AdditionalConfigurationChangeEvent;
 import com.google.devtools.build.lib.analysis.config.CoreOptions;
-import com.google.devtools.build.lib.analysis.test.TestConfiguration.TestOptions;
 import com.google.devtools.build.lib.bazel.repository.downloader.DelegatingDownloader;
 import com.google.devtools.build.lib.bazel.repository.downloader.HttpDownloader;
 import com.google.devtools.build.lib.buildtool.BuildRequestOptions;
@@ -330,7 +329,7 @@ public void exit(AbruptExitException exception) {
         }
       }
       for (Map.Entry<String, String> entry :
-          options.getOptions(TestOptions.class).testEnvironment) {
+          options.getOptions(CoreOptions.class).testEnvironment) {
         if (entry.getValue() == null) {
           visibleTestEnv.add(entry.getKey());
         }

src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationStarlarkTest.java

@@ -42,28 +42,22 @@ public void testStarlarkWithTestEnvOptions() throws Exception {
         MyInfo = provider()
 
         def _test_rule_impl(ctx):
-            out = ctx.actions.declare_file(ctx.label.name)
-            ctx.actions.write(out, "exit 0", is_executable = True)
-            return [
-                DefaultInfo(executable = out),
-                MyInfo(test_env = ctx.configuration.test_env),
-            ]
-
-        my_test = rule(
+            return MyInfo(test_env = ctx.configuration.test_env)
+
+        test_rule = rule(
             implementation = _test_rule_impl,
             attrs = {},
-            test = True,
         )
         """);
 
     scratch.file(
         "examples/config_starlark/BUILD",
         """
-        load("//examples/rule:config_test.bzl", "my_test")
+        load("//examples/rule:config_test.bzl", "test_rule")
 
         package(default_visibility = ["//visibility:public"])
 
-        my_test(
+        test_rule(
             name = "my_target",
         )
         """);

src/test/java/com/google/devtools/build/lib/remote/BUILD

@@ -87,7 +87,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:constraints/constraint_constants",
         "//src/main/java/com/google/devtools/build/lib/analysis:server_directories",
         "//src/main/java/com/google/devtools/build/lib/analysis:symlink_entry",
-        "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration",
         "//src/main/java/com/google/devtools/build/lib/analysis/platform",
         "//src/main/java/com/google/devtools/build/lib/authandtls",
         "//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper",

src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java

@@ -36,7 +36,6 @@
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.ServerDirectories;
 import com.google.devtools.build.lib.analysis.config.CoreOptions;
-import com.google.devtools.build.lib.analysis.test.TestConfiguration.TestOptions;
 import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions;
 import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialHelperEnvironment;
 import com.google.devtools.build.lib.authandtls.credentialhelper.CredentialModule;
@@ -140,7 +139,6 @@ private static CommandEnvironment createTestCommandEnvironment(
     PackageOptions packageOptions = Options.getDefaults(PackageOptions.class);
     ClientOptions clientOptions = Options.getDefaults(ClientOptions.class);
     ExecutionOptions executionOptions = Options.getDefaults(ExecutionOptions.class);
-    TestOptions testOptions = Options.getDefaults(TestOptions.class);
 
     AuthAndTLSOptions authAndTLSOptions = Options.getDefaults(AuthAndTLSOptions.class);
 
@@ -152,7 +150,6 @@ private static CommandEnvironment createTestCommandEnvironment(
     when(options.getOptions(RemoteOptions.class)).thenReturn(remoteOptions);
     when(options.getOptions(AuthAndTLSOptions.class)).thenReturn(authAndTLSOptions);
     when(options.getOptions(ExecutionOptions.class)).thenReturn(executionOptions);
-    when(options.getOptions(TestOptions.class)).thenReturn(testOptions);
 
     String productName = "bazel";
     Scratch scratch = new Scratch(new InMemoryFileSystem(DigestHashFunction.SHA256));

src/test/shell/bazel/bazel_test_test.sh

@@ -1116,31 +1116,4 @@ EOF
   expect_log "-- Test exited prematurely (TEST_PREMATURE_EXIT_FILE exists) --"
 }
 
-function test_test_env_change_preserves_cache() {
-  local -r pkg="pkg${LINENO}"
-  mkdir -p "${pkg}"
-  cat > "$pkg/BUILD" <<'EOF'
-load(":defs.bzl", "my_rule")
-my_rule(
-  name = "my_rule",
-)
-EOF
-  cat > "$pkg/defs.bzl" <<'EOF'
-def _my_rule_impl(ctx):
-  print("my_rule is being analyzed")
-  out = ctx.actions.declare_file(ctx.label.name)
-  ctx.actions.write(out, "hi")
-  return [DefaultInfo(files = depset([out]))]
-my_rule = rule(_my_rule_impl)
-EOF
-
-  bazel build "${pkg}:my_rule" >$TEST_log 2>&1 \
-   || fail "expected build to pass"
-  expect_log "my_rule is being analyzed"
-
-  bazel build --test_env=FOO=bar "${pkg}:my_rule" >$TEST_log 2>&1 \
-   || fail "expected build to pass"
-  expect_not_log "my_rule is being analyzed"
-}
-
 run_suite "bazel test tests"