From 75a16b7107479441274fa983c04264cdf8556479 Mon Sep 17 00:00:00 2001 From: twigg Date: Tue, 16 Nov 2021 13:29:12 -0800 Subject: [PATCH] Add OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH See OptionMetadataTag.java for documentation on what the tag does. Also audited the transitive calls of OutputDirectories.buildMnemonic and annotated options that seemingly can safely have the tag. (Note that some, like AppleConfiguration, have a getOutputDirectory that is too intricate for me to comfortably add the tag to any of the associated options.) Related to https://github.com/bazelbuild/bazel/issues/14023. PiperOrigin-RevId: 410335434 --- .../build/lib/analysis/PlatformOptions.java | 1 + .../lib/analysis/config/CoreOptions.java | 2 + .../starlark/FunctionTransitionUtil.java | 30 ++++--- .../rules/android/AndroidConfiguration.java | 2 +- .../build/lib/rules/cpp/CppOptions.java | 1 + .../build/lib/rules/python/PythonOptions.java | 1 + .../options/OptionFilterDescriptions.java | 5 +- .../common/options/OptionMetadataTag.java | 21 ++++- src/main/protobuf/option_filters.proto | 2 + .../StarlarkAttrTransitionProviderTest.java | 82 ++++++++++++++++--- 10 files changed, 121 insertions(+), 26 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/PlatformOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/PlatformOptions.java index f19f77d2561f84..2c93cf8b61ddc6 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/PlatformOptions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/PlatformOptions.java @@ -99,6 +99,7 @@ public static boolean platformIsDefault(Label platform) { OptionEffectTag.CHANGES_INPUTS, OptionEffectTag.LOADING_AND_ANALYSIS }, + metadataTags = {OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH}, help = "The labels of the platform rules describing the target platforms for the current " + "command.") 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 b9fb5bcf285eb5..87e660f05d21e7 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 @@ -117,6 +117,7 @@ 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; @@ -226,6 +227,7 @@ 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; 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 13ebd9926a5c21..9c449b9d8fa13c 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 @@ -17,6 +17,7 @@ import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition.COMMAND_LINE_OPTION_PREFIX; import static com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition.PATCH_TRANSITION_KEY; +import static java.util.Arrays.stream; import static java.util.stream.Collectors.joining; import com.google.common.base.Joiner; @@ -36,6 +37,7 @@ import com.google.devtools.build.lib.packages.StructImpl; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.common.options.OptionDefinition; +import com.google.devtools.common.options.OptionMetadataTag; import com.google.devtools.common.options.OptionsParser; import com.google.devtools.common.options.OptionsParsingException; import java.lang.reflect.Field; @@ -277,8 +279,9 @@ private static BuildOptions applyTransition( // toOptions being null means the transition hasn't changed anything. We avoid preemptively // cloning it from fromOptions since options cloning is an expensive operation. BuildOptions toOptions = null; - // The names and values of options (Starlark + native) that are different after this transition. - Set convertedNewValues = new HashSet<>(); + // The names of options (Starlark + native) that are different after this transition and must + // be added to "affected by Starlark transition" + Set convertedAffectedOptions = new HashSet<>(); // Starlark options that are different after this transition. We collect all of them, then clone // the build options once with all cumulative changes. Native option changes, in contrast, are // set directly in the BuildOptions instance. The former approach is preferred since it makes @@ -286,22 +289,22 @@ private static BuildOptions applyTransition( // reasons. While not preferred, direct mutation doesn't require expensive cloning. Map changedStarlarkOptions = new LinkedHashMap<>(); for (Map.Entry entry : newValues.entrySet()) { - String optionName = entry.getKey(); + String optionKey = entry.getKey(); Object optionValue = entry.getValue(); - if (!optionName.startsWith(COMMAND_LINE_OPTION_PREFIX)) { + if (!optionKey.startsWith(COMMAND_LINE_OPTION_PREFIX)) { // The transition changes a Starlark option. - Label optionLabel = Label.parseAbsoluteUnchecked(optionName); + Label optionLabel = Label.parseAbsoluteUnchecked(optionKey); Object oldValue = fromOptions.getStarlarkOptions().get(optionLabel); if ((oldValue == null && optionValue != null) || (oldValue != null && optionValue == null) || (oldValue != null && !oldValue.equals(optionValue))) { changedStarlarkOptions.put(optionLabel, optionValue); - convertedNewValues.add(optionLabel.toString()); + convertedAffectedOptions.add(optionLabel.toString()); } } else { // The transition changes a native option. - optionName = optionName.substring(COMMAND_LINE_OPTION_PREFIX.length()); + String optionName = optionKey.substring(COMMAND_LINE_OPTION_PREFIX.length()); // Convert NoneType to null. if (optionValue instanceof NoneType) { @@ -363,7 +366,10 @@ private static BuildOptions applyTransition( toOptions = fromOptions.clone(); } field.set(toOptions.get(optionInfo.getOptionClass()), convertedValue); - convertedNewValues.add(entry.getKey()); + + if (!optionInfo.hasMetadataTag(OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH)) { + convertedAffectedOptions.add(optionKey); + } } } catch (IllegalArgumentException e) { @@ -392,11 +398,11 @@ private static BuildOptions applyTransition( if (starlarkTransition.isForAnalysisTesting()) { // We need to record every time we change a configuration option. // see {@link #updateOutputDirectoryNameFragment} for usage. - convertedNewValues.add("//command_line_option:evaluating for analysis test"); + convertedAffectedOptions.add("//command_line_option:evaluating for analysis test"); toOptions.get(CoreOptions.class).evaluatingForAnalysisTest = true; } - updateAffectedByStarlarkTransition(toOptions.get(CoreOptions.class), convertedNewValues); + updateAffectedByStarlarkTransition(toOptions.get(CoreOptions.class), convertedAffectedOptions); return toOptions; } @@ -502,6 +508,10 @@ Class getOptionClass() { OptionDefinition getDefinition() { return definition; } + + boolean hasMetadataTag(OptionMetadataTag tag) { + return stream(getDefinition().getOptionMetadataTags()).anyMatch(tag::equals); + } } private FunctionTransitionUtil() {} 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 b1822156352bd2..0b0188bcf937f0 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 @@ -201,7 +201,7 @@ public static class Options extends FragmentOptions { converter = ConfigurationDistinguisherConverter.class, documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, effectTags = OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION, - metadataTags = {OptionMetadataTag.INTERNAL}) + metadataTags = {OptionMetadataTag.INTERNAL, OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH}) public ConfigurationDistinguisher configurationDistinguisher; // TODO(blaze-configurability): Mark this as deprecated in favor of --android_platforms. 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 ee1bd0453f18d5..21aa69afe230f0 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 @@ -150,6 +150,7 @@ public String getTypeDescription() { 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/PythonOptions.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java index 599e30242fcdd1..379f344a4674b6 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 @@ -151,6 +151,7 @@ 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/common/options/OptionFilterDescriptions.java b/src/main/java/com/google/devtools/common/options/OptionFilterDescriptions.java index c5569672051569..d85a8c464219a1 100644 --- a/src/main/java/com/google/devtools/common/options/OptionFilterDescriptions.java +++ b/src/main/java/com/google/devtools/common/options/OptionFilterDescriptions.java @@ -181,7 +181,10 @@ 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."); + "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."); 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 cfb9f8f7b07174..7b0bd2bcb9dc7c 100644 --- a/src/main/java/com/google/devtools/common/options/OptionMetadataTag.java +++ b/src/main/java/com/google/devtools/common/options/OptionMetadataTag.java @@ -45,7 +45,7 @@ public enum OptionMetadataTag { * These are flags that should never be set by a user. This tag is used to make sure that options * that form the protocol between the client and the server are not logged. * - *

