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