From 44c30aceec076a0c25f506508704df0b9aeb6578 Mon Sep 17 00:00:00 2001 From: twigg Date: Wed, 8 Dec 2021 15:57:11 -0800 Subject: [PATCH] Add value equals and hashCode to BuildConfigurationValue Note that they only incorporate the values that are not wholly dependent on other values and not server global. To minimize chance for error here, a factory method was added to BuildConfiguration that clearly takes a set of independent, varying values (which should be part of equality) and then a constrained set of server global values (which should not be part of equality). This is so re-computation of BuildConfigurationValue by BuildConfigurationFunction that results in an equal BuildConfigurationValue avoids full Skyframe invalidation of all ConfiguredTargetFunction computations. PiperOrigin-RevId: 415119618 --- .../google/devtools/build/lib/analysis/BUILD | 2 + .../analysis/ConfiguredRuleClassProvider.java | 28 ++++--- .../config/BuildConfigurationValue.java | 81 +++++++++++++++++-- .../lib/analysis/config/FragmentFactory.java | 5 +- .../bazel/rules/BazelRuleClassProvider.java | 3 +- .../skyframe/BuildConfigurationFunction.java | 46 ++--------- .../java/com/google/devtools/build/lib/BUILD | 3 +- .../config/BuildConfigurationValueTest.java | 20 +++++ .../devtools/build/lib/analysis/util/BUILD | 1 + .../analysis/util/ConfigurationTestCase.java | 48 +++++++++-- .../rules/BazelRuleClassProviderTest.java | 2 +- .../lib/runtime/BuildEventStreamerTest.java | 32 ++++++-- 12 files changed, 192 insertions(+), 79 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index bc52780189d085..56de81fc1d0232 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -1524,6 +1524,8 @@ java_library( ":config/core_options", ":config/fragment", ":config/fragment_class_set", + ":config/fragment_factory", + ":config/fragment_registry", ":config/invalid_configuration_exception", ":config/run_under", ":platform_options", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java index bee3bc7be8655b..296ddf68e2d713 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java @@ -64,7 +64,6 @@ import java.io.InputStream; import java.io.OutputStream; import java.lang.reflect.Constructor; -import java.lang.reflect.InvocationTargetException; import java.util.ArrayList; import java.util.HashMap; import java.util.List; @@ -72,6 +71,7 @@ import java.util.Optional; import java.util.Set; import java.util.TreeSet; +import java.util.function.Function; import java.util.zip.ZipEntry; import java.util.zip.ZipInputStream; import javax.annotation.Nullable; @@ -86,7 +86,8 @@ * configuration options is guaranteed not to change over the life time of the Blaze server. */ // This class has no subclasses except those created by the evil that is mockery. -public /*final*/ class ConfiguredRuleClassProvider implements RuleClassProvider { +public /*final*/ class ConfiguredRuleClassProvider + implements RuleClassProvider, BuildConfigurationValue.GlobalStateProvider { /** * A coherent set of options, fragments, aspects and rules; each of these may declare a dependency @@ -158,7 +159,7 @@ public static final class Builder implements RuleDefinitionEnvironment { private final ImmutableList.Builder symlinkDefinitions = ImmutableList.builder(); private final Set reservedActionMnemonics = new TreeSet<>(); - private BuildConfigurationValue.ActionEnvironmentProvider actionEnvironmentProvider = + private Function actionEnvironmentProvider = (BuildOptions options) -> ActionEnvironment.EMPTY; private ConstraintSemantics constraintSemantics = new RuleContextConstraintSemantics(); @@ -327,7 +328,7 @@ public Builder addReservedActionMnemonic(String mnemonic) { } public Builder setActionEnvironmentProvider( - BuildConfigurationValue.ActionEnvironmentProvider actionEnvironmentProvider) { + Function actionEnvironmentProvider) { this.actionEnvironmentProvider = actionEnvironmentProvider; return this; } @@ -422,10 +423,7 @@ private static RuleConfiguredTargetFactory createFactory( try { Constructor ctor = factoryClass.getConstructor(); return ctor.newInstance(); - } catch (NoSuchMethodException - | IllegalAccessException - | InstantiationException - | InvocationTargetException e) { + } catch (ReflectiveOperationException e) { throw new IllegalStateException(e); } } @@ -440,7 +438,8 @@ private void commitRuleDefinition(Class definitionClas RuleDefinition.Metadata metadata = instance.getMetadata(); checkArgument( ruleClassMap.get(metadata.name()) == null, - "The rule " + metadata.name() + " was committed already, use another name"); + "The rule %s was committed already, use another name", + metadata.name()); List> ancestors = metadata.ancestors(); @@ -649,7 +648,7 @@ public Builder setNetworkAllowlistForTests(Label allowlist) { private final ImmutableSet reservedActionMnemonics; - private final BuildConfigurationValue.ActionEnvironmentProvider actionEnvironmentProvider; + private final Function actionEnvironmentProvider; private final ImmutableMap> configurationFragmentMap; @@ -682,7 +681,7 @@ private ConfiguredRuleClassProvider( ImmutableList starlarkBootstraps, ImmutableList symlinkDefinitions, ImmutableSet reservedActionMnemonics, - BuildConfigurationValue.ActionEnvironmentProvider actionEnvironmentProvider, + Function actionEnvironmentProvider, ConstraintSemantics constraintSemantics, ThirdPartyLicenseExistencePolicy thirdPartyLicenseExistencePolicy, @Nullable Label networkAllowlistForTests) { @@ -761,6 +760,7 @@ public NativeAspectClass getNativeAspectClass(String key) { return nativeAspectClassMap.get(key); } + @Override public FragmentRegistry getFragmentRegistry() { return fragmentRegistry; } @@ -910,11 +910,13 @@ public ThirdPartyLicenseExistencePolicy getThirdPartyLicenseExistencePolicy() { } /** Returns a reserved set of action mnemonics. These cannot be used from a Starlark action. */ + @Override public ImmutableSet getReservedActionMnemonics() { return reservedActionMnemonics; } - public BuildConfigurationValue.ActionEnvironmentProvider getActionEnvironmentProvider() { - return actionEnvironmentProvider; + @Override + public ActionEnvironment getActionEnvironment(BuildOptions buildOptions) { + return actionEnvironmentProvider.apply(buildOptions); } } 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 1fa84e2ef4a12c..0e18f440b4f829 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 @@ -47,6 +47,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.TreeMap; import java.util.function.Supplier; @@ -86,9 +87,14 @@ public class BuildConfigurationValue implements BuildConfigurationApi, SkyValue private static final Interner> executionInfoInterner = BlazeInterners.newWeakInterner(); - /** Compute the default shell environment for actions from the command line options. */ - public interface ActionEnvironmentProvider { + /** Global state necessary to build a BuildConfiguration. */ + public interface GlobalStateProvider { + /** Computes the default shell environment for actions from the command line options. */ ActionEnvironment getActionEnvironment(BuildOptions options); + + FragmentRegistry getFragmentRegistry(); + + ImmutableSet getReservedActionMnemonics(); } private final OutputDirectories outputDirectories; @@ -153,14 +159,55 @@ private ActionEnvironment setupTestEnvironment() { return ActionEnvironment.split(testEnv); } - public BuildConfigurationValue( + // Only BuildConfigurationFunction (and tests for mocking purposes) should instantiate this. + public static BuildConfigurationValue create( + BuildOptions buildOptions, + RepositoryName mainRepositoryName, + boolean siblingRepositoryLayout, + // Arguments below this are server-global. BlazeDirectories directories, - ImmutableMap, Fragment> fragments, + GlobalStateProvider globalProvider, + FragmentFactory fragmentFactory) + throws InvalidConfigurationException { + + FragmentClassSet fragmentClasses = globalProvider.getFragmentRegistry().getAllFragments(); + ImmutableSortedMap, Fragment> fragments = + getConfigurationFragments(buildOptions, fragmentClasses, fragmentFactory); + + return new BuildConfigurationValue( + buildOptions, + mainRepositoryName, + siblingRepositoryLayout, + directories, + fragments, + globalProvider.getReservedActionMnemonics(), + globalProvider.getActionEnvironment(buildOptions)); + } + + private static ImmutableSortedMap, Fragment> getConfigurationFragments( + BuildOptions buildOptions, FragmentClassSet fragmentClasses, FragmentFactory fragmentFactory) + throws InvalidConfigurationException { + ImmutableSortedMap.Builder, Fragment> fragments = + ImmutableSortedMap.orderedBy(FragmentClassSet.LEXICAL_FRAGMENT_SORTER); + for (Class fragmentClass : fragmentClasses) { + Fragment fragment = fragmentFactory.createFragment(buildOptions, fragmentClass); + if (fragment != null) { + fragments.put(fragmentClass, fragment); + } + } + return fragments.build(); + } + + // Package-visible for serialization purposes. + BuildConfigurationValue( BuildOptions buildOptions, - ImmutableSet reservedActionMnemonics, - ActionEnvironment actionEnvironment, RepositoryName mainRepositoryName, - boolean siblingRepositoryLayout) + boolean siblingRepositoryLayout, + // Arguments below this are either server-global and constant or completely dependent values. + BlazeDirectories directories, + ImmutableMap, Fragment> fragments, + ImmutableSet reservedActionMnemonics, + ActionEnvironment actionEnvironment) throws InvalidMnemonicException { this.fragments = fragmentsInterner.intern( @@ -219,6 +266,26 @@ public BuildConfigurationValue( this.commandLineLimits = new CommandLineLimits(options.minParamFileSize); } + @Override + public boolean equals(Object other) { + if (this == other) { + return true; + } + if (!(other instanceof BuildConfigurationValue)) { + return false; + } + // Only considering arguments that are non-dependent and non-server-global. + BuildConfigurationValue otherVal = (BuildConfigurationValue) other; + return this.buildOptions.equals(otherVal.buildOptions) + && this.mainRepositoryName.equals(otherVal.mainRepositoryName) + && this.siblingRepositoryLayout == otherVal.siblingRepositoryLayout; + } + + @Override + public int hashCode() { + return Objects.hash(buildOptions, mainRepositoryName, siblingRepositoryLayout); + } + private ImmutableMap> buildIndexOfStarlarkVisibleFragments() { ImmutableMap.Builder> builder = ImmutableMap.builder(); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/FragmentFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/config/FragmentFactory.java index f2cb0fa1115a5f..7a6e3263cac7d0 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/FragmentFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/FragmentFactory.java @@ -17,8 +17,8 @@ import com.github.benmanes.caffeine.cache.LoadingCache; import com.google.auto.value.AutoValue; import com.google.common.base.Throwables; +import com.google.common.collect.ImmutableSet; import java.lang.reflect.InvocationTargetException; -import java.util.Set; import java.util.concurrent.CompletionException; import javax.annotation.Nullable; @@ -67,7 +67,8 @@ public Fragment createFragment(BuildOptions buildOptions, Class fragment) { BuildOptions.Builder trimmed = BuildOptions.builder(); - Set> requiredOptions = Fragment.requiredOptions(fragment); + ImmutableSet> requiredOptions = + Fragment.requiredOptions(fragment); for (FragmentOptions options : original.getNativeOptions()) { // CoreOptions is implicitly required by all fragments. if (options instanceof CoreOptions || requiredOptions.contains(options.getClass())) { diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java index 5c9042983d8245..103513c4bcd5ab 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java @@ -26,7 +26,6 @@ import com.google.devtools.build.lib.analysis.PlatformConfiguration; import com.google.devtools.build.lib.analysis.ShellConfiguration; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; -import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.ActionEnvironmentProvider; import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.config.Fragment; @@ -183,7 +182,7 @@ public static class StrictActionEnvConfiguration extends Fragment { public StrictActionEnvConfiguration(BuildOptions buildOptions) {} } - public static final ActionEnvironmentProvider SHELL_ACTION_ENV = + public static final Function SHELL_ACTION_ENV = (BuildOptions options) -> { boolean strictActionEnv = options.get(StrictActionEnvOptions.class).useStrictActionEnv; OS os = OS.getCurrent(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java index e017a75c8057df..65bbb8ca45df57 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java @@ -13,16 +13,11 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; -import com.google.common.collect.ImmutableSortedMap; -import com.google.devtools.build.lib.actions.ActionEnvironment; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; -import com.google.devtools.build.lib.analysis.config.Fragment; -import com.google.devtools.build.lib.analysis.config.FragmentClassSet; import com.google.devtools.build.lib.analysis.config.FragmentFactory; import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException; -import com.google.devtools.build.lib.analysis.config.OutputDirectories.InvalidMnemonicException; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.packages.RuleClassProvider; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; @@ -53,52 +48,27 @@ public SkyValue compute(SkyKey skyKey, Environment env) if (workspaceNameValue == null) { return null; } - FragmentClassSet fragmentClasses = ruleClassProvider.getFragmentRegistry().getAllFragments(); - - BuildConfigurationKey key = (BuildConfigurationKey) skyKey.argument(); - ImmutableSortedMap, Fragment> fragments; - try { - fragments = getConfigurationFragments(key, fragmentClasses); - } catch (InvalidConfigurationException e) { - throw new BuildConfigurationFunctionException(e); - } StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env); if (starlarkSemantics == null) { return null; } - ActionEnvironment actionEnvironment = - ruleClassProvider.getActionEnvironmentProvider().getActionEnvironment(key.getOptions()); - + BuildConfigurationKey key = (BuildConfigurationKey) skyKey.argument(); try { - return new BuildConfigurationValue( - directories, - fragments, + return BuildConfigurationValue.create( key.getOptions(), - ruleClassProvider.getReservedActionMnemonics(), - actionEnvironment, RepositoryName.createFromValidStrippedName(workspaceNameValue.getName()), - starlarkSemantics.getBool(BuildLanguageOptions.EXPERIMENTAL_SIBLING_REPOSITORY_LAYOUT)); - } catch (InvalidMnemonicException e) { + starlarkSemantics.getBool(BuildLanguageOptions.EXPERIMENTAL_SIBLING_REPOSITORY_LAYOUT), + // Arguments below this are server-global. + directories, + ruleClassProvider, + fragmentFactory); + } catch (InvalidConfigurationException e) { throw new BuildConfigurationFunctionException(e); } } - private ImmutableSortedMap, Fragment> getConfigurationFragments( - BuildConfigurationKey key, FragmentClassSet fragmentClasses) - throws InvalidConfigurationException { - ImmutableSortedMap.Builder, Fragment> fragments = - ImmutableSortedMap.orderedBy(FragmentClassSet.LEXICAL_FRAGMENT_SORTER); - for (Class fragmentClass : fragmentClasses) { - Fragment fragment = fragmentFactory.createFragment(key.getOptions(), fragmentClass); - if (fragment != null) { - fragments.put(fragmentClass, fragment); - } - } - return fragments.build(); - } - private static final class BuildConfigurationFunctionException extends SkyFunctionException { BuildConfigurationFunctionException(InvalidConfigurationException e) { super(e, Transience.PERSISTENT); diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD index 98f98775f87e5a..57bf74799e34d9 100644 --- a/src/test/java/com/google/devtools/build/lib/BUILD +++ b/src/test/java/com/google/devtools/build/lib/BUILD @@ -134,8 +134,9 @@ java_test( "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options", "//src/main/java/com/google/devtools/build/lib/analysis:config/core_options", "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment", - "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_class_set", + "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_factory", "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options", + "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_registry", "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", "//src/main/java/com/google/devtools/build/lib/analysis:server_directories", "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration", diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValueTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValueTest.java index b7f1c784e31b0e..1a8f286de2e3fe 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValueTest.java @@ -20,6 +20,7 @@ import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; +import com.google.common.testing.EqualsTester; import com.google.devtools.build.lib.analysis.config.BuildOptions.MapBackedChecksumCache; import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsChecksumCache; import com.google.devtools.build.lib.analysis.config.OutputDirectories.InvalidMnemonicException; @@ -382,6 +383,25 @@ public void testPlatformInOutputDir() throws Exception { .matches(".*/[^/]+-out/alpha-fastbuild"); } + @Test + public void testConfigurationEquality() throws Exception { + // Note that, in practice, test_arg should not be used as a no-op argument; however, + // these configurations are never trimmed nor even used to build targets so not an issue. + new EqualsTester() + .addEqualityGroup( + createRaw(parseBuildOptions("--test_arg=1a"), "@testrepo", false), + createRaw(parseBuildOptions("--test_arg=1a"), "@testrepo", false)) + // Different BuildOptions means non-equal + .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=1b"), "@testrepo", false)) + // Different --experimental_sibling_repository_layout means non-equal + .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=2"), "@testrepo", true)) + .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=2"), "@testrepo", false)) + // Different repositoryName means non-equal + .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=3"), "@testrepo1", false)) + .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=3"), "@testrepo2", false)) + .testEquals(); + } + /** * Partial verification of deserialized BuildConfigurationValue. * diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD index 9ebde3dca38809..b9229bae604869 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD @@ -49,6 +49,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:config/config_matching_provider", "//src/main/java/com/google/devtools/build/lib/analysis:config/core_option_converters", "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment", + "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_factory", "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options", "//src/main/java/com/google/devtools/build/lib/analysis:config/host_transition", "//src/main/java/com/google/devtools/build/lib/analysis:config/invalid_configuration_exception", diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/ConfigurationTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/ConfigurationTestCase.java index be4eab94518cac..3de28af4d2a0f7 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/ConfigurationTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/ConfigurationTestCase.java @@ -28,6 +28,7 @@ import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; import com.google.devtools.build.lib.analysis.config.BuildOptions; +import com.google.devtools.build.lib.analysis.config.FragmentFactory; import com.google.devtools.build.lib.analysis.config.FragmentOptions; import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException; import com.google.devtools.build.lib.clock.BlazeClock; @@ -45,6 +46,7 @@ import com.google.devtools.build.lib.testutil.FoundationTestCase; import com.google.devtools.build.lib.testutil.SkyframeExecutorTestHelper; import com.google.devtools.build.lib.testutil.TestConstants; +import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.Root; @@ -88,6 +90,7 @@ public static final class TestOptions extends OptionsBase { protected SequencedSkyframeExecutor skyframeExecutor; protected ImmutableSet> buildOptionClasses; protected final ActionKeyContext actionKeyContext = new ActionKeyContext(); + private FragmentFactory fragmentFactory; @Before public final void initializeSkyframeExecutor() throws Exception { @@ -100,7 +103,6 @@ public final void initializeSkyframeExecutor() throws Exception { outputBase, ImmutableList.of(Root.fromPath(rootDirectory)), BazelSkyframeExecutorConstants.BUILD_FILES_BY_PRIORITY); - final PackageFactory pkgFactory; BlazeDirectories directories = new BlazeDirectories( new ServerDirectories(rootDirectory, outputBase, outputBase), @@ -113,7 +115,7 @@ public final void initializeSkyframeExecutor() throws Exception { analysisMock.setupMockClient(mockToolsConfig); analysisMock.setupMockWorkspaceFiles(directories.getEmbeddedBinariesRoot()); - pkgFactory = + PackageFactory pkgFactory = analysisMock .getPackageFactoryBuilderForTesting(directories) .build(ruleClassProvider, fileSystem); @@ -158,6 +160,7 @@ public final void initializeSkyframeExecutor() throws Exception { analysisMock.setupMockClient(mockToolsConfig); analysisMock.setupMockWorkspaceFiles(directories.getEmbeddedBinariesRoot()); buildOptionClasses = ruleClassProvider.getFragmentRegistry().getOptionsClasses(); + fragmentFactory = new FragmentFactory(); } protected void checkError(String expectedMessage, String... options) { @@ -184,6 +187,17 @@ protected BuildConfigurationCollection createCollection(String... args) throws E */ protected BuildConfigurationCollection createCollection( ImmutableMap starlarkOptions, String... args) throws Exception { + + Pair pair = parseBuildOptionsWithTestOptions(starlarkOptions, args); + + skyframeExecutor.handleDiffsForTesting(reporter); + return skyframeExecutor.createConfigurations( + reporter, pair.getFirst(), ImmutableSortedSet.copyOf(pair.getSecond().multiCpus), false); + } + + /** Parses purported commandline options into a BuildOptions (assumes default parsing context.) */ + private Pair parseBuildOptionsWithTestOptions( + ImmutableMap starlarkOptions, String... args) throws Exception { OptionsParser parser = OptionsParser.builder() .optionsClasses( @@ -196,12 +210,32 @@ protected BuildConfigurationCollection createCollection( parser.parse(TestConstants.PRODUCT_SPECIFIC_FLAGS); parser.parse(args); - ImmutableSortedSet multiCpu = ImmutableSortedSet.copyOf( - parser.getOptions(TestOptions.class).multiCpus); + return Pair.of( + BuildOptions.of(buildOptionClasses, parser), parser.getOptions(TestOptions.class)); + } - skyframeExecutor.handleDiffsForTesting(reporter); - return skyframeExecutor.createConfigurations( - reporter, BuildOptions.of(buildOptionClasses, parser), multiCpu, false); + /** Parses purported commandline options into a BuildOptions (assumes default parsing context.) */ + protected BuildOptions parseBuildOptions( + ImmutableMap starlarkOptions, String... args) throws Exception { + return parseBuildOptionsWithTestOptions(starlarkOptions, args).getFirst(); + } + + /** Parses purported commandline options into a BuildOptions (assumes default parsing context.) */ + protected BuildOptions parseBuildOptions(String... args) throws Exception { + return parseBuildOptions(ImmutableMap.of(), args); + } + + /** Returns a raw {@link BuildConfigurationValue} with the given parameters. */ + protected BuildConfigurationValue createRaw( + BuildOptions buildOptions, String repositoryName, boolean siblingRepositoryLayout) + throws Exception { + return BuildConfigurationValue.create( + buildOptions, + RepositoryName.create(repositoryName), + siblingRepositoryLayout, + skyframeExecutor.getBlazeDirectoriesForTesting(), + skyframeExecutor.getRuleClassProviderForTesting(), + fragmentFactory); } /** diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProviderTest.java b/src/test/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProviderTest.java index 4e339f781e61f1..5e3020bb3315e9 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProviderTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProviderTest.java @@ -174,7 +174,7 @@ public void strictActionEnv() throws Exception { "--experimental_strict_action_env", "--action_env=FOO=bar"); - ActionEnvironment env = BazelRuleClassProvider.SHELL_ACTION_ENV.getActionEnvironment(options); + ActionEnvironment env = BazelRuleClassProvider.SHELL_ACTION_ENV.apply(options); assertThat(env.getFixedEnv().toMap()).containsEntry("PATH", "/bin:/usr/bin:/usr/local/bin"); assertThat(env.getFixedEnv().toMap()).containsEntry("FOO", "bar"); } diff --git a/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java b/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java index 33d413a33c7af4..efec6b3f87791b 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java +++ b/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java @@ -22,7 +22,6 @@ import com.google.common.base.Stopwatch; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.eventbus.Subscribe; import com.google.common.util.concurrent.Futures; @@ -42,6 +41,8 @@ import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.CoreOptions; +import com.google.devtools.build.lib.analysis.config.FragmentFactory; +import com.google.devtools.build.lib.analysis.config.FragmentRegistry; import com.google.devtools.build.lib.bugreport.BugReport; import com.google.devtools.build.lib.buildeventstream.AnnounceBuildEventTransportsEvent; import com.google.devtools.build.lib.buildeventstream.ArtifactGroupNamer; @@ -941,18 +942,33 @@ public void testReportedConfigurations() throws Exception { new GenericBuildEvent( testId("Initial"), ImmutableSet.of(ProgressEvent.INITIAL_PROGRESS_UPDATE)); BuildConfigurationValue configuration = - new BuildConfigurationValue( + BuildConfigurationValue.create( + defaultBuildOptions, + RepositoryName.createFromValidStrippedName("workspace"), + /*siblingRepositoryLayout=*/ false, new BlazeDirectories( new ServerDirectories(outputBase, outputBase, outputBase), rootDirectory, /*defaultSystemJavabase=*/ null, "productName"), - /*fragments=*/ ImmutableMap.of(), - defaultBuildOptions, - /*reservedActionMnemonics=*/ ImmutableSet.of(), - ActionEnvironment.EMPTY, - RepositoryName.createFromValidStrippedName("workspace"), - /*siblingRepositoryLayout=*/ false); + new BuildConfigurationValue.GlobalStateProvider() { + @Override + public ActionEnvironment getActionEnvironment(BuildOptions buildOptions) { + return ActionEnvironment.EMPTY; + } + + @Override + public FragmentRegistry getFragmentRegistry() { + return FragmentRegistry.create( + ImmutableList.of(), ImmutableList.of(), ImmutableList.of()); + } + + @Override + public ImmutableSet getReservedActionMnemonics() { + return ImmutableSet.of(); + } + }, + new FragmentFactory()); BuildEvent firstWithConfiguration = new GenericConfigurationEvent(testId("first"), configuration.toBuildEvent()); BuildEvent secondWithConfiguration =