From 4dc0d119a6f4901aa8353e75941c19205542743e Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 13 Sep 2023 14:04:10 -0700 Subject: [PATCH] Replace EXPLICIT_IN_OUTPUT_PATH and Fragment.getOutputDirectoryName w/ Fragment.processForOutputDirectoryMnemonic This better consolidates all the code that computes the output path mnemonic (both the explicit parts like k8-fastbuild and the ST-hash) into one place, OutputPathMnemonicComputer. Note that the determination of the baseline options is still done in BuildConfigurationFunction and passed in. In the new system, Fragment override a function called processForOutputDirectoryMnemonic and have access to an addToMnemonic function, which explicitly adds an entry to the mnemonic, and markAsExplicitInOutputPath, which excludes an option from consideration by ST-hash. This also resolves a code maintenance issue with the old version where options had to be marked as EXPLICIT_IN_OUTPUT_PATH in the FragmentOptions while also having the exact logic for how they were represented in Fragment. Now, both the declaration and naming logic can be co-located. As a mild cleanup, the cpu is no longer explicitly added to the output path as part of CppConfiguration fragment but instead is now always the first part of the mnemonic. This (along with the consolidation done by this CL in general) should simplify later work on including the `--platforms` as explicitly part of the output path. The exact order of mnemonic parts will scramble slightly. The mnemonic now always starts with [cpu/platform]-[compilation_mode]-[platform_suffix] versus the old behavior of the cpu only being added as part of the CppFragment logic and [compilation_mode]-[platform_suffix] coming later. However, the new order should be more consistent and explainable in general. There is a slight regression in behavior for `--output_directory_naming_scheme=legacy` as the code for updating `affected by starlark transition` after a transition cannot yet know if the changed options will be explicit in the output path. Thus, all changes variables are included in the ST-hash. (This is actually a reversion back to much older behaviors and hopefully most users have migrated to using `--output_directory_naming_scheme=diff_against_dynamic_baseline`.) RELNOTES: Change output paths to consistently start with [cpu]-[compilation_mode] along with other cleanups to output path generation logic. PiperOrigin-RevId: 565154905 Change-Id: Ic3501b768a0d77d1f0811cc68ec91ece332cd5df --- .../google/devtools/build/lib/analysis/BUILD | 4 + .../config/BuildConfigurationValue.java | 67 ++- .../lib/analysis/config/CoreOptions.java | 6 +- .../build/lib/analysis/config/Fragment.java | 65 ++- .../analysis/config/OutputDirectories.java | 114 +---- .../config/OutputPathMnemonicComputer.java | 404 ++++++++++++++++++ .../starlark/FunctionTransitionUtil.java | 8 +- .../rules/android/AndroidConfiguration.java | 13 +- .../rules/apple/AppleCommandLineOptions.java | 16 +- .../lib/rules/apple/AppleConfiguration.java | 10 +- .../build/lib/rules/cpp/CppConfiguration.java | 10 +- .../build/lib/rules/cpp/CppOptions.java | 2 +- .../lib/rules/python/PythonConfiguration.java | 13 +- .../build/lib/rules/python/PythonOptions.java | 1 - .../skyframe/BuildConfigurationFunction.java | 179 +------- .../android/AndroidConfigurationApi.java | 1 + .../options/OptionFilterDescriptions.java | 5 +- .../common/options/OptionMetadataTag.java | 17 +- src/main/protobuf/option_filters.proto | 2 +- .../StarlarkAttrTransitionProviderTest.java | 166 ++++--- .../StarlarkRuleTransitionProviderTest.java | 2 +- .../BuildConfigurationFunctionTest.java | 27 +- .../config/BuildConfigurationValueTest.java | 38 +- .../analysis/util/ConfigurationTestCase.java | 14 +- .../lib/buildtool/ConvenienceSymlinkTest.java | 71 ++- .../rules/objc/BazelJ2ObjcLibraryTest.java | 14 +- .../lib/rules/objc/ObjcRuleTestCase.java | 11 +- .../lib/runtime/BuildEventStreamerTest.java | 4 +- .../starlark_configurations_test.sh | 2 +- 29 files changed, 719 insertions(+), 567 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/analysis/config/OutputPathMnemonicComputer.java 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 ccd9d6a166f207..7b2e8cdd51b65d 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -1626,6 +1626,7 @@ java_library( "config/BuildConfigurationValue.java", "config/ConfigRequestedEvent.java", "config/OutputDirectories.java", + "config/OutputPathMnemonicComputer.java", ], deps = [ ":blaze_directories", @@ -1637,9 +1638,12 @@ java_library( ":config/fragment", ":config/fragment_class_set", ":config/fragment_factory", + ":config/fragment_options", ":config/fragment_registry", ":config/invalid_configuration_exception", + ":config/optioninfo", ":config/run_under", + ":config/starlark_defined_config_transition", ":platform_options", "//src/main/java/com/google/devtools/build/lib/actions:action_environment", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", 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 c18e4168b96211..db108e8428186f 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.config.OutputDirectories.InvalidMnemonicException; import com.google.devtools.build.lib.buildeventstream.BuildEvent; import com.google.devtools.build.lib.buildeventstream.BuildEventIdUtil; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos; @@ -121,7 +120,7 @@ public interface GlobalStateProvider { * *

See b/203470434 or #14023 for more information and planned behavior changes. */ - private final String transitionDirectoryNameFragment; + private final String mnemonic; private final ImmutableMap commandLineBuildVariables; @@ -158,12 +157,12 @@ private ActionEnvironment setupTestEnvironment() { return ActionEnvironment.split(testEnv); } - // Only BuildConfigurationFunction (and tests for mocking purposes) should instantiate this. + // Only BuildConfigurationFunction should instantiate this. public static BuildConfigurationValue create( BuildOptions buildOptions, + @Nullable BuildOptions baselineOptions, String workspaceName, boolean siblingRepositoryLayout, - String transitionDirectoryNameFragment, // Arguments below this are server-global. BlazeDirectories directories, GlobalStateProvider globalProvider, @@ -177,11 +176,47 @@ public static BuildConfigurationValue create( ImmutableSortedMap, Fragment> fragments = getConfigurationFragments(buildOptions, fragmentClasses, fragmentFactory); + String mnemonic = + OutputPathMnemonicComputer.computeMnemonic(buildOptions, baselineOptions, fragments); + return new BuildConfigurationValue( buildOptions, + mnemonic, + workspaceName, + siblingRepositoryLayout, + directories, + fragments, + globalProvider.getReservedActionMnemonics(), + globalProvider.getActionEnvironment(buildOptions)); + } + + // TODO(blaze-configurability-team): Ideally tests use the above create; however, + // ConfigurationTestCase most just checks equality constraints and this wants to directly + // fiddle with the mnemonic (and supplying a baselineOptions would be somewhat heavy). + @VisibleForTesting + public static BuildConfigurationValue createForTesting( + BuildOptions buildOptions, + String mnemonic, + String workspaceName, + boolean siblingRepositoryLayout, + // Arguments below this are server-global. + BlazeDirectories directories, + GlobalStateProvider globalProvider, + FragmentFactory fragmentFactory) + throws InvalidConfigurationException { + + FragmentClassSet fragmentClasses = + buildOptions.hasNoConfig() + ? FragmentClassSet.of(ImmutableSet.of()) + : globalProvider.getFragmentRegistry().getAllFragments(); + ImmutableSortedMap, Fragment> fragments = + getConfigurationFragments(buildOptions, fragmentClasses, fragmentFactory); + + return new BuildConfigurationValue( + buildOptions, + mnemonic, workspaceName, siblingRepositoryLayout, - transitionDirectoryNameFragment, directories, fragments, globalProvider.getReservedActionMnemonics(), @@ -205,35 +240,33 @@ private static ImmutableSortedMap, Fragment> getConfig // Package-visible for serialization purposes. BuildConfigurationValue( BuildOptions buildOptions, + String mnemonic, String workspaceName, boolean siblingRepositoryLayout, - String transitionDirectoryNameFragment, // Arguments below this are either server-global and constant or completely dependent values. BlazeDirectories directories, ImmutableMap, Fragment> fragments, ImmutableSet reservedActionMnemonics, - ActionEnvironment actionEnvironment) - throws InvalidMnemonicException { + ActionEnvironment actionEnvironment) { this.fragments = fragmentsInterner.intern( ImmutableSortedMap.copyOf(fragments, FragmentClassSet.LEXICAL_FRAGMENT_SORTER)); this.starlarkVisibleFragments = buildIndexOfStarlarkVisibleFragments(); this.buildOptions = buildOptions; + this.mnemonic = mnemonic; this.options = buildOptions.get(CoreOptions.class); PlatformOptions platformOptions = null; if (buildOptions.contains(PlatformOptions.class)) { platformOptions = buildOptions.get(PlatformOptions.class); } - this.transitionDirectoryNameFragment = transitionDirectoryNameFragment; this.outputDirectories = new OutputDirectories( directories, options, platformOptions, - this.fragments, + mnemonic, workspaceName, - siblingRepositoryLayout, - transitionDirectoryNameFragment); + siblingRepositoryLayout); this.workspaceName = workspaceName; this.siblingRepositoryLayout = siblingRepositoryLayout; @@ -281,13 +314,12 @@ public boolean equals(Object other) { return this.buildOptions.equals(otherVal.buildOptions) && this.workspaceName.equals(otherVal.workspaceName) && this.siblingRepositoryLayout == otherVal.siblingRepositoryLayout - && this.transitionDirectoryNameFragment.equals(otherVal.transitionDirectoryNameFragment); + && this.mnemonic.equals(otherVal.mnemonic); } @Override public int hashCode() { - return Objects.hash( - buildOptions, workspaceName, siblingRepositoryLayout, transitionDirectoryNameFragment); + return Objects.hash(buildOptions, workspaceName, siblingRepositoryLayout, mnemonic); } private ImmutableMap> buildIndexOfStarlarkVisibleFragments() { @@ -479,11 +511,6 @@ public String getOutputDirectoryName() { return outputDirectories.getOutputDirName(); } - @VisibleForTesting - public String getTransitionDirectoryNameFragment() { - return transitionDirectoryNameFragment; - } - @Override public String toString() { return checksum(); 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 de3083d63f6988..59052c94af13bf 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 @@ -125,7 +125,6 @@ public class CoreOptions extends FragmentOptions implements Cloneable { converter = AutoCpuConverter.class, documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS, effectTags = {OptionEffectTag.CHANGES_INPUTS, OptionEffectTag.AFFECTS_OUTPUTS}, - metadataTags = {OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH}, help = "The target CPU.") public String cpu; @@ -255,7 +254,6 @@ public class CoreOptions extends FragmentOptions implements Cloneable { defaultValue = "fastbuild", documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS, effectTags = {OptionEffectTag.AFFECTS_OUTPUTS, OptionEffectTag.ACTION_COMMAND_LINES}, - metadataTags = {OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH}, help = "Specify the mode the binary will be built in. Values: 'fastbuild', 'dbg', 'opt'.") public CompilationMode compilationMode; @@ -316,9 +314,9 @@ public String getTypeDescription() { metadataTags = {OptionMetadataTag.INTERNAL}) public List affectedByStarlarkTransition; - /** Values for the --experimental_exec_configuration_distinguisher options * */ + /** Values for the --experimental_exec_configuration_distinguisher options */ public enum ExecConfigurationDistinguisherScheme { - /** Use hash of selected execution platform for platform_suffix. * */ + /** Use hash of selected execution platform for platform_suffix. */ LEGACY, /** Do not touch platform_suffix or do anything else. * */ OFF, diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/Fragment.java b/src/main/java/com/google/devtools/build/lib/analysis/config/Fragment.java index cf3994d629399e..80e48988cab208 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/Fragment.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/Fragment.java @@ -16,6 +16,7 @@ import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.events.EventHandler; +import com.google.errorprone.annotations.CanIgnoreReturnValue; import java.util.List; import javax.annotation.Nullable; import net.starlark.java.eval.StarlarkValue; @@ -58,14 +59,68 @@ public boolean isImmutable() { public void reportInvalidOptions(EventHandler reporter, BuildOptions buildOptions) {} /** - * Returns a fragment of the output directory name for this configuration. The output directory - * for the whole configuration contains all the short names by all fragments. + * Context needed by implementations of {@link Fragment#processForOutputPathMnemonic}. + * + *

The Fragment constructor should already have sufficient access to targetOptions as per + * RequiresOption above. So a getTargetOption method should not be necessary. */ - @Nullable - public String getOutputDirectoryName() { - return null; + public static interface OutputDirectoriesContext { + /** If available, get the baseline version of some FragmentOption */ + @Nullable + public T getBaseline(Class optionsClass); + + /** + * Adds given String to the explicit part of the output path. + * + *

A null or empty value is not added to the mnemonic. Ideally this function will eventually + * just error when supplied those values. + * + * @throws AddToMnemonicException if given value cannot be put in an output path. + */ + @CanIgnoreReturnValue + public OutputDirectoriesContext addToMnemonic(@Nullable String value) + throws AddToMnemonicException; + + /** + * Mark the option as explicit in output path so it no longer contributes to hash computation. + * + *

Options which are marked must be explicitly included in the output path by {@link + * addToMnemonic} (or indirectly in {@link Fragment.getOutputDirectoryName}) and thus will not + * be included in the hash of changed options used to generically disambiguate output + * directories of different configurations. (See {@link OutputPathMnemonicComputer}.) + * + *

This tag should only be added to options that can guarantee that any change to that option + * corresponds to a change to {@link OutputPathMnemonicComputer.computeMnemonic}. Put + * mathematically, given any two BuildOptions instances A and B with respective values for the + * marked option a and b (where all other options are the same and there is some potentially + * null baseline): {@code a == b iff computeMnemonic(A, baseline) == computeMnemonic(b, + * baseline)} + * + *

As a historical note, this used to be implemented as EXPLICIT_IN_OUTPUT_PATH + */ + @CanIgnoreReturnValue + public OutputDirectoriesContext markAsExplicitInOutputPathFor(String optionName); + + /** bubble up error with adding to mnemonic (likely a problematic value supplied) */ + public static final class AddToMnemonicException extends Exception { + final Exception tunneledException; + final String badValue; + + AddToMnemonicException(String badValue, Exception e) { + super("Invalid option value " + badValue, e); + this.tunneledException = e; + this.badValue = badValue; + } + } } + /** + * Returns a fragment of the output directory name for this set of options. See {@link + * BuildConfigurationFunction.computeMnemonic}) + */ + public void processForOutputPathMnemonic(OutputDirectoriesContext ctx) + throws OutputDirectoriesContext.AddToMnemonicException {} + /** Returns the option classes needed to create a fragment. */ public static ImmutableSet> requiredOptions( Class fragmentClass) { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/OutputDirectories.java b/src/main/java/com/google/devtools/build/lib/analysis/config/OutputDirectories.java index 615bf50f8440d8..5726071eba69f7 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/OutputDirectories.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/OutputDirectories.java @@ -14,26 +14,16 @@ package com.google.devtools.build.lib.analysis.config; -import static com.google.common.base.Predicates.not; -import static java.util.stream.Collectors.joining; -import com.google.common.base.Strings; -import com.google.common.collect.ImmutableSortedMap; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.ArtifactRoot.RootType; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.PlatformOptions; -import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.RepositoryName; -import com.google.devtools.build.lib.server.FailureDetails.BuildConfiguration.Code; import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; -import com.google.devtools.build.lib.vfs.PathFragment.InvalidBaseNameException; -import java.util.ArrayList; -import java.util.List; -import java.util.Map; import javax.annotation.Nullable; /** @@ -128,14 +118,11 @@ public ArtifactRoot getRoot( BlazeDirectories directories, CoreOptions options, @Nullable PlatformOptions platformOptions, - ImmutableSortedMap, Fragment> fragments, + String mnemonic, String workspaceName, - boolean siblingRepositoryLayout, - String transitionDirectoryNameFragment) - throws InvalidMnemonicException { + boolean siblingRepositoryLayout) { this.directories = directories; - this.mnemonic = - buildMnemonic(options, platformOptions, fragments, transitionDirectoryNameFragment); + this.mnemonic = mnemonic; this.outputDirectory = OutputDirectory.OUTPUT.getRoot(mnemonic, directories, workspaceName); this.binDirectory = OutputDirectory.BIN.getRoot(mnemonic, directories, workspaceName); @@ -152,92 +139,6 @@ public ArtifactRoot getRoot( this.execRoot = directories.getExecRoot(workspaceName); } - private static void addMnemonicPart( - List nameParts, String part, String errorTemplate, Object... spec) - throws InvalidMnemonicException { - if (Strings.isNullOrEmpty(part)) { - return; - } - - validateMnemonicPart(part, errorTemplate, spec); - - nameParts.add(part); - } - - /** - * Validate that part is valid for use in the mnemonic, emitting an error message based on the - * template if not. - * - *

The error template is expanded with the part itself as the first argument, and any remaining - * elements of errorArgs following. - */ - private static void validateMnemonicPart(String part, String errorTemplate, Object... errorArgs) - throws InvalidMnemonicException { - try { - PathFragment.checkSeparators(part); - } catch (InvalidBaseNameException e) { - Object[] args = new Object[errorArgs.length + 1]; - args[0] = part; - System.arraycopy(errorArgs, 0, args, 1, errorArgs.length); - String message = String.format(errorTemplate, args); - throw new InvalidMnemonicException(message, e); - } - } - - private static String buildMnemonic( - CoreOptions options, - @Nullable PlatformOptions platformOptions, - ImmutableSortedMap, Fragment> fragments, - String transitionDirectoryNameFragment) - throws InvalidMnemonicException { - // See explanation at declaration for outputRoots. - List nameParts = new ArrayList<>(); - - // Add the fragment-specific sections. - for (Map.Entry, Fragment> entry : fragments.entrySet()) { - String outputDirectoryName = entry.getValue().getOutputDirectoryName(); - addMnemonicPart( - nameParts, - outputDirectoryName, - "Output directory name '%s' specified by %s", - entry.getKey().getSimpleName()); - } - - // Add the compilation mode. - addMnemonicPart(nameParts, options.compilationMode.toString(), "Compilation mode '%s'"); - - // Add the platform suffix, if any. - addMnemonicPart(nameParts, options.platformSuffix, "Platform suffix '%s'"); - - // Add the transition suffix. - addMnemonicPart( - nameParts, transitionDirectoryNameFragment, "Transition directory name fragment '%s'"); - - // Join all the parts. - String mnemonic = nameParts.stream().filter(not(Strings::isNullOrEmpty)).collect(joining("-")); - - // Replace the CPU idenfitier. - String cpuIdentifier = buildCpuIdentifier(options, platformOptions); - validateMnemonicPart(cpuIdentifier, "CPU name '%s'"); - mnemonic = mnemonic.replace("{CPU}", cpuIdentifier); - - return mnemonic; - } - - private static String buildCpuIdentifier( - CoreOptions options, @Nullable PlatformOptions platformOptions) { - if (options.platformInOutputDir && platformOptions != null) { - Label targetPlatform = platformOptions.computeTargetPlatform(); - // Only use non-default platforms. - if (!PlatformOptions.platformIsDefault(targetPlatform)) { - return targetPlatform.getName(); - } - } - - // Fall back to using the CPU. - return options.cpu; - } - private ArtifactRoot buildDerivedRoot( String nameFragment, RepositoryName repository, boolean isMiddleman) { // e.g., execroot/mainRepoName/bazel-out/[repoName/]config/bin @@ -341,13 +242,4 @@ boolean mergeGenfilesDirectory() { BlazeDirectories getDirectories() { return directories; } - - /** Indicates a failure to construct the mnemonic for an output directory. */ - public static class InvalidMnemonicException extends InvalidConfigurationException { - InvalidMnemonicException(String message, InvalidBaseNameException e) { - super( - message + " is invalid as part of a path: " + e.getMessage(), - Code.INVALID_OUTPUT_DIRECTORY_MNEMONIC); - } - } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/OutputPathMnemonicComputer.java b/src/main/java/com/google/devtools/build/lib/analysis/config/OutputPathMnemonicComputer.java new file mode 100644 index 00000000000000..a4c9f457820aad --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/OutputPathMnemonicComputer.java @@ -0,0 +1,404 @@ +// Copyright 2023 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.analysis.config; + +import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition.COMMAND_LINE_OPTION_PREFIX; + +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Strings; +import com.google.common.base.VerifyException; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableSortedMap; +import com.google.devtools.build.lib.analysis.PlatformOptions; +import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.server.FailureDetails.BuildConfiguration.Code; +import com.google.devtools.build.lib.util.Fingerprint; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.PathFragment.InvalidBaseNameException; +import com.google.devtools.common.options.OptionDefinition; +import com.google.errorprone.annotations.CanIgnoreReturnValue; +import java.util.Map; +import java.util.TreeMap; +import javax.annotation.Nullable; + +/** + * Machinery for computing the output path mnemonic given the target options, constructed fragments + * for those target options, and current baseline options (which will be null in legacy mode or odd + * parts of the test infra). + */ +@VisibleForTesting +public final class OutputPathMnemonicComputer { + // The length of the hash of the config tacked onto the end of the output path. + // Limited for ergonomics and MAX_PATH reasons. + private static final int HASH_LENGTH = 12; + + private OutputPathMnemonicComputer() {} + + /** Indicates a failure to construct the mnemonic for an output directory. */ + public static class InvalidMnemonicException extends InvalidConfigurationException { + InvalidMnemonicException(String message, Exception e) { + super( + message + " is invalid as part of a path: " + e.getMessage(), + Code.INVALID_OUTPUT_DIRECTORY_MNEMONIC); + } + } + + /** + * Create a fresh context to pass to {@link Fragment.processForOutputPathMnemonic} + * + *

Needs to be fresh since want new state tracking the current mnemonic and explicit in output + * path option exclusions. + * + *

Note that this class roughly has two sets of methods: 1. The overrides of + * Fragment.OutputDirectoriesContext 2. The new methods used by OutputPathMnemonicComputer to make + * its own additions to the mnemonic and extraction of the information. + */ + private static final class MnemonicContext implements Fragment.OutputDirectoriesContext { + @Nullable private final BuildOptions baselineOptions; + private final StringBuilder mnemonicBuilder; + private final ImmutableSet.Builder explicitInOutputPathBuilder; + + private MnemonicContext(@Nullable BuildOptions baselineOptions) { + this.baselineOptions = baselineOptions; + this.mnemonicBuilder = new StringBuilder(); + this.explicitInOutputPathBuilder = ImmutableSet.builder(); + } + + // Implementations for FragmentOptions to use: + /* If available, get the baseline version of some FragmentOptions */ + @Nullable + @Override + public T getBaseline(Class optionsClass) { + if (baselineOptions == null) { + return null; + } + return baselineOptions.get(optionsClass); + } + + /* Adds given String to the explicit part of the output path. */ + @Override + @CanIgnoreReturnValue + public Fragment.OutputDirectoriesContext addToMnemonic(@Nullable String value) + throws Fragment.OutputDirectoriesContext.AddToMnemonicException { + if (Strings.isNullOrEmpty(value)) { + return this; + } + try { + // Allowing for path separators (e.g. /) would be a disaster. + PathFragment.checkSeparators(value); + // Want dashes in-between additions. + // (Note that length of a StringBuilder is very cheap to check so this performs fine.) + if (mnemonicBuilder.length() > 0) { + mnemonicBuilder.append("-"); + } + mnemonicBuilder.append(value); + } catch (InvalidBaseNameException e) { + throw new AddToMnemonicException(value, e); + } + return this; + } + + /* See docs at {@link Fragment.OutputDirectoriesContext.markAsExplicitInOutputPathFor}*/ + @Override + @CanIgnoreReturnValue + public Fragment.OutputDirectoriesContext markAsExplicitInOutputPathFor(String optionName) { + explicitInOutputPathBuilder.add(optionName); + return this; + } + + // Interface and Implementations for BuildConfigurationFunction to use: + public void consume(Fragment fragment) throws InvalidMnemonicException { + try { + fragment.processForOutputPathMnemonic(this); + } catch (AddToMnemonicException e) { + throw new InvalidMnemonicException( + String.format( + "Output directory name '%s' specified by %s", + e.badValue, fragment.getClass().getSimpleName()), + e.tunneledException); + } + } + + @CanIgnoreReturnValue + public Fragment.OutputDirectoriesContext checkedAddToMnemonic( + @Nullable String value, String valueCtx) throws InvalidMnemonicException { + try { + addToMnemonic(value); + } catch (AddToMnemonicException e) { + throw new InvalidMnemonicException( + String.format("%s '%s'", valueCtx, e.badValue), e.tunneledException); + } + return this; + } + + public String getMnemonic() { + return mnemonicBuilder.toString(); + } + + public ImmutableSet getExplicitInOutputPathOptions() { + return explicitInOutputPathBuilder.build(); + } + } + + /** + * Compute and return the output path mnemonic. + * + *

The general form is [cpu]-[compilation_mode]-[platform_suffix?]-...-[-ST-hash?] where ... is + * any additions requested by the {@link Fragment} via {@link + * Fragment.OutputDirectoriesContext.addToMnemonic} during calls to {@link + * Fragment.processForOutputPathMnemonic}. + * + *

platform_suffix is omitted if empty. + * + *

The exact ST-hash used depends on if baselineOptions is available: + * + *

If not, assume in legacy mode and use `affected by starlark transition` to see what options + * need to be hashed. + * + *

If available, the hash includes all options that are different between buildOptions and + * baselineOptions but were also not excluded from the output path by a call to {@link + * Fragment.OutputDirectoriesContext.markAsExplicitInOutputPathFor} + */ + static final String computeMnemonic( + BuildOptions buildOptions, + @Nullable BuildOptions baselineOptions, + ImmutableSortedMap, Fragment> fragments) + throws InvalidMnemonicException { + + CoreOptions coreOptions = buildOptions.get(CoreOptions.class); + + if (buildOptions.hasNoConfig()) { + // Historically, the noconfig output path mnemonic had the compilation mode. + return coreOptions.compilationMode + "-noconfig"; // See NoConfigTransition. + } + + PlatformOptions platformOptions = buildOptions.get(PlatformOptions.class); + + MnemonicContext ctx = new MnemonicContext(baselineOptions); + + ctx.checkedAddToMnemonic( + buildCpuIdentifier(coreOptions, platformOptions), "CPU/Platform descriptor"); + ctx.markAsExplicitInOutputPathFor("cpu"); + + ctx.checkedAddToMnemonic(coreOptions.compilationMode.toString(), "Compilation mode"); + ctx.markAsExplicitInOutputPathFor("compilation_mode"); + + if (!Strings.isNullOrEmpty(coreOptions.platformSuffix)) { + ctx.checkedAddToMnemonic(coreOptions.platformSuffix, "Platform suffix"); + } + ctx.markAsExplicitInOutputPathFor("platform_suffix"); + + for (Map.Entry, Fragment> entry : fragments.entrySet()) { + ctx.consume(entry.getValue()); + } + + ImmutableSet explicitInOutputPathOptions = ctx.getExplicitInOutputPathOptions(); + + // Sanity check that every listed option in explicitInOutputPathOptions actually exists. + // TODO(blaze-configurability-team): Should technically be unnecessary to do this every time as + // all the calls to markAsExplicitInOutputPathFor should be constant for a given release. + // Instead, could do this when a specific flag is supplied and just check in test code. + // Alternatively, just do a better job of caching the call to OptionInfo.buildMapFrom as only + // that call is the expensive part. + ImmutableMap optionInfoMap = OptionInfo.buildMapFrom(buildOptions); + ImmutableSet missingOptions = + explicitInOutputPathOptions.stream() + .filter(optionName -> !optionInfoMap.containsKey(optionName)) + .collect(toImmutableSet()); + if (!missingOptions.isEmpty()) { + throw new IllegalStateException( + "Internal error: Options registered for special output handling that do not exist: " + + missingOptions); + } + + if (baselineOptions == null) { + ctx.checkedAddToMnemonic( + computeNameFragmentWithAffectedByStarlarkTransition(buildOptions), + "Transition directory name fragment"); + } else { + ctx.checkedAddToMnemonic( + computeNameFragmentWithDiff(buildOptions, baselineOptions, explicitInOutputPathOptions), + "Transition directory name fragment"); + } + return ctx.getMnemonic(); + } + + private static String buildCpuIdentifier( + CoreOptions options, @Nullable PlatformOptions platformOptions) { + if (options.platformInOutputDir && platformOptions != null) { + Label targetPlatform = platformOptions.computeTargetPlatform(); + // Only use non-default platforms. + if (!PlatformOptions.platformIsDefault(targetPlatform)) { + return targetPlatform.getName(); + } + } + + // Fall back to using the CPU. + return options.cpu; + } + + /** + * Compute the hash for the new BuildOptions based on the names and values of all options (both + * native and Starlark) that are different from some supplied baseline configuration. + */ + @VisibleForTesting + public static String computeNameFragmentWithDiff( + BuildOptions toOptions, + BuildOptions baselineOptions, + ImmutableSet explicitInOutputPathOptions) { + // Quick short-circuit for trivial case. + if (toOptions.equals(baselineOptions)) { + return ""; + } + + // TODO(blaze-configurability-team): As a mild performance update, getFirst already includes + // details of the corresponding option. Could incorporate this instead of hashChosenOptions + // regenerating the OptionDefinitions and values. + BuildOptions.OptionsDiff diff = BuildOptions.diff(toOptions, baselineOptions); + // Note: getFirst only excludes options trimmed between baselineOptions to toOptions and this is + // considered OK as a given Rule should not be being built with options of different + // trimmings. See longform note in {@link ConfiguredTargetKey} for details. + ImmutableSet chosenNativeOptions = + diff.getFirst().keySet().stream() + .filter(optionDef -> !explicitInOutputPathOptions.contains(optionDef.getOptionName())) + .map(OptionDefinition::getOptionName) + .collect(toImmutableSet()); + // Note: getChangedStarlarkOptions includes all changed options, added options and removed + // options between baselineOptions and toOptions. This is necessary since there is no current + // notion of trimming a Starlark option: 'null' or non-existent justs means set to default. + ImmutableSet chosenStarlarkOptions = + diff.getChangedStarlarkOptions().stream().map(Label::toString).collect(toImmutableSet()); + return hashChosenOptions(toOptions, chosenNativeOptions, chosenStarlarkOptions); + } + + /** + * Compute the output directory name fragment corresponding to the new BuildOptions based on the + * names and values of all options (both native and Starlark) previously transitioned anywhere in + * the build by Starlark transitions. Options only set on command line are not affecting the + * computation. + * + * @param toOptions the {@link BuildOptions} to use to calculate which we need to compute {@code + * transitionDirectoryNameFragment}. + */ + private static String computeNameFragmentWithAffectedByStarlarkTransition( + BuildOptions toOptions) { + CoreOptions buildConfigOptions = toOptions.get(CoreOptions.class); + if (buildConfigOptions.affectedByStarlarkTransition.isEmpty()) { + return ""; + } + + ImmutableList.Builder affectedNativeOptions = ImmutableList.builder(); + ImmutableList.Builder affectedStarlarkOptions = ImmutableList.builder(); + + // Note that explicitInOutputPathOptions is not sent to this function. + // It is possible for two BuildOptions to differ only in `affected by Starlark transition` + // where the only different is one includes a marked option and the other doesn't. + // Thus, must include all options so those cases get a different output path. + // This legacy is no longer the default and thus entire code path is slated for removal. + for (String optionName : buildConfigOptions.affectedByStarlarkTransition) { + if (optionName.startsWith(COMMAND_LINE_OPTION_PREFIX)) { + String nativeOptionName = optionName.substring(COMMAND_LINE_OPTION_PREFIX.length()); + affectedNativeOptions.add(nativeOptionName); + } else { + affectedStarlarkOptions.add(optionName); + } + } + + return hashChosenOptions( + toOptions, affectedNativeOptions.build(), affectedStarlarkOptions.build()); + } + + /** + * Compute a hash of the given BuildOptions by hashing only the options referenced in both + * chosenNative and chosenStarlark. The order of the chosen order does not matter (as this + * function will effectively sort them into a canonical order) and the pre-hash for each option + * will be of the form (//command_line_option:[native option]|[Starlark option label])=[value]. + * + *

If a supplied native option does not exist, it is skipped (as it is presumed non-existence + * is due to trimming). + * + *

If a supplied Starlark option does exist, the pre-hash will be [Starlark option label]@null + * (as it is presumed non-existence is due to being set to default value). + */ + private static String hashChosenOptions( + BuildOptions toOptions, Iterable chosenNative, Iterable chosenStarlark) { + // TODO(blaze-configurability-team): A mild performance optimization would have this be global. + ImmutableMap optionInfoMap = OptionInfo.buildMapFrom(toOptions); + + // Note that the TreeMap guarantees a stable ordering of keys and thus + // it is okay if chosenNative or chosenStarlark do not have a stable iteration order + TreeMap toHash = new TreeMap<>(); + for (String nativeOptionName : chosenNative) { + Object value; + try { + OptionInfo optionInfo = optionInfoMap.get(nativeOptionName); + if (optionInfo == null) { + // This can occur if toOptions has been trimmed but the supplied chosen native options + // includes that trimmed options. + // (e.g. legacy naming mode, using --trim_test_configuration and --test_arg transition). + continue; + } + value = + optionInfo + .getDefinition() + .getField() + .get(toOptions.get(optionInfoMap.get(nativeOptionName).getOptionClass())); + } catch (IllegalAccessException e) { + throw new VerifyException( + "IllegalAccess for option " + nativeOptionName + ": " + e.getMessage()); + } + // TODO(blaze-configurability-team): The commandline option is legacy and can be removed + // after fixing up all the associated tests. + toHash.put("//command_line_option:" + nativeOptionName, value); + } + for (String starlarkOptionName : chosenStarlark) { + Object value = + toOptions.getStarlarkOptions().get(Label.parseCanonicalUnchecked(starlarkOptionName)); + toHash.put(starlarkOptionName, value); + } + + if (toHash.isEmpty()) { + return ""; + } else { + ImmutableList.Builder hashStrs = ImmutableList.builderWithExpectedSize(toHash.size()); + for (Map.Entry singleOptionAndValue : toHash.entrySet()) { + Object value = singleOptionAndValue.getValue(); + if (value != null) { + hashStrs.add(singleOptionAndValue.getKey() + "=" + value); + } else { + // Avoid using =null to different from value being the non-null String "null" + hashStrs.add(singleOptionAndValue.getKey() + "@null"); + } + } + return transitionDirectoryNameFragment(hashStrs.build()); + } + } + + @VisibleForTesting + public static String transitionDirectoryNameFragment(Iterable opts) { + Fingerprint fp = new Fingerprint(); + for (String opt : opts) { + fp.addString(opt); + } + // Shorten the hash to HASH_LENGTH characters. This should provide sufficient collision + // avoidance (that is, we don't expect anyone to experience a collision ever). + // Shortening the hash is important for Windows paths that tend to be short. + String suffix = fp.hexDigestAndReset().substring(0, HASH_LENGTH); + return "ST-" + suffix; + } +} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java index 03bfdb4b72b449..cb7d52a1273acb 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java @@ -39,7 +39,6 @@ import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.packages.StructImpl; import com.google.devtools.common.options.OptionDefinition; -import com.google.devtools.common.options.OptionMetadataTag; import com.google.devtools.common.options.OptionsParsingException; import java.lang.reflect.Field; import java.util.HashSet; @@ -434,9 +433,7 @@ private static BuildOptions applyTransition( } field.set(toOptions.get(optionInfo.getOptionClass()), convertedValue); - if (!optionInfo.hasOptionMetadataTag(OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH)) { - convertedAffectedOptions.add(optionKey); - } + convertedAffectedOptions.add(optionKey); } } catch (IllegalArgumentException e) { @@ -496,9 +493,6 @@ public static ImmutableSet getAffectedByStarlarkTransitionViaDiff( BuildOptions.OptionsDiff diff = BuildOptions.diff(toOptions, baselineOptions); Stream diffNative = diff.getFirst().keySet().stream() - .filter( - optionDef -> - !optionDef.hasOptionMetadataTag(OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH)) .map(option -> COMMAND_LINE_OPTION_PREFIX + option.getOptionName()); // Note: getChangedStarlarkOptions includes all changed options, added options and removed // options between baselineOptions and toOptions. This is necessary since there is no current diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java index c0df5c32ba1088..a935142518d4b5 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java @@ -197,13 +197,14 @@ public enum ManifestMergerOrder { /** Android configuration options. */ public static class Options extends FragmentOptions { + // TODO(blaze-configurability-team): Deprecate this when legacy output directory scheme is gone. @Option( name = "Android configuration distinguisher", defaultValue = "MAIN", converter = ConfigurationDistinguisherConverter.class, documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, effectTags = OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION, - metadataTags = {OptionMetadataTag.INTERNAL, OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH}) + metadataTags = {OptionMetadataTag.INTERNAL}) public ConfigurationDistinguisher configurationDistinguisher; // TODO(blaze-configurability): Mark this as deprecated in favor of --android_platforms. @@ -1513,6 +1514,16 @@ public String getOutputDirectoryName() { return configurationDistinguisher.suffix; } + // TODO(blaze-configurability-team): Deprecate this. + @Override + public void processForOutputPathMnemonic(Fragment.OutputDirectoriesContext ctx) + throws Fragment.OutputDirectoriesContext.AddToMnemonicException { + ctx.markAsExplicitInOutputPathFor("Android configuration distinguisher"); + if (configurationDistinguisher.suffix != null) { + ctx.addToMnemonic(configurationDistinguisher.suffix); + } + } + public boolean filterRJarsFromAndroidTest() { return filterRJarsFromAndroidTest; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/apple/AppleCommandLineOptions.java b/src/main/java/com/google/devtools/build/lib/rules/apple/AppleCommandLineOptions.java index 0a4a72a6d48cf5..37047e0a6cf4fa 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/apple/AppleCommandLineOptions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/apple/AppleCommandLineOptions.java @@ -235,14 +235,14 @@ public class AppleCommandLineOptions extends FragmentOptions { // This option must only be set by those transitions for this purpose. // TODO(bazel-team): Remove this once we have dynamic configurations but make sure that different // configurations (e.g. by min os version) always use different output paths. + // TODO(blaze-configurability-team): Deprecate this when legacy output directory scheme is gone. @Option( - name = "apple configuration distinguisher", - defaultValue = "UNKNOWN", - converter = ConfigurationDistinguisherConverter.class, - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, - effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION}, - metadataTags = {OptionMetadataTag.INTERNAL} - ) + name = "apple configuration distinguisher", + defaultValue = "UNKNOWN", + converter = ConfigurationDistinguisherConverter.class, + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION}, + metadataTags = {OptionMetadataTag.INTERNAL}) public ConfigurationDistinguisher configurationDistinguisher; @Option( @@ -405,7 +405,7 @@ public FragmentOptions getExec() { // be needed. exec.applePlatformType = PlatformType.MACOS; exec.configurationDistinguisher = ConfigurationDistinguisher.UNKNOWN; - // Preseve Xcode selection preferences so that the same Xcode version is used throughout the + // Preserve Xcode selection preferences so that the same Xcode version is used throughout the // build. exec.preferMutualXcode = preferMutualXcode; exec.includeXcodeExecutionRequirements = includeXcodeExecutionRequirements; diff --git a/src/main/java/com/google/devtools/build/lib/rules/apple/AppleConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/apple/AppleConfiguration.java index 1c357376a775e2..12fb935b2aedbf 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/apple/AppleConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/apple/AppleConfiguration.java @@ -34,7 +34,6 @@ import com.google.devtools.build.lib.util.CPU; import java.util.ArrayList; import java.util.List; -import javax.annotation.Nullable; import net.starlark.java.eval.EvalException; import net.starlark.java.eval.StarlarkThread; import net.starlark.java.eval.StarlarkValue; @@ -398,9 +397,9 @@ public Label getXcodeConfigLabel() { return xcodeConfigLabel; } - @Nullable @Override - public String getOutputDirectoryName() { + public void processForOutputPathMnemonic(Fragment.OutputDirectoriesContext ctx) + throws Fragment.OutputDirectoriesContext.AddToMnemonicException { List components = new ArrayList<>(); if (!appleCpus.appleSplitCpu().isEmpty()) { components.add(applePlatformType.toString().toLowerCase()); @@ -414,10 +413,9 @@ public String getOutputDirectoryName() { components.add(configurationDistinguisher.getFileSystemName()); } - if (components.isEmpty()) { - return null; + if (!components.isEmpty()) { + ctx.addToMnemonic(Joiner.on('-').join(components)); } - return Joiner.on('-').join(components); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java index e7e98f8bfbfed8..28a63b6bab260d 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java @@ -593,14 +593,12 @@ public void reportInvalidOptions(EventHandler reporter, BuildOptions buildOption } @Override - public String getOutputDirectoryName() { - // Add a tag that will be replaced with the CPU identifier. - String result = "{CPU}"; + public void processForOutputPathMnemonic(OutputDirectoriesContext ctx) + throws Fragment.OutputDirectoriesContext.AddToMnemonicException { + ctx.markAsExplicitInOutputPathFor("cc_output_directory_tag"); if (!cppOptions.outputDirectoryTag.isEmpty()) { - result += "-" + cppOptions.outputDirectoryTag; + ctx.addToMnemonic(cppOptions.outputDirectoryTag); } - - return result; } /** Returns true if we should share identical native libraries between different targets. */ diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java index c0896b128e281b..671d2c56e2b6c0 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java @@ -140,12 +140,12 @@ public String getTypeDescription() { // This is different from --platform_suffix in that that one is designed to facilitate the // migration to toolchains and this one is designed to eliminate the C++ toolchain identifier // from the output directory path. + // TODO(blaze-configurability-team): Deprecate this when legacy output directory scheme is gone. @Option( name = "cc_output_directory_tag", defaultValue = "", documentationCategory = OptionDocumentationCategory.TOOLCHAIN, effectTags = {OptionEffectTag.AFFECTS_OUTPUTS}, - metadataTags = {OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH}, help = "Specifies a suffix to be added to the configuration directory.") public String outputDirectoryTag; diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java index 3264501b5ed684..95847bca53f1a2 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java @@ -113,8 +113,8 @@ public String getDefaultPythonVersionForStarlark() { } @Override - @Nullable - public String getOutputDirectoryName() { + public void processForOutputPathMnemonic(Fragment.OutputDirectoriesContext ctx) + throws Fragment.OutputDirectoriesContext.AddToMnemonicException { Preconditions.checkState(version.isTargetValue()); // The only possible Python target version values are PY2 and PY3. Historically, PY3 targets got // a "-py3" suffix and PY2 targets got the empty suffix, so that the bazel-bin symlink pointed @@ -126,10 +126,15 @@ public String getOutputDirectoryName() { + "versions. Please check that PythonConfiguration#getOutputDirectoryName() is still " + "needed and is still able to avoid output directory clashes, then update this " + "canary message."); + ctx.markAsExplicitInOutputPathFor("python_version"); if (py2OutputsAreSuffixed) { - return version == PythonVersion.PY2 ? "py2" : null; + if (version == PythonVersion.PY2) { + ctx.addToMnemonic("py2"); + } } else { - return version == PythonVersion.PY3 ? "py3" : null; + if (version == PythonVersion.PY3) { + ctx.addToMnemonic("py3"); + } } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java index 438d742b7b944b..8faf557c446bbd 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java @@ -165,7 +165,6 @@ public String getTypeDescription() { OptionEffectTag.LOADING_AND_ANALYSIS, OptionEffectTag.AFFECTS_OUTPUTS // because of "-py2"/"-py3" output root }, - metadataTags = {OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH}, help = "The Python major version mode, either `PY2` or `PY3`. Note that this is overridden by " + "`py_binary` and `py_test` targets (even if they don't explicitly specify a " 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 090e113ddecd1d..c0ea96a9564b6c 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,14 +13,6 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; -import static com.google.common.collect.ImmutableSet.toImmutableSet; -import static com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition.COMMAND_LINE_OPTION_PREFIX; - -import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.VerifyException; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; 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; @@ -29,30 +21,19 @@ 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.InvalidConfigurationException; -import com.google.devtools.build.lib.analysis.config.OptionInfo; import com.google.devtools.build.lib.analysis.config.transitions.BaselineOptionsValue; -import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.packages.RuleClassProvider; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; -import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionException; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; -import com.google.devtools.common.options.OptionDefinition; -import com.google.devtools.common.options.OptionMetadataTag; -import java.util.Map; -import java.util.TreeMap; import javax.annotation.Nullable; import net.starlark.java.eval.StarlarkSemantics; /** A builder for {@link BuildConfigurationValue} instances. */ public final class BuildConfigurationFunction implements SkyFunction { - // The length of the hash of the config tacked onto the end of the output path. - // Limited for ergonomics and MAX_PATH reasons. - private static final int HASH_LENGTH = 12; - private final BlazeDirectories directories; private final ConfiguredRuleClassProvider ruleClassProvider; private final FragmentFactory fragmentFactory = new FragmentFactory(); @@ -82,10 +63,8 @@ public SkyValue compute(SkyKey skyKey, Environment env) BuildOptions targetOptions = key.getOptions(); CoreOptions coreOptions = targetOptions.get(CoreOptions.class); - String transitionDirectoryNameFragment; - if (targetOptions.hasNoConfig()) { - transitionDirectoryNameFragment = "noconfig"; // See NoConfigTransition. - } else if (coreOptions.useBaselineForOutputDirectoryNamingScheme()) { + BuildOptions baselineOptions = null; + if (coreOptions.useBaselineForOutputDirectoryNamingScheme()) { boolean applyExecTransitionToBaseline = coreOptions.outputDirectoryNamingScheme.equals( CoreOptions.OutputDirectoryNamingScheme.DIFF_AGAINST_DYNAMIC_BASELINE) @@ -96,22 +75,17 @@ public SkyValue compute(SkyKey skyKey, Environment env) if (baselineOptionsValue == null) { return null; } - - transitionDirectoryNameFragment = - computeNameFragmentWithDiff(targetOptions, baselineOptionsValue.toOptions()); - } else { - transitionDirectoryNameFragment = - computeNameFragmentWithAffectedByStarlarkTransition(targetOptions); + baselineOptions = baselineOptionsValue.toOptions(); } try { var configurationValue = BuildConfigurationValue.create( targetOptions, + baselineOptions, workspaceNameValue.getName(), starlarkSemantics.getBool( BuildLanguageOptions.EXPERIMENTAL_SIBLING_REPOSITORY_LAYOUT), - transitionDirectoryNameFragment, // Arguments below this are server-global. directories, ruleClassProvider, @@ -128,149 +102,4 @@ private static final class BuildConfigurationFunctionException extends SkyFuncti super(e, Transience.PERSISTENT); } } - - /** - * Compute the hash for the new BuildOptions based on the names and values of all options (both - * native and Starlark) that are different from some supplied baseline configuration. - */ - @VisibleForTesting - public static String computeNameFragmentWithDiff( - BuildOptions toOptions, BuildOptions baselineOptions) { - // Quick short-circuit for trivial case. - if (toOptions.equals(baselineOptions)) { - return ""; - } - - // TODO(blaze-configurability-team): As a mild performance update, getFirst already includes - // details of the corresponding option. Could incorporate this instead of hashChosenOptions - // regenerating the OptionDefinitions and values. - BuildOptions.OptionsDiff diff = BuildOptions.diff(toOptions, baselineOptions); - // Note: getFirst only excludes options trimmed between baselineOptions to toOptions and this is - // considered OK as a given Rule should not be being built with options of different - // trimmings. See longform note in {@link ConfiguredTargetKey} for details. - ImmutableSet chosenNativeOptions = - diff.getFirst().keySet().stream() - .filter( - optionDef -> - !optionDef.hasOptionMetadataTag(OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH)) - .map(OptionDefinition::getOptionName) - .collect(toImmutableSet()); - // Note: getChangedStarlarkOptions includes all changed options, added options and removed - // options between baselineOptions and toOptions. This is necessary since there is no current - // notion of trimming a Starlark option: 'null' or non-existent justs means set to default. - ImmutableSet chosenStarlarkOptions = - diff.getChangedStarlarkOptions().stream().map(Label::toString).collect(toImmutableSet()); - return hashChosenOptions(toOptions, chosenNativeOptions, chosenStarlarkOptions); - } - - /** - * Compute the output directory name fragment corresponding to the new BuildOptions based on the - * names and values of all options (both native and Starlark) previously transitioned anywhere in - * the build by Starlark transitions. Options only set on command line are not affecting the - * computation. - * - * @param toOptions the {@link BuildOptions} to use to calculate which we need to compute {@code - * transitionDirectoryNameFragment}. - */ - private static String computeNameFragmentWithAffectedByStarlarkTransition( - BuildOptions toOptions) { - CoreOptions buildConfigOptions = toOptions.get(CoreOptions.class); - if (buildConfigOptions.affectedByStarlarkTransition.isEmpty()) { - return ""; - } - - ImmutableList.Builder affectedNativeOptions = ImmutableList.builder(); - ImmutableList.Builder affectedStarlarkOptions = ImmutableList.builder(); - - for (String optionName : buildConfigOptions.affectedByStarlarkTransition) { - if (optionName.startsWith(COMMAND_LINE_OPTION_PREFIX)) { - String nativeOptionName = optionName.substring(COMMAND_LINE_OPTION_PREFIX.length()); - affectedNativeOptions.add(nativeOptionName); - } else { - affectedStarlarkOptions.add(optionName); - } - } - - return hashChosenOptions( - toOptions, affectedNativeOptions.build(), affectedStarlarkOptions.build()); - } - - /** - * Compute a hash of the given BuildOptions by hashing only the options referenced in both - * chosenNative and chosenStarlark. The order of the chosen order does not matter (as this - * function will effectively sort them into a canonical order) and the pre-hash for each option - * will be of the form (//command_line_option:[native option]|[Starlark option label])=[value]. - * - *

If a supplied native option does not exist, it is skipped (as it is presumed non-existence - * is due to trimming). - * - *

If a supplied Starlark option does exist, the pre-hash will be [Starlark option label]@null - * (as it is presumed non-existence is due to being set to default value). - */ - private static String hashChosenOptions( - BuildOptions toOptions, Iterable chosenNative, Iterable chosenStarlark) { - // TODO(blaze-configurability-team): A mild performance optimization would have this be global. - ImmutableMap optionInfoMap = OptionInfo.buildMapFrom(toOptions); - - // Note that the TreeMap guarantees a stable ordering of keys and thus - // it is okay if chosenNative or chosenStarlark do not have a stable iteration order - TreeMap toHash = new TreeMap<>(); - for (String nativeOptionName : chosenNative) { - Object value; - try { - OptionInfo optionInfo = optionInfoMap.get(nativeOptionName); - if (optionInfo == null) { - // This can occur if toOptions has been trimmed but the supplied chosen native options - // includes that trimmed options. - // (e.g. legacy naming mode, using --trim_test_configuration and --test_arg transition). - continue; - } - value = - optionInfo - .getDefinition() - .getField() - .get(toOptions.get(optionInfoMap.get(nativeOptionName).getOptionClass())); - } catch (IllegalAccessException e) { - throw new VerifyException( - "IllegalAccess for option " + nativeOptionName + ": " + e.getMessage()); - } - // TODO(blaze-configurability-team): The commandline option is legacy and can be removed - // after fixing up all the associated tests. - toHash.put("//command_line_option:" + nativeOptionName, value); - } - for (String starlarkOptionName : chosenStarlark) { - Object value = - toOptions.getStarlarkOptions().get(Label.parseCanonicalUnchecked(starlarkOptionName)); - toHash.put(starlarkOptionName, value); - } - - if (toHash.isEmpty()) { - return ""; - } else { - ImmutableList.Builder hashStrs = ImmutableList.builderWithExpectedSize(toHash.size()); - for (Map.Entry singleOptionAndValue : toHash.entrySet()) { - Object value = singleOptionAndValue.getValue(); - if (value != null) { - hashStrs.add(singleOptionAndValue.getKey() + "=" + value); - } else { - // Avoid using =null to different from value being the non-null String "null" - hashStrs.add(singleOptionAndValue.getKey() + "@null"); - } - } - return transitionDirectoryNameFragment(hashStrs.build()); - } - } - - @VisibleForTesting - public static String transitionDirectoryNameFragment(Iterable opts) { - Fingerprint fp = new Fingerprint(); - for (String opt : opts) { - fp.addString(opt); - } - // Shorten the hash to 48 bits. This should provide sufficient collision avoidance - // (that is, we don't expect anyone to experience a collision ever). - // Shortening the hash is important for Windows paths that tend to be short. - String suffix = fp.hexDigestAndReset().substring(0, HASH_LENGTH); - return "ST-" + suffix; - } } diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/android/AndroidConfigurationApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/android/AndroidConfigurationApi.java index ecc0679010164c..29333d2461a814 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/android/AndroidConfigurationApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/android/AndroidConfigurationApi.java @@ -242,6 +242,7 @@ public interface AndroidConfigurationApi extends StarlarkValue { documented = false) boolean persistentMultiplexDexDesugar(); + // TODO(blaze-configurability-team): Deprecate this. @StarlarkMethod( name = "get_output_directory_name", structField = true, diff --git a/src/main/java/com/google/devtools/common/options/OptionFilterDescriptions.java b/src/main/java/com/google/devtools/common/options/OptionFilterDescriptions.java index e87fbdfdd1b268..dbc09a8af31c70 100644 --- a/src/main/java/com/google/devtools/common/options/OptionFilterDescriptions.java +++ b/src/main/java/com/google/devtools/common/options/OptionFilterDescriptions.java @@ -187,10 +187,7 @@ public static ImmutableMap getOptionMetadataTagDescri "This option should not be used by a user, and should not be logged.") .put( OptionMetadataTag.INTERNAL, // Here for completeness, these options are UNDOCUMENTED. - "This option isn't even a option, and should not be logged.") - .put( - OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH, - "This option is explicitly mentioned in the output directory."); + "This option isn't even a option, and should not be logged."); return effectTagDescriptionBuilder.build(); } } diff --git a/src/main/java/com/google/devtools/common/options/OptionMetadataTag.java b/src/main/java/com/google/devtools/common/options/OptionMetadataTag.java index 7b0bd2bcb9dc7c..a5c8580850fd64 100644 --- a/src/main/java/com/google/devtools/common/options/OptionMetadataTag.java +++ b/src/main/java/com/google/devtools/common/options/OptionMetadataTag.java @@ -55,25 +55,10 @@ public enum OptionMetadataTag { * *

These should be in category {@link OptionDocumentationCategory.UNDOCUMENTED}. */ - INTERNAL(4), + INTERNAL(4); // reserved TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES(5) - /** - * Options which are EXPLICIT_IN_OUTPUT_PATH are explicitly included in the output path by {@link - * OutputDirectories.buildMnemonic} (or indirectly in {@link Fragment.getOutputDirectoryName}) and - * thus should not be included in the hash of changed options used to generically disambiguate - * output directories of different configurations. (See {@link - * FunctionTransitionUtil.computeOutputDirectoryNameFragment}.) - * - *

This tag should only be added to options that can guarantee that any change to that option - * corresponds to a change to {@link OutputDirectories.buildMnemonic}. Put mathematically, given - * any two BuildOptions instances A and B with respective values for the marked option a and b - * (and all other options are the same): {@code a == b iff OutputDirectories.buildMnemonic(A) == - * OutputDirectories.buildMnemonic(B)} - */ - EXPLICIT_IN_OUTPUT_PATH(6); - private final int value; OptionMetadataTag(int value) { diff --git a/src/main/protobuf/option_filters.proto b/src/main/protobuf/option_filters.proto index d931083c40522a..8fba26438130a3 100644 --- a/src/main/protobuf/option_filters.proto +++ b/src/main/protobuf/option_filters.proto @@ -55,5 +55,5 @@ enum OptionMetadataTag { INTERNAL = 4; reserved "TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES"; reserved 5; - EXPLICIT_IN_OUTPUT_PATH = 6; + reserved 6; } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java index d8396d9eecd2ef..efead04a869bcb 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java @@ -25,6 +25,7 @@ import com.google.common.collect.Maps; import com.google.common.eventbus.EventBus; import com.google.devtools.build.lib.analysis.config.CoreOptions; +import com.google.devtools.build.lib.analysis.config.OutputPathMnemonicComputer; import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.analysis.util.DummyTestFragment; @@ -38,7 +39,6 @@ import com.google.devtools.build.lib.packages.util.BazelMockAndroidSupport; import com.google.devtools.build.lib.rules.cpp.CppConfiguration; import com.google.devtools.build.lib.rules.cpp.CppOptions; -import com.google.devtools.build.lib.skyframe.BuildConfigurationFunction; import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; import com.google.devtools.build.lib.testutil.TestConstants; import com.google.devtools.build.lib.testutil.TestRuleClassProvider; @@ -1187,8 +1187,8 @@ private Object getStarlarkOption(ConfiguredTarget target, String absName) { return getStarlarkOptions(target).get(Label.parseCanonicalUnchecked(absName)); } - private String getTransitionDirectoryNameFragment(ConfiguredTarget target) { - return getConfiguration(target).getTransitionDirectoryNameFragment(); + private String getMnemonic(ConfiguredTarget target) { + return getConfiguration(target).getMnemonic(); } @Test @@ -1294,7 +1294,7 @@ public void testTransitionBackToStarlarkDefaultOK() throws Exception { ConfiguredTarget test = getConfiguredTarget("//test/starlark:test"); // '//test/starlark:did-transition ensures ST-hash is 'turned on' since :test has no ST-hash - // and thus will trivially have a unique getTransitionDirectoryNameFragment + // and thus will trivially have a unique getMnemonic @SuppressWarnings("unchecked") ConfiguredTarget dep1 = @@ -1312,18 +1312,11 @@ public void testTransitionBackToStarlarkDefaultOK() throws Exception { (List) getMyInfoFromTarget(dep2).getValue("dep")); // These must be true - assertThat(getTransitionDirectoryNameFragment(dep1)) - .isNotEqualTo(getTransitionDirectoryNameFragment(dep2)); + assertThat(getMnemonic(dep1)).isNotEqualTo(getMnemonic(dep2)); - assertThat(getTransitionDirectoryNameFragment(dep2)) - .isNotEqualTo(getTransitionDirectoryNameFragment(dep3)); + assertThat(getMnemonic(dep2)).isNotEqualTo(getMnemonic(dep3)); - // TODO(blaze-configurability-team): When "affected by starlark transition" is gone, - // will be equal and thus getTransitionDirectoryNameFragment can be equal. - if (!getConfiguration(dep1).equals(getConfiguration(dep3))) { - assertThat(getTransitionDirectoryNameFragment(dep1)) - .isNotEqualTo(getTransitionDirectoryNameFragment(dep3)); - } + assertThat(getMnemonic(dep1)).isEqualTo(getMnemonic(dep3)); } @Test @@ -1361,9 +1354,9 @@ public void testTransitionOnBuildSetting_onlyTransitionsAffectsDirectory() throw // Assert that transitionDirectoryNameFragment is only affected by options // set via transitions. Not by native or starlark options set via command line, // never touched by any transition. - assertThat(getTransitionDirectoryNameFragment(dep)) - .isEqualTo( - BuildConfigurationFunction.transitionDirectoryNameFragment( + assertThat(getMnemonic(dep)) + .endsWith( + OutputPathMnemonicComputer.transitionDirectoryNameFragment( ImmutableList.of("//test/starlark:the-answer=42"))); } @@ -1416,9 +1409,9 @@ public void testTransitionOnCompilationMode_hasNoHash() throws Exception { (List) getMyInfoFromTarget(dep1).getValue("dep")); // Assert transitionDirectoryNameFragment is empty for all configurations - assertThat(getTransitionDirectoryNameFragment(test)).isEmpty(); - assertThat(getTransitionDirectoryNameFragment(dep1)).isEmpty(); - assertThat(getTransitionDirectoryNameFragment(dep2)).isEmpty(); + assertThat(getMnemonic(test)).doesNotContain("-ST-"); + assertThat(getMnemonic(dep1)).doesNotContain("-ST-"); + assertThat(getMnemonic(dep2)).doesNotContain("-ST-"); // test and dep1 should have different configurations b/c compilation_mode changed assertThat(getConfiguration(test)).isNotEqualTo(getConfiguration(dep1)); @@ -1485,14 +1478,14 @@ public void testOutputDirHash_multipleNativeOptionTransitions_legacyNaming() thr assertThat(affectedOptions) .containsExactly("//command_line_option:foo", "//command_line_option:bar"); - assertThat(getTransitionDirectoryNameFragment(test)) - .isEqualTo( - BuildConfigurationFunction.transitionDirectoryNameFragment( + assertThat(getMnemonic(test)) + .endsWith( + OutputPathMnemonicComputer.transitionDirectoryNameFragment( ImmutableList.of("//command_line_option:foo=foosball"))); - assertThat(getTransitionDirectoryNameFragment(dep)) - .isEqualTo( - BuildConfigurationFunction.transitionDirectoryNameFragment( + assertThat(getMnemonic(dep)) + .endsWith( + OutputPathMnemonicComputer.transitionDirectoryNameFragment( ImmutableList.of( "//command_line_option:bar=barsball", "//command_line_option:foo=foosball"))); } @@ -1509,14 +1502,14 @@ public void testOutputDirHash_multipleNativeOptionTransitions_diffNaming() throw Iterables.getOnlyElement( (List) getMyInfoFromTarget(test).getValue("dep")); - assertThat(getTransitionDirectoryNameFragment(test)) - .isEqualTo( - BuildConfigurationFunction.transitionDirectoryNameFragment( + assertThat(getMnemonic(test)) + .endsWith( + OutputPathMnemonicComputer.transitionDirectoryNameFragment( ImmutableList.of("//command_line_option:foo=foosball"))); - assertThat(getTransitionDirectoryNameFragment(dep)) - .isEqualTo( - BuildConfigurationFunction.transitionDirectoryNameFragment( + assertThat(getMnemonic(dep)) + .endsWith( + OutputPathMnemonicComputer.transitionDirectoryNameFragment( ImmutableList.of( "//command_line_option:bar=barsball", "//command_line_option:foo=foosball"))); } @@ -1547,13 +1540,13 @@ public void testOutputDirHash_onlyExec_diffDynamic() throws Exception { ConfiguredTarget dep = (ConfiguredTarget) getMyInfoFromTarget(test).getValue("dep"); - assertThat(getTransitionDirectoryNameFragment(test)).isEmpty(); + assertThat(getMnemonic(test)).doesNotContain("-ST-"); // Until platforms is EXPLICIT_IN_OUTPUT_PATH, it will change here as well. // But, nothing else should be different. - assertThat(getTransitionDirectoryNameFragment(dep)) - .isEqualTo( - BuildConfigurationFunction.transitionDirectoryNameFragment( + assertThat(getMnemonic(dep)) + .endsWith( + OutputPathMnemonicComputer.transitionDirectoryNameFragment( ImmutableList.of( "//command_line_option:platforms=" + getConfiguration(dep) @@ -1600,15 +1593,15 @@ public void testOutputDirHash_starlarkRevertedByExec_diffDynamic() throws Except ConfiguredTarget dep = (ConfiguredTarget) getMyInfoFromTarget(test).getValue("dep"); - assertThat(getTransitionDirectoryNameFragment(test)) - .isEqualTo( - BuildConfigurationFunction.transitionDirectoryNameFragment( + assertThat(getMnemonic(test)) + .endsWith( + OutputPathMnemonicComputer.transitionDirectoryNameFragment( ImmutableList.of("//command_line_option:set_by_exec=at_target"))); // Until platforms is EXPLICIT_IN_OUTPUT_PATH, it will change here as well. - assertThat(getTransitionDirectoryNameFragment(dep)) - .isEqualTo( - BuildConfigurationFunction.transitionDirectoryNameFragment( + assertThat(getMnemonic(dep)) + .endsWith( + OutputPathMnemonicComputer.transitionDirectoryNameFragment( ImmutableList.of( "//command_line_option:platforms=" + getConfiguration(dep) @@ -1664,8 +1657,7 @@ public void testOutputDirHash_noop_changeToSameState() throws Exception { Iterables.getOnlyElement( (List) getMyInfoFromTarget(test).getValue("dep")); - assertThat(getTransitionDirectoryNameFragment(test)) - .isEqualTo(getTransitionDirectoryNameFragment(dep)); + assertThat(getMnemonic(test)).isEqualTo(getMnemonic(dep)); } @Test @@ -1708,8 +1700,7 @@ public void testOutputDirHash_noop_emptyReturns() throws Exception { Iterables.getOnlyElement( (List) getMyInfoFromTarget(test).getValue("dep")); - assertThat(getTransitionDirectoryNameFragment(test)) - .isEqualTo(getTransitionDirectoryNameFragment(dep)); + assertThat(getMnemonic(test)).isEqualTo(getMnemonic(dep)); } // Test that setting all starlark options back to default != null hash of top level. @@ -1770,7 +1761,7 @@ public void testOutputDirHash_multipleStarlarkOptionTransitions_backToDefaultCom (List) getMyInfoFromTarget(getConfiguredTarget("//test")).getValue("dep")); - assertThat(getTransitionDirectoryNameFragment(dep)).isNotEmpty(); + assertThat(getMnemonic(dep)).contains("-ST-"); } /** See comment above {@link FunctionTransitionUtil#updateOutputDirectoryNameFragment} */ @@ -1825,13 +1816,13 @@ public void testOutputDirHash_starlarkOption_differentBoolRepresentationsNotEqua Iterables.getOnlyElement( (List) getMyInfoFromTarget(test).getValue("dep")); - assertThat(getTransitionDirectoryNameFragment(test)) - .isEqualTo( - BuildConfigurationFunction.transitionDirectoryNameFragment( + assertThat(getMnemonic(test)) + .endsWith( + OutputPathMnemonicComputer.transitionDirectoryNameFragment( ImmutableList.of("//test:foo=1"))); - assertThat(getTransitionDirectoryNameFragment(dep)) - .isEqualTo( - BuildConfigurationFunction.transitionDirectoryNameFragment( + assertThat(getMnemonic(dep)) + .endsWith( + OutputPathMnemonicComputer.transitionDirectoryNameFragment( ImmutableList.of("//test:foo=true"))); } @@ -1879,8 +1870,7 @@ public void testOutputDirHash_nativeOption_differentBoolRepresentationsEquals() Iterables.getOnlyElement( (List) getMyInfoFromTarget(test).getValue("dep")); - assertThat(getTransitionDirectoryNameFragment(test)) - .isEqualTo(getTransitionDirectoryNameFragment(dep)); + assertThat(getMnemonic(test)).isEqualTo(getMnemonic(dep)); } private void writeFilesWithMultipleStarlarkTransitions() throws Exception { @@ -1942,13 +1932,13 @@ public void testOutputDirHash_multipleStarlarkTransitions_legacyNaming() throws getConfiguration(dep).getOptions().get(CoreOptions.class).affectedByStarlarkTransition; assertThat(affectedOptions).containsExactly("//test:bar", "//test:foo"); - assertThat(getTransitionDirectoryNameFragment(test)) - .isEqualTo( - BuildConfigurationFunction.transitionDirectoryNameFragment( + assertThat(getMnemonic(test)) + .endsWith( + OutputPathMnemonicComputer.transitionDirectoryNameFragment( ImmutableList.of("//test:foo=foosball"))); - assertThat(getTransitionDirectoryNameFragment(dep)) - .isEqualTo( - BuildConfigurationFunction.transitionDirectoryNameFragment( + assertThat(getMnemonic(dep)) + .endsWith( + OutputPathMnemonicComputer.transitionDirectoryNameFragment( ImmutableList.of("//test:bar=barsball", "//test:foo=foosball"))); } @@ -1964,13 +1954,13 @@ public void testOutputDirHash_multipleStarlarkTransitions_diffNaming() throws Ex Iterables.getOnlyElement( (List) getMyInfoFromTarget(test).getValue("dep")); - assertThat(getTransitionDirectoryNameFragment(test)) - .isEqualTo( - BuildConfigurationFunction.transitionDirectoryNameFragment( + assertThat(getMnemonic(test)) + .endsWith( + OutputPathMnemonicComputer.transitionDirectoryNameFragment( ImmutableList.of("//test:foo=foosball"))); - assertThat(getTransitionDirectoryNameFragment(dep)) - .isEqualTo( - BuildConfigurationFunction.transitionDirectoryNameFragment( + assertThat(getMnemonic(dep)) + .endsWith( + OutputPathMnemonicComputer.transitionDirectoryNameFragment( ImmutableList.of("//test:bar=barsball", "//test:foo=foosball"))); } @@ -2057,9 +2047,9 @@ public void testOutputDirHash_multipleMixedTransitions_legacyNaming() throws Exc assertThat(affectedOptionsTop).containsExactly("//command_line_option:foo"); assertThat(getConfiguration(top).getOptions().getStarlarkOptions()).isEmpty(); - assertThat(getTransitionDirectoryNameFragment(top)) - .isEqualTo( - BuildConfigurationFunction.transitionDirectoryNameFragment( + assertThat(getMnemonic(top)) + .endsWith( + OutputPathMnemonicComputer.transitionDirectoryNameFragment( ImmutableList.of("//command_line_option:foo=foosball"))); // test:middle (foo_transition, zee_transition, bar_transition) @@ -2077,9 +2067,9 @@ public void testOutputDirHash_multipleMixedTransitions_legacyNaming() throws Exc .containsExactly( Maps.immutableEntry(Label.parseCanonicalUnchecked("//test:zee"), "zeesball")); - assertThat(getTransitionDirectoryNameFragment(middle)) - .isEqualTo( - BuildConfigurationFunction.transitionDirectoryNameFragment( + assertThat(getMnemonic(middle)) + .endsWith( + OutputPathMnemonicComputer.transitionDirectoryNameFragment( ImmutableList.of( "//command_line_option:bar=barsball", "//command_line_option:foo=foosball", @@ -2102,9 +2092,9 @@ public void testOutputDirHash_multipleMixedTransitions_legacyNaming() throws Exc .containsExactly( Maps.immutableEntry(Label.parseCanonicalUnchecked("//test:zee"), "zeesball"), Maps.immutableEntry(Label.parseCanonicalUnchecked("//test:xan"), "xansball")); - assertThat(getTransitionDirectoryNameFragment(bottom)) - .isEqualTo( - BuildConfigurationFunction.transitionDirectoryNameFragment( + assertThat(getMnemonic(bottom)) + .endsWith( + OutputPathMnemonicComputer.transitionDirectoryNameFragment( ImmutableList.of( "//command_line_option:bar=barsball", "//command_line_option:foo=foosball", "//test:xan=xansball", "//test:zee=zeesball"))); @@ -2119,9 +2109,9 @@ public void testOutputDirHash_multipleMixedTransitions_diffNaming() throws Excep ConfiguredTarget top = getConfiguredTarget("//test:top"); assertThat(getConfiguration(top).getOptions().getStarlarkOptions()).isEmpty(); - assertThat(getTransitionDirectoryNameFragment(top)) - .isEqualTo( - BuildConfigurationFunction.transitionDirectoryNameFragment( + assertThat(getMnemonic(top)) + .endsWith( + OutputPathMnemonicComputer.transitionDirectoryNameFragment( ImmutableList.of("//command_line_option:foo=foosball"))); // test:middle (foo_transition, zee_transition, bar_transition) @@ -2133,9 +2123,9 @@ public void testOutputDirHash_multipleMixedTransitions_diffNaming() throws Excep .containsExactly( Maps.immutableEntry(Label.parseCanonicalUnchecked("//test:zee"), "zeesball")); - assertThat(getTransitionDirectoryNameFragment(middle)) - .isEqualTo( - BuildConfigurationFunction.transitionDirectoryNameFragment( + assertThat(getMnemonic(middle)) + .endsWith( + OutputPathMnemonicComputer.transitionDirectoryNameFragment( ImmutableList.of( "//command_line_option:bar=barsball", "//command_line_option:foo=foosball", @@ -2151,9 +2141,9 @@ public void testOutputDirHash_multipleMixedTransitions_diffNaming() throws Excep .containsExactly( Maps.immutableEntry(Label.parseCanonicalUnchecked("//test:zee"), "zeesball"), Maps.immutableEntry(Label.parseCanonicalUnchecked("//test:xan"), "xansball")); - assertThat(getTransitionDirectoryNameFragment(bottom)) - .isEqualTo( - BuildConfigurationFunction.transitionDirectoryNameFragment( + assertThat(getMnemonic(bottom)) + .endsWith( + OutputPathMnemonicComputer.transitionDirectoryNameFragment( ImmutableList.of( "//command_line_option:bar=barsball", "//command_line_option:foo=foosball", "//test:xan=xansball", "//test:zee=zeesball"))); @@ -2419,8 +2409,7 @@ private void testNoOpTransitionLeavesSameConfig_native(boolean directRead) throw ConfiguredTarget dep = Iterables.getOnlyElement( (List) getMyInfoFromTarget(test).getValue("dep")); - assertThat(getTransitionDirectoryNameFragment(test)) - .isEqualTo(getTransitionDirectoryNameFragment(dep)); + assertThat(getMnemonic(test)).isEqualTo(getMnemonic(dep)); } @Test @@ -2492,8 +2481,7 @@ private void testNoOpTransitionLeavesSameConfig_starlark(boolean directRead, boo ConfiguredTarget dep = Iterables.getOnlyElement( (List) getMyInfoFromTarget(test).getValue("dep")); - assertThat(getTransitionDirectoryNameFragment(test)) - .isEqualTo(getTransitionDirectoryNameFragment(dep)); + assertThat(getMnemonic(test)).isEqualTo(getMnemonic(dep)); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java index ba0bc227834141..c52d3163015804 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java @@ -1600,7 +1600,7 @@ public void invalidMnemonicFromDepTransition() throws Exception { "genrule(name = 'test', srcs = [':bottom'], outs = ['out'], cmd = 'touch $@')"); reporter.removeHandler(failFastHandler); assertThat(getConfiguredTarget("//test:test")).isNull(); - assertContainsEvent("CPU name '//bad:cpu' is invalid as part of a path: must not contain /"); + assertContainsEvent("'//bad:cpu' is invalid as part of a path: must not contain /"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationFunctionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationFunctionTest.java index 7faa02e86bd03c..6ded392b8a7c3a 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationFunctionTest.java @@ -23,7 +23,6 @@ import com.google.devtools.build.lib.packages.Provider; import com.google.devtools.build.lib.packages.StarlarkProvider; import com.google.devtools.build.lib.packages.StructImpl; -import com.google.devtools.build.lib.skyframe.BuildConfigurationFunction; import java.util.List; import org.junit.Before; import org.junit.Test; @@ -72,8 +71,8 @@ private CoreOptions getCoreOptions(ConfiguredTarget target) { return getConfiguration(target).getOptions().get(CoreOptions.class); } - private String getTransitionDirectoryNameFragment(ConfiguredTarget target) { - return getConfiguration(target).getTransitionDirectoryNameFragment(); + private String getMnemonic(ConfiguredTarget target) { + return getConfiguration(target).getMnemonic(); } @Test @@ -114,7 +113,7 @@ public void testDiffAgainstBaselineOutputScheme_hasHash() throws Exception { useConfiguration("--experimental_output_directory_naming_scheme=diff_against_baseline"); ConfiguredTarget test = getConfiguredTarget("//test"); - assertThat(getTransitionDirectoryNameFragment(test)).isEmpty(); + assertThat(getMnemonic(test)).doesNotContain("-ST-"); assertThat(getCoreOptions(test).affectedByStarlarkTransition).isEmpty(); @SuppressWarnings("unchecked") @@ -122,9 +121,9 @@ public void testDiffAgainstBaselineOutputScheme_hasHash() throws Exception { Iterables.getOnlyElement( (List) getMyInfoFromTarget(test).getValue("dep")); - assertThat(getTransitionDirectoryNameFragment(dep)) - .isEqualTo( - BuildConfigurationFunction.transitionDirectoryNameFragment( + assertThat(getMnemonic(dep)) + .endsWith( + OutputPathMnemonicComputer.transitionDirectoryNameFragment( ImmutableList.of("//test:foo=transitioned"))); assertThat(getCoreOptions(dep).affectedByStarlarkTransition).isEmpty(); } @@ -168,7 +167,7 @@ public void testDiffAgainstBaselineOutputScheme_avoidHashForInExplicitOutputPath ConfiguredTarget test = getConfiguredTarget("//test"); assertThat(getConfiguration(test).getMnemonic()).contains("fastbuild"); - assertThat(getTransitionDirectoryNameFragment(test)).isEmpty(); + assertThat(getMnemonic(test)).doesNotContain("-ST-"); assertThat(getCoreOptions(test).affectedByStarlarkTransition).isEmpty(); @SuppressWarnings("unchecked") @@ -177,7 +176,7 @@ public void testDiffAgainstBaselineOutputScheme_avoidHashForInExplicitOutputPath (List) getMyInfoFromTarget(test).getValue("dep")); assertThat(getConfiguration(dep).getMnemonic()).contains("opt"); - assertThat(getTransitionDirectoryNameFragment(dep)).isEmpty(); + assertThat(getMnemonic(dep)).doesNotContain("-ST-"); assertThat(getCoreOptions(dep).affectedByStarlarkTransition).isEmpty(); } @@ -223,7 +222,7 @@ public void testDiffAgainstBaselineOutputScheme_abaAvoidsHash() throws Exception useConfiguration("--experimental_output_directory_naming_scheme=diff_against_baseline"); ConfiguredTarget test = getConfiguredTarget("//test"); - assertThat(getTransitionDirectoryNameFragment(test)).isEmpty(); + assertThat(getMnemonic(test)).doesNotContain("-ST-"); assertThat(getCoreOptions(test).affectedByStarlarkTransition).isEmpty(); @SuppressWarnings("unchecked") @@ -231,9 +230,9 @@ public void testDiffAgainstBaselineOutputScheme_abaAvoidsHash() throws Exception Iterables.getOnlyElement( (List) getMyInfoFromTarget(test).getValue("dep")); - assertThat(getTransitionDirectoryNameFragment(middle)) - .isEqualTo( - BuildConfigurationFunction.transitionDirectoryNameFragment( + assertThat(getMnemonic(middle)) + .endsWith( + OutputPathMnemonicComputer.transitionDirectoryNameFragment( ImmutableList.of("//test:foo=transitioned"))); assertThat(getCoreOptions(middle).affectedByStarlarkTransition).isEmpty(); @@ -242,7 +241,7 @@ public void testDiffAgainstBaselineOutputScheme_abaAvoidsHash() throws Exception Iterables.getOnlyElement( (List) getMyInfoFromTarget(middle).getValue("dep")); - assertThat(getTransitionDirectoryNameFragment(test)).isEmpty(); + assertThat(getMnemonic(test)).doesNotContain("-ST-"); assertThat(getCoreOptions(test).affectedByStarlarkTransition).isEmpty(); assertThat(getConfiguration(test)).isEqualTo(getConfiguration(root)); 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 5ae3f15f90b7f5..818a5d2906cf06 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 @@ -14,14 +14,12 @@ package com.google.devtools.build.lib.analysis.config; import static com.google.common.truth.Truth.assertThat; -import static org.junit.Assert.assertThrows; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; 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; import com.google.devtools.build.lib.analysis.util.ConfigurationTestCase; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.RepositoryName; @@ -268,22 +266,6 @@ private ImmutableList getTestConfigurations() throws Ex "#a=pounda")); } - @Test - public void throwsOnBadMnemonic() { - InvalidMnemonicException e = - assertThrows(InvalidMnemonicException.class, () -> create("--cpu=//bad/cpu")); - assertThat(e) - .hasMessageThat() - .isEqualTo("CPU name '//bad/cpu' is invalid as part of a path: must not contain /"); - e = - assertThrows( - InvalidMnemonicException.class, () -> create("--platform_suffix=//bad/suffix")); - assertThat(e) - .hasMessageThat() - .isEqualTo( - "Platform suffix '//bad/suffix' is invalid as part of a path: must not contain /"); - } - @Test public void testCodec() throws Exception { // Unnecessary ImmutableList.copyOf apparently necessary to choose non-varargs constructor. @@ -327,20 +309,20 @@ public void testConfigurationEquality() throws Exception { // 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, "")) + createRaw(parseBuildOptions("--test_arg=1a"), "k8", "testrepo", false), + createRaw(parseBuildOptions("--test_arg=1a"), "k8", "testrepo", false)) // Different BuildOptions means non-equal - .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=1b"), "testrepo", false, "")) + .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=1b"), "k8", "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, "")) + .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=2"), "k8", "testrepo", true)) + .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=2"), "k8", "testrepo", false)) // Different repositoryName means non-equal - .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=3"), "testrepo1", false, "")) - .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=3"), "testrepo2", false, "")) + .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=3"), "k8", "testrepo1", false)) + .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=3"), "k8", "testrepo2", false)) // Different transitionDirectoryNameFragment means non-equal - .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=3"), "testrepo", false, "")) - .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=3"), "testrepo", false, "a")) - .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=3"), "testrepo", false, "b")) + .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=3"), "k8", "testrepo", false)) + .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=3"), "arm", "testrepo", false)) + .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=3"), "risc", "testrepo", false)) .testEquals(); } 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 508393bf6443f5..93e519c49eb8db 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 @@ -24,7 +24,6 @@ import com.google.devtools.build.lib.analysis.ServerDirectories; 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.FragmentOptions; import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException; @@ -94,6 +93,7 @@ public final void initializeSkyframeExecutor() throws Exception { analysisMock = AnalysisMock.get(); ConfiguredRuleClassProvider ruleClassProvider = analysisMock.createRuleClassProvider(); + buildOptionClasses = ruleClassProvider.getFragmentRegistry().getOptionsClasses(); PathPackageLocator pkgLocator = new PathPackageLocator( outputBase, @@ -132,8 +132,7 @@ public final void initializeSkyframeExecutor() throws Exception { ImmutableList.of( PrecomputedValue.injected( PrecomputedValue.BASELINE_CONFIGURATION, - BuildOptions.getDefaultBuildOptionsForFragments( - ImmutableList.of(CoreOptions.class))), + BuildOptions.getDefaultBuildOptionsForFragments(buildOptionClasses)), PrecomputedValue.injected( RepositoryDelegatorFunction.RESOLVED_FILE_INSTEAD_OF_WORKSPACE, Optional.empty()), PrecomputedValue.injected( @@ -160,7 +159,6 @@ public final void initializeSkyframeExecutor() throws Exception { mockToolsConfig = new MockToolsConfig(rootDirectory); analysisMock.setupMockClient(mockToolsConfig); analysisMock.setupMockWorkspaceFiles(directories.getEmbeddedBinariesRoot()); - buildOptionClasses = ruleClassProvider.getFragmentRegistry().getOptionsClasses(); fragmentFactory = new FragmentFactory(); } @@ -230,15 +228,15 @@ protected BuildOptions parseBuildOptions(String... args) throws Exception { /** Returns a raw {@link BuildConfigurationValue} with the given parameters. */ protected BuildConfigurationValue createRaw( BuildOptions buildOptions, + String mnemonic, String workspaceName, - boolean siblingRepositoryLayout, - String transitionDirectoryNameFragment) + boolean siblingRepositoryLayout) throws Exception { - return BuildConfigurationValue.create( + return BuildConfigurationValue.createForTesting( buildOptions, + mnemonic, workspaceName, siblingRepositoryLayout, - transitionDirectoryNameFragment, skyframeExecutor.getBlazeDirectoriesForTesting(), skyframeExecutor.getRuleClassProviderForTesting(), fragmentFactory); diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java index aa8c9a2271c8b3..771d31dfa10eeb 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java +++ b/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java @@ -52,7 +52,6 @@ import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; -import com.google.devtools.common.options.OptionMetadataTag; import com.google.testing.junit.testparameterinjector.TestParameter; import com.google.testing.junit.testparameterinjector.TestParameterInjector; import java.io.IOException; @@ -69,7 +68,6 @@ public static final class PathTestOptions extends FragmentOptions { name = "output_directory_name", documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, effectTags = {OptionEffectTag.AFFECTS_OUTPUTS}, - metadataTags = {OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH}, defaultValue = "default") public String outputDirectoryName; @@ -91,8 +89,10 @@ public PathTestConfiguration(BuildOptions buildOptions) { } @Override - public String getOutputDirectoryName() { - return outputDirectoryName; + public void processForOutputPathMnemonic(OutputDirectoriesContext ctx) + throws Fragment.OutputDirectoriesContext.AddToMnemonicException { + ctx.markAsExplicitInOutputPathFor("output_directory_name"); + ctx.addToMnemonic(outputDirectoryName); } } @@ -328,13 +328,13 @@ public void sanityCheckOutputDirectory() throws Exception { .relativeTo(getOutputPath()) .toString()))) .containsExactly( - "//path:from_flag", "set_by_flag-" + getTargetConfiguration().getCpu() + "-fastbuild", + "//path:from_flag", getTargetConfiguration().getCpu() + "-fastbuild-set_by_flag", "//path:from_transition", - "set_by_transition-" + getTargetConfiguration().getCpu() + "-fastbuild", + getTargetConfiguration().getCpu() + "-fastbuild-set_by_transition", "//path:unrelated_transition", - "set_by_flag-" + getTargetConfiguration().getCpu() + "-fastbuild", + getTargetConfiguration().getCpu() + "-fastbuild-set_by_flag", "//path:outgoing_transition", - "set_by_flag-" + getTargetConfiguration().getCpu() + "-fastbuild"); + getTargetConfiguration().getCpu() + "-fastbuild-set_by_flag"); } @Test @@ -366,14 +366,13 @@ public void buildingNothing_unsetsSymlinks() throws Exception { // these were also not created under other names "nothing-bin", getOutputPath() - .getRelative("default-" + getTargetConfiguration().getCpu() + "-fastbuild/bin"), + .getRelative(getTargetConfiguration().getCpu() + "-fastbuild-default/bin"), "nothing-genfiles", getOutputPath() - .getRelative("default-" + getTargetConfiguration().getCpu() + "-fastbuild/bin"), + .getRelative(getTargetConfiguration().getCpu() + "-fastbuild-default/bin"), "nothing-testlogs", getOutputPath() - .getRelative( - "default-" + getTargetConfiguration().getCpu() + "-fastbuild/testlogs"), + .getRelative(getTargetConfiguration().getCpu() + "-fastbuild-default/testlogs"), "nothing-" + TestConstants.WORKSPACE_NAME, getExecRoot(), "nothing-out", @@ -479,15 +478,13 @@ public void buildingTargetsWithDifferentOutputDirectories_setsSymlinksIfAnyAreTo .containsExactly( "ambiguous-bin", getOutputPath() - .getRelative("default-" + getTargetConfiguration().getCpu() + "-fastbuild/bin"), + .getRelative(getTargetConfiguration().getCpu() + "-fastbuild-default/bin"), "ambiguous-genfiles", getOutputPath() - .getRelative( - "default-" + getTargetConfiguration().getCpu() + "-fastbuild/genfiles"), + .getRelative(getTargetConfiguration().getCpu() + "-fastbuild-default/genfiles"), "ambiguous-testlogs", getOutputPath() - .getRelative( - "default-" + getTargetConfiguration().getCpu() + "-fastbuild/testlogs"), + .getRelative(getTargetConfiguration().getCpu() + "-fastbuild-default/testlogs"), "ambiguous-" + TestConstants.WORKSPACE_NAME, getExecRoot(), "ambiguous-out", @@ -512,15 +509,13 @@ public void buildingTargetsWithSameConfiguration_setsSymlinks() throws Exception .containsExactly( "same-bin", getOutputPath() - .getRelative("configured-" + getTargetConfiguration().getCpu() + "-fastbuild/bin"), + .getRelative(getTargetConfiguration().getCpu() + "-fastbuild-configured/bin"), "same-genfiles", getOutputPath() - .getRelative( - "configured-" + getTargetConfiguration().getCpu() + "-fastbuild/genfiles"), + .getRelative(getTargetConfiguration().getCpu() + "-fastbuild-configured/genfiles"), "same-testlogs", getOutputPath() - .getRelative( - "configured-" + getTargetConfiguration().getCpu() + "-fastbuild/testlogs"), + .getRelative(getTargetConfiguration().getCpu() + "-fastbuild-configured/testlogs"), "same-" + TestConstants.WORKSPACE_NAME, getExecRoot(), "same-out", @@ -549,15 +544,13 @@ public void buildingSameConfigurationTargetsWithDifferentConfigurationDeps_setsS .containsExactly( "united-bin", getOutputPath() - .getRelative("from_flag-" + getTargetConfiguration().getCpu() + "-fastbuild/bin"), + .getRelative(getTargetConfiguration().getCpu() + "-fastbuild-from_flag/bin"), "united-genfiles", getOutputPath() - .getRelative( - "from_flag-" + getTargetConfiguration().getCpu() + "-fastbuild/genfiles"), + .getRelative(getTargetConfiguration().getCpu() + "-fastbuild-from_flag/genfiles"), "united-testlogs", getOutputPath() - .getRelative( - "from_flag-" + getTargetConfiguration().getCpu() + "-fastbuild/testlogs"), + .getRelative(getTargetConfiguration().getCpu() + "-fastbuild-from_flag/testlogs"), "united-" + TestConstants.WORKSPACE_NAME, getExecRoot(), "united-out", @@ -587,15 +580,13 @@ public void differentConfigurationSameOutputDirectory_setsSymlinks() throws Exce .containsExactly( "unchanged-bin", getOutputPath() - .getRelative("from_flag-" + getTargetConfiguration().getCpu() + "-fastbuild/bin"), + .getRelative(getTargetConfiguration().getCpu() + "-fastbuild-from_flag/bin"), "unchanged-genfiles", getOutputPath() - .getRelative( - "from_flag-" + getTargetConfiguration().getCpu() + "-fastbuild/genfiles"), + .getRelative(getTargetConfiguration().getCpu() + "-fastbuild-from_flag/genfiles"), "unchanged-testlogs", getOutputPath() - .getRelative( - "from_flag-" + getTargetConfiguration().getCpu() + "-fastbuild/testlogs"), + .getRelative(getTargetConfiguration().getCpu() + "-fastbuild-from_flag/testlogs"), "unchanged-" + TestConstants.WORKSPACE_NAME, getExecRoot(), "unchanged-out", @@ -623,15 +614,13 @@ public void nullConfigurationWithOtherMatchingOutputDir_setsSymlinks() throws Ex .containsExactly( "mixed-bin", getOutputPath() - .getRelative("from_flag-" + getTargetConfiguration().getCpu() + "-fastbuild/bin"), + .getRelative(getTargetConfiguration().getCpu() + "-fastbuild-from_flag/bin"), "mixed-genfiles", getOutputPath() - .getRelative( - "from_flag-" + getTargetConfiguration().getCpu() + "-fastbuild/genfiles"), + .getRelative(getTargetConfiguration().getCpu() + "-fastbuild-from_flag/genfiles"), "mixed-testlogs", getOutputPath() - .getRelative( - "from_flag-" + getTargetConfiguration().getCpu() + "-fastbuild/testlogs"), + .getRelative(getTargetConfiguration().getCpu() + "-fastbuild-from_flag/testlogs"), "mixed-" + TestConstants.WORKSPACE_NAME, getExecRoot(), "mixed-out", @@ -809,19 +798,17 @@ public void settingSymlinksReplacesSymlinksEvenIfNotPointingInsideExecRoot() thr assertThat(binLink.readSymbolicLink()) .isEqualTo( getOutputPath() - .getRelative("from_flag-" + getTargetConfiguration().getCpu() + "-fastbuild/bin") + .getRelative(getTargetConfiguration().getCpu() + "-fastbuild-from_flag/bin") .asFragment()); assertThat(genfilesLink.readSymbolicLink()) .isEqualTo( getOutputPath() - .getRelative( - "from_flag-" + getTargetConfiguration().getCpu() + "-fastbuild/genfiles") + .getRelative(getTargetConfiguration().getCpu() + "-fastbuild-from_flag/genfiles") .asFragment()); assertThat(testlogsLink.readSymbolicLink()) .isEqualTo( getOutputPath() - .getRelative( - "from_flag-" + getTargetConfiguration().getCpu() + "-fastbuild/testlogs") + .getRelative(getTargetConfiguration().getCpu() + "-fastbuild-from_flag/testlogs") .asFragment()); assertThat(workspaceLink.readSymbolicLink()).isEqualTo(getExecRoot().asFragment()); assertThat(outLink.readSymbolicLink()).isEqualTo(getOutputPath().asFragment()); diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/BazelJ2ObjcLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/BazelJ2ObjcLibraryTest.java index 1ab651e60598e5..c74e31deaad925 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/BazelJ2ObjcLibraryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/BazelJ2ObjcLibraryTest.java @@ -290,7 +290,7 @@ public void testJ2ObjcProtoClassMappingFilesExportedInJavaLibrary() throws Excep assertThat(classMappingFilesList.get(0).getExecPathString()) .containsMatch( - "/applebin_macos-darwin_x86_64-fastbuild-ST-[^/]*/bin/java/com/google/dummy/test/proto/test.clsmap.properties"); + "/darwin_x86_64-fastbuild-applebin_macos-ST-[^/]*/bin/java/com/google/dummy/test/proto/test.clsmap.properties"); } @Test @@ -318,15 +318,15 @@ public void testJavaProtoLibraryWithProtoLibrary() throws Exception { getArtifacts(j2ObjcMappingFileInfo, "class_mapping_files"); assertThat(classMappingFilesList.get(0).getExecPathString()) .containsMatch( - "/applebin_macos-darwin_x86_64-fastbuild-ST-[^/]*/bin/x/test.clsmap.properties"); + "/darwin_x86_64-fastbuild-applebin_macos-ST-[^/]*/bin/x/test.clsmap.properties"); ObjcProvider objcProvider = target.get(ObjcProvider.STARLARK_CONSTRUCTOR); CcCompilationContext ccCompilationContext = target.get(CcInfo.PROVIDER).getCcCompilationContext(); assertThat(ccCompilationContext.getDeclaredIncludeSrcs().toList().toString()) - .containsMatch("/applebin_macos-darwin_x86_64-fastbuild-ST-[^/]*/bin]x/test.j2objc.pb.h"); + .containsMatch("/darwin_x86_64-fastbuild-applebin_macos-ST-[^/]*/bin]x/test.j2objc.pb.h"); assertThat(objcProvider.get(ObjcProvider.SOURCE).toList().toString()) - .containsMatch("/applebin_macos-darwin_x86_64-fastbuild-ST-[^/]*/bin]x/test.j2objc.pb.m,"); + .containsMatch("/darwin_x86_64-fastbuild-applebin_macos-ST-[^/]*/bin]x/test.j2objc.pb.m,"); } @Test @@ -368,7 +368,7 @@ public void testJavaProtoLibraryWithProtoLibrary_external() throws Exception { assertThat(classMappingFilesList.get(0).getExecPathString()) .containsMatch( - "/applebin_macos-darwin_x86_64-fastbuild-ST-[^/]*/bin/external/bla/foo/test.clsmap.properties"); + "/darwin_x86_64-fastbuild-applebin_macos-ST-[^/]*/bin/external/bla/foo/test.clsmap.properties"); ObjcProvider objcProvider = target.get(ObjcProvider.STARLARK_CONSTRUCTOR); CcCompilationContext ccCompilationContext = @@ -376,10 +376,10 @@ public void testJavaProtoLibraryWithProtoLibrary_external() throws Exception { assertThat(ccCompilationContext.getDeclaredIncludeSrcs().toList().toString()) .containsMatch( - "/applebin_macos-darwin_x86_64-fastbuild-ST-[^/]*/bin]external/bla/foo/test.j2objc.pb.h"); + "/darwin_x86_64-fastbuild-applebin_macos-ST-[^/]*/bin]external/bla/foo/test.j2objc.pb.h"); assertThat(objcProvider.get(ObjcProvider.SOURCE).toList().toString()) .containsMatch( - "/applebin_macos-darwin_x86_64-fastbuild-ST-[^/]*/bin]external/bla/foo/test.j2objc.pb.m"); + "/darwin_x86_64-fastbuild-applebin_macos-ST-[^/]*/bin]external/bla/foo/test.j2objc.pb.m"); assertThat(ccCompilationContext.getIncludeDirs()) .contains( getConfiguration(target) diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java index 016e5638f2b826..1137c70bb46543 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java @@ -25,6 +25,7 @@ import com.google.common.base.Optional; import com.google.common.base.Splitter; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.truth.Correspondence; import com.google.devtools.build.lib.actions.Action; @@ -41,6 +42,7 @@ import com.google.devtools.build.lib.analysis.config.CompilationMode; import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException; +import com.google.devtools.build.lib.analysis.config.OutputPathMnemonicComputer; import com.google.devtools.build.lib.analysis.config.transitions.SplitTransition; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.analysis.util.ScratchAttributeWriter; @@ -56,7 +58,6 @@ import com.google.devtools.build.lib.rules.cpp.CcInfo; import com.google.devtools.build.lib.rules.cpp.CppLinkAction; import com.google.devtools.build.lib.rules.objc.CompilationSupport.ExtraLinkArgs; -import com.google.devtools.build.lib.skyframe.BuildConfigurationFunction; import com.google.devtools.build.lib.testutil.Scratch; import com.google.devtools.build.lib.testutil.TestConstants; import com.google.devtools.build.lib.vfs.PathFragment; @@ -166,8 +167,8 @@ private String configurationDir( transitionedConfig.get(AppleCommandLineOptions.class).appleSplitCpu = arch; hash = "-" - + BuildConfigurationFunction.computeNameFragmentWithDiff( - transitionedConfig, targetConfig.getOptions()); + + OutputPathMnemonicComputer.computeNameFragmentWithDiff( + transitionedConfig, targetConfig.getOptions(), ImmutableSet.of("cpu")); } switch (configurationDistinguisher) { @@ -179,7 +180,7 @@ private String configurationDir( TestConstants.PRODUCT_NAME, arch, minOsSegment, modeSegment); case APPLEBIN_IOS: return String.format( - "%1$s-out/ios-%2$s%4$s-%3$s-ios_%2$s-%5$s%6$s/", + "%1$s-out/ios_%2$s-%5$s-ios-%2$s%4$s-%3$s%6$s/", TestConstants.PRODUCT_NAME, arch, configurationDistinguisher.toString().toLowerCase(Locale.US), @@ -188,7 +189,7 @@ private String configurationDir( hash); case APPLEBIN_WATCHOS: return String.format( - "%1$s-out/watchos-%2$s%4$s-%3$s-watchos_%2$s-%5$s%6$s/", + "%1$s-out/watchos_%2$s-%5$s-watchos-%2$s%4$s-%3$s%6$s/", TestConstants.PRODUCT_NAME, arch, configurationDistinguisher.toString().toLowerCase(Locale.US), 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 d1b8437c4f185f..d1ad00fd6cf201 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 @@ -943,11 +943,11 @@ public void testReportedConfigurations() throws Exception { new GenericBuildEvent( testId("Initial"), ImmutableSet.of(ProgressEvent.INITIAL_PROGRESS_UPDATE)); BuildConfigurationValue configuration = - BuildConfigurationValue.create( + BuildConfigurationValue.createForTesting( defaultBuildOptions, + "some_mnemonic", "workspace", /* siblingRepositoryLayout= */ false, - /* transitionDirectoryNameFragment= */ "", new BlazeDirectories( new ServerDirectories(outputBase, outputBase, outputBase), rootDirectory, diff --git a/src/test/shell/integration/starlark_configurations_test.sh b/src/test/shell/integration/starlark_configurations_test.sh index b1a6c3147903ae..b5ad195218098a 100755 --- a/src/test/shell/integration/starlark_configurations_test.sh +++ b/src/test/shell/integration/starlark_configurations_test.sh @@ -622,7 +622,7 @@ my_rule(name = 'test') EOF bazel build //test:test >& "$TEST_log" || exit_code="$?" assert_equals 1 "$exit_code" || fail "Expected exit code 1" - expect_log "CPU name '//bad:cpu'" + expect_log "CPU/Platform descriptor '//bad:cpu'" expect_log "is invalid as part of a path: must not contain /" }