diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD index 71deae4ac94099..5a13c6dab4fab8 100644 --- a/src/main/java/com/google/devtools/build/lib/BUILD +++ b/src/main/java/com/google/devtools/build/lib/BUILD @@ -358,6 +358,7 @@ 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 0d7cb31cd93fda..00b2d01d88bec3 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,6 +27,7 @@ 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; @@ -165,9 +166,13 @@ public void reportInvalidOptions(EventHandler reporter) { * be inherited from the client environment. */ private ActionEnvironment setupTestEnvironment() { - // We make a copy first to remove duplicate entries; last one wins. + if (!buildOptions.contains(TestOptions.class)) { + // TestOptions have been trimmed. + return ActionEnvironment.EMPTY; + } + // Order doesn't matter here as ActionEnvironment sorts by key. Map testEnv = new HashMap<>(); - for (Map.Entry entry : options.testEnvironment) { + for (Map.Entry entry : buildOptions.get(TestOptions.class).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 b4e663454c365c..ea7500f7d34146 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,11 +34,9 @@ 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; @@ -420,24 +418,6 @@ 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. @@ -985,20 +965,6 @@ 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) { @@ -1056,7 +1022,6 @@ 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 b4c454d279143d..790a0f04bfa97e 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,7 +20,10 @@ 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. */ @@ -89,6 +92,25 @@ 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 7874dc6eb9ae1a..ea26b96b94f190 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,6 +34,7 @@ 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; @@ -84,6 +85,21 @@ 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", @@ -346,6 +362,13 @@ 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 6cb683e2d0fabd..edb9342224e0ed 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,6 +32,7 @@ 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; @@ -329,7 +330,7 @@ public void exit(AbruptExitException exception) { } } for (Map.Entry entry : - options.getOptions(CoreOptions.class).testEnvironment) { + options.getOptions(TestOptions.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 03a497bbb62e35..a8527d356f39d2 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,22 +42,28 @@ public void testStarlarkWithTestEnvOptions() throws Exception { MyInfo = provider() def _test_rule_impl(ctx): - return MyInfo(test_env = ctx.configuration.test_env) - - test_rule = rule( + 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( implementation = _test_rule_impl, attrs = {}, + test = True, ) """); scratch.file( "examples/config_starlark/BUILD", """ - load("//examples/rule:config_test.bzl", "test_rule") + load("//examples/rule:config_test.bzl", "my_test") package(default_visibility = ["//visibility:public"]) - test_rule( + my_test( 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 81a2d284a28c56..b7878000439c05 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD @@ -87,6 +87,7 @@ 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 11215c8bb4c261..6ae610218c26e3 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,6 +36,7 @@ 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; @@ -139,6 +140,7 @@ 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); @@ -150,6 +152,7 @@ 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 7d6d90d4f04cfb..c602d8ab10157a 100755 --- a/src/test/shell/bazel/bazel_test_test.sh +++ b/src/test/shell/bazel/bazel_test_test.sh @@ -1116,4 +1116,31 @@ 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"