diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD index 5a13c6dab4fab8..71deae4ac94099 100644 --- a/src/main/java/com/google/devtools/build/lib/BUILD +++ b/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", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java index 00b2d01d88bec3..0d7cb31cd93fda 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java +++ b/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 testEnv = new HashMap<>(); - for (Map.Entry entry : buildOptions.get(TestOptions.class).testEnvironment) { + for (Map.Entry entry : options.testEnvironment) { testEnv.put(entry.getKey(), entry.getValue()); } return ActionEnvironment.split(testEnv); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java index ea7500f7d34146..b4e663454c365c 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java +++ b/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> 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 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> normalizeEntries( + List> entries) { + LinkedHashMap normalizedEntries = new LinkedHashMap<>(); + for (Map.Entry 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> sortEntries( List> 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; diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/FragmentOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/FragmentOptions.java index 790a0f04bfa97e..b4c454d279143d 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/FragmentOptions.java +++ b/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 dedupAndSort(@Nullable List 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> normalizeEntries( - List> entries) { - LinkedHashMap normalizedEntries = new LinkedHashMap<>(); - for (Map.Entry 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 diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java index ea26b96b94f190..7874dc6eb9ae1a 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java +++ b/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> 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; diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java index edb9342224e0ed..6cb683e2d0fabd 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java +++ b/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 entry : - options.getOptions(TestOptions.class).testEnvironment) { + options.getOptions(CoreOptions.class).testEnvironment) { if (entry.getValue() == null) { visibleTestEnv.add(entry.getKey()); } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationStarlarkTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationStarlarkTest.java index a8527d356f39d2..03a497bbb62e35 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationStarlarkTest.java +++ b/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", ) """); diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD index b7878000439c05..81a2d284a28c56 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/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", diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java index 6ae610218c26e3..11215c8bb4c261 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteModuleTest.java +++ b/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)); diff --git a/src/test/shell/bazel/bazel_test_test.sh b/src/test/shell/bazel/bazel_test_test.sh index c602d8ab10157a..7d6d90d4f04cfb 100755 --- a/src/test/shell/bazel/bazel_test_test.sh +++ b/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"