These should be in category {@code OptionDocumentationCategory.UNDOCUMENTED}. + *

These should be in category {@link OptionDocumentationCategory.UNDOCUMENTED}. */ HIDDEN(3), @@ -53,12 +53,27 @@ public enum OptionMetadataTag { * Options which are INTERNAL are not recognized by the parser at all, and so cannot be used as * flags. * - *

These should be in category {@code OptionDocumentationCategory.UNDOCUMENTED}. + *

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 424e3af2072a27..d931083c40522a 100644 --- a/src/main/protobuf/option_filters.proto +++ b/src/main/protobuf/option_filters.proto @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. syntax = "proto3"; + package options; // option java_api_version = 2; @@ -54,4 +55,5 @@ enum OptionMetadataTag { INTERNAL = 4; reserved "TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES"; reserved 5; + EXPLICIT_IN_OUTPUT_PATH = 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 fe1cb2fa93c4df..222c2cd37310ad 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 @@ -1121,6 +1121,22 @@ private void writeRulesWithAttrTransitionBzl() throws Exception { ")"); } + private CoreOptions getCoreOptions(ConfiguredTarget target) { + return getConfiguration(target).getOptions().get(CoreOptions.class); + } + + private ImmutableMap getStarlarkOptions(ConfiguredTarget target) { + return getConfiguration(target).getOptions().getStarlarkOptions(); + } + + private Object getStarlarkOption(ConfiguredTarget target, String absName) { + return getStarlarkOptions(target).get(Label.parseAbsoluteUnchecked(absName)); + } + + private String getTransitionDirectoryNameFragment(ConfiguredTarget target) { + return getConfiguration(target).getTransitionDirectoryNameFragment(); + } + @Test public void testTransitionOnBuildSetting_fromDefault() throws Exception { writeAllowlistFile(); @@ -1297,20 +1313,64 @@ public void testTransitionOnBuildSetting_onlyTransitionsAffectsDirectory() throw ImmutableList.of("//test/starlark:the-answer=42"))); } - private CoreOptions getCoreOptions(ConfiguredTarget target) { - return getConfiguration(target).getOptions().get(CoreOptions.class); - } + @Test + public void testTransitionOnCompilationMode_hasNoHash() throws Exception { + writeAllowlistFile(); + writeBuildSettingsBzl(); + scratch.file( + "test/starlark/rules.bzl", + "load('//myinfo:myinfo.bzl', 'MyInfo')", + "def _transition_impl(settings, attr):", + " return {", + " '//command_line_option:compilation_mode': attr.cmode_for_dep,", + "}", + "my_transition = transition(", + " implementation = _transition_impl,", + " inputs = [],", + " outputs = ['//command_line_option:compilation_mode']", + ")", + "def _rule_impl(ctx):", + " return MyInfo(dep = ctx.attr.dep)", + "my_rule = rule(", + " implementation = _rule_impl,", + " attrs = {", + " 'dep': attr.label(cfg = my_transition),", + " 'cmode_for_dep': attr.string(),", + " '_allowlist_function_transition': attr.label(", + " default = '//tools/allowlists/function_transition_allowlist'),", + " }", + ")"); + scratch.file( + "test/starlark/BUILD", + "load('//test/starlark:rules.bzl', 'my_rule')", + "load('//test/starlark:build_settings.bzl', 'int_flag')", + "my_rule(name = 'test', dep = ':dep1', cmode_for_dep='opt')", + "my_rule(name = 'dep1', dep = ':dep2', cmode_for_dep='fastbuild')", + "my_rule(name = 'dep2')"); + useConfiguration(ImmutableMap.of(), "--compilation_mode=fastbuild"); - private ImmutableMap getStarlarkOptions(ConfiguredTarget target) { - return getConfiguration(target).getOptions().getStarlarkOptions(); - } + ConfiguredTarget test = getConfiguredTarget("//test/starlark:test"); - private Object getStarlarkOption(ConfiguredTarget target, String absName) { - return getStarlarkOptions(target).get(Label.parseAbsoluteUnchecked(absName)); - } + @SuppressWarnings("unchecked") + ConfiguredTarget dep1 = + Iterables.getOnlyElement( + (List) getMyInfoFromTarget(test).getValue("dep")); - private String getTransitionDirectoryNameFragment(ConfiguredTarget target) { - return getConfiguration(target).getTransitionDirectoryNameFragment(); + @SuppressWarnings("unchecked") + ConfiguredTarget dep2 = + Iterables.getOnlyElement( + (List) getMyInfoFromTarget(dep1).getValue("dep")); + + // Assert transitionDirectoryNameFragment is empty for all configurations + assertThat(getTransitionDirectoryNameFragment(test)).isEmpty(); + assertThat(getTransitionDirectoryNameFragment(dep1)).isEmpty(); + assertThat(getTransitionDirectoryNameFragment(dep2)).isEmpty(); + + // test and dep1 should have different configurations b/c compilation_mode changed + assertThat(getConfiguration(test)).isNotEqualTo(getConfiguration(dep1)); + + // test and dep2 should have identical configurations + assertThat(getConfiguration(test)).isEqualTo(getConfiguration(dep2)); } @Test