diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkExecTransitionLoader.java b/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkExecTransitionLoader.java index 730d3335732d83..3167cae9fc30cc 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkExecTransitionLoader.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkExecTransitionLoader.java @@ -102,7 +102,20 @@ public static Optional loadStarlarkExecTran flagName, userRef, parsedRef.starlarkSymbolName() + " is not a Starlark transition"); } return Optional.of( - new StarlarkAttributeTransitionProvider((StarlarkDefinedConfigTransition) transition)); + new StarlarkExecTransitionProvider((StarlarkDefinedConfigTransition) transition)); + } + + /** A marker class to distinguish the exec transition from other starlark transitions. */ + static class StarlarkExecTransitionProvider extends StarlarkAttributeTransitionProvider { + StarlarkExecTransitionProvider(StarlarkDefinedConfigTransition execTransition) { + super(execTransition); + } + + @Override + public boolean allowImmutableFlagChanges() { + // The exec transition must be allowed to change otherwise immutable flags. + return true; + } } /** 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 b31d4f0f0ae0fe..e61f6e59309b6c 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 @@ -45,6 +45,7 @@ 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; @@ -76,7 +77,15 @@ public final class FunctionTransitionUtil { * incoming {@link BuildOptions}. For native options, this involves a preprocess step of * converting options to their "command line form". * - *

Also validate that transitions output the declared results. + *

Also perform validation on the inputs and outputs: + * + *

    + *
  1. Ensure that all native input options exist + *
  2. Ensure that all native output options exist + *
  3. Ensure that there are no attempts to update the {@code --define} option. + *
  4. Ensure that no {@link OptionMetadataTag#IMMUTABLE immutable} native options are updated. + *
  5. Ensure that transitions output all of the declared options. + *
* * @param fromOptions the pre-transition build options * @param starlarkTransition the transition to apply @@ -87,15 +96,19 @@ public final class FunctionTransitionUtil { static ImmutableMap applyAndValidate( BuildOptions fromOptions, StarlarkDefinedConfigTransition starlarkTransition, + boolean allowImmutableFlagChanges, StructImpl attrObject, EventHandler handler) throws InterruptedException { try { - checkForDenylistedOptions(starlarkTransition); - // TODO(waltl): Consider building this once and using it across different split transitions, // or reusing BuildOptionDetails. ImmutableMap optionInfoMap = OptionInfo.buildMapFrom(fromOptions); + + validateInputOptions(starlarkTransition.getInputs(), optionInfoMap); + validateOutputOptions( + starlarkTransition.getOutputs(), allowImmutableFlagChanges, optionInfoMap); + ImmutableMap settings = buildSettings(fromOptions, optionInfoMap, starlarkTransition); @@ -236,13 +249,83 @@ private static Map handleImplicitPlatformChange( .buildOrThrow(); } - private static void checkForDenylistedOptions(StarlarkDefinedConfigTransition transition) + private static boolean isNativeOptionValid( + ImmutableMap optionInfoMap, String flag) { + String optionName = flag.substring(COMMAND_LINE_OPTION_PREFIX.length()); + + // Make sure the option exists. + return optionInfoMap.containsKey(optionName); + } + + /** + * Check if a native option is immutable. + * + * @return whether or not the option is immutable + * @throws VerifyException if the option does not exist + */ + private static boolean isNativeOptionImmutable( + ImmutableMap optionInfoMap, String flag) { + String optionName = flag.substring(COMMAND_LINE_OPTION_PREFIX.length()); + OptionInfo optionInfo = optionInfoMap.get(optionName); + if (optionInfo == null) { + throw new VerifyException( + "Cannot check if option " + flag + " is immutable: it does not exist"); + } + return optionInfo.hasOptionMetadataTag(OptionMetadataTag.IMMUTABLE); + } + + private static void validateInputOptions( + ImmutableList options, ImmutableMap optionInfoMap) + throws ValidationException { + ImmutableList invalidNativeOptions = + options.stream() + .filter(IS_NATIVE_OPTION) + .filter(optionName -> !isNativeOptionValid(optionInfoMap, optionName)) + .collect(toImmutableList()); + if (!invalidNativeOptions.isEmpty()) { + throw ValidationException.format( + "transition inputs [%s] do not correspond to valid settings", + Joiner.on(", ").join(invalidNativeOptions)); + } + } + + private static void validateOutputOptions( + ImmutableList options, + boolean allowImmutableFlagChanges, + ImmutableMap optionInfoMap) throws ValidationException { - if (transition.getOutputs().contains("//command_line_option:define")) { + if (options.contains("//command_line_option:define")) { throw new ValidationException( "Starlark transition on --define not supported - try using build settings" + " (https://bazel.build/rules/config#user-defined-build-settings)."); } + + // TODO: blaze-configurability - Move the checks for incompatible and experimental flags to here + // (currently in ConfigGlobalLibrary.validateBuildSettingKeys). + + ImmutableList invalidNativeOptions = + options.stream() + .filter(IS_NATIVE_OPTION) + .filter(optionName -> !isNativeOptionValid(optionInfoMap, optionName)) + .collect(toImmutableList()); + if (!invalidNativeOptions.isEmpty()) { + throw ValidationException.format( + "transition outputs [%s] do not correspond to valid settings", + Joiner.on(", ").join(invalidNativeOptions)); + } + + if (!allowImmutableFlagChanges) { + ImmutableList immutableNativeOptions = + options.stream() + .filter(IS_NATIVE_OPTION) + .filter(optionName -> isNativeOptionImmutable(optionInfoMap, optionName)) + .collect(toImmutableList()); + if (!immutableNativeOptions.isEmpty()) { + throw ValidationException.format( + "transition outputs [%s] cannot be changed: they are immutable", + Joiner.on(", ").join(immutableNativeOptions)); + } + } } /** @@ -389,6 +472,7 @@ private static BuildOptions applyTransition( } else { // The transition changes a native option. String optionName = optionKey.substring(COMMAND_LINE_OPTION_PREFIX.length()); + OptionInfo optionInfo = optionInfoMap.get(optionName); // Convert NoneType to null. if (optionValue instanceof NoneType) { @@ -409,12 +493,6 @@ private static BuildOptions applyTransition( optionValue = ImmutableMap.copyOf(((Map) optionValue)); } try { - if (!optionInfoMap.containsKey(optionName)) { - throw ValidationException.format( - "transition output '%s' does not correspond to a valid setting", entry.getKey()); - } - - OptionInfo optionInfo = optionInfoMap.get(optionName); OptionDefinition def = optionInfo.getDefinition(); Field field = def.getField(); // TODO(b/153867317): check for crashing options types in this logic. diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttributeTransitionProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttributeTransitionProvider.java index 69be9a9ecbb659..874bcc2fd0446a 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttributeTransitionProvider.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttributeTransitionProvider.java @@ -80,6 +80,10 @@ public SplitTransition create(AttributeTransitionData data) { starlarkDefinedConfigTransition, (ConfiguredAttributeMapper) attributeMap); } + public boolean allowImmutableFlagChanges() { + return false; + } + @Override public TransitionType transitionType() { return TransitionType.ATTRIBUTE; @@ -137,7 +141,12 @@ public final ImmutableMap split( // we just use the original BuildOptions and trust the transition's enforcement logic. BuildOptions buildOptions = buildOptionsView.underlying(); ImmutableMap res = - applyAndValidate(buildOptions, starlarkDefinedConfigTransition, attrObject, eventHandler); + applyAndValidate( + buildOptions, + starlarkDefinedConfigTransition, + allowImmutableFlagChanges(), + attrObject, + eventHandler); if (res == null) { return ImmutableMap.of("error", buildOptions.clone()); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleTransitionProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleTransitionProvider.java index 55aa5c0d7b2abd..079d00c30f5f13 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleTransitionProvider.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleTransitionProvider.java @@ -93,6 +93,10 @@ public TransitionType transitionType() { return TransitionType.RULE; } + public boolean allowImmutableFlagChanges() { + return false; + } + private FunctionPatchTransition createTransition( Rule rule, ImmutableMap configConditions, String configHash) { LinkedHashMap attributes = new LinkedHashMap<>(); @@ -198,7 +202,12 @@ public BuildOptions patch(BuildOptionsView buildOptionsView, EventHandler eventH // we just use the original BuildOptions and trust the transition's enforcement logic. BuildOptions buildOptions = buildOptionsView.underlying(); Map result = - applyAndValidate(buildOptions, starlarkDefinedConfigTransition, attrObject, eventHandler); + applyAndValidate( + buildOptions, + starlarkDefinedConfigTransition, + allowImmutableFlagChanges(), + attrObject, + eventHandler); if (result == null) { return buildOptions.clone(); } 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 dbc09a8af31c70..c8ed02dc3c0498 100644 --- a/src/main/java/com/google/devtools/common/options/OptionFilterDescriptions.java +++ b/src/main/java/com/google/devtools/common/options/OptionFilterDescriptions.java @@ -187,7 +187,8 @@ 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.IMMUTABLE, "This option cannot be changed in a transition."); 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 a5c8580850fd64..f70e6c03b6f15c 100644 --- a/src/main/java/com/google/devtools/common/options/OptionMetadataTag.java +++ b/src/main/java/com/google/devtools/common/options/OptionMetadataTag.java @@ -55,9 +55,13 @@ public enum OptionMetadataTag { * *

These should be in category {@link OptionDocumentationCategory.UNDOCUMENTED}. */ - INTERNAL(4); + INTERNAL(4), // reserved TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES(5) + // reserved EXPLICIT_IN_OUTPUT_PATH(6) + + /** Options which are IMMUTABLE cannot be changed in (non-exec) Starlark transitions. */ + IMMUTABLE(7); private final int value; diff --git a/src/main/protobuf/option_filters.proto b/src/main/protobuf/option_filters.proto index 8fba26438130a3..629e006888a318 100644 --- a/src/main/protobuf/option_filters.proto +++ b/src/main/protobuf/option_filters.proto @@ -55,5 +55,7 @@ enum OptionMetadataTag { INTERNAL = 4; reserved "TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES"; reserved 5; + reserved "EXPLICIT_IN_OUTPUT_PATH"; reserved 6; + IMMUTABLE = 7; } 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 81879bfd222097..8fbe307dc6319b 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 @@ -810,8 +810,8 @@ public void testInvalidNativeOptionOutput() throws Exception { reporter.removeHandler(failFastHandler); getConfiguredTarget("//test/starlark:test"); assertContainsEvent( - "transition output '//command_line_option:foobarbaz' " - + "does not correspond to a valid setting"); + "transition outputs [//command_line_option:foobarbaz] " + + "do not correspond to valid settings"); } @Test @@ -955,8 +955,8 @@ public void testInvalidNativeOptionOutput_analysisTest() throws Exception { reporter.removeHandler(failFastHandler); getConfiguredTarget("//test/starlark:test"); assertContainsEvent( - "transition output '//command_line_option:foobarbaz' " - + "does not correspond to a valid setting"); + "transition outputs [//command_line_option:foobarbaz] " + + "do not correspond to valid settings"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkTransitionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkTransitionTest.java index 8afd6fd96f513c..d4894744537a9b 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkTransitionTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkTransitionTest.java @@ -15,9 +15,18 @@ import static com.google.common.truth.Truth.assertThat; +import com.google.devtools.build.lib.analysis.config.BuildOptions; +import com.google.devtools.build.lib.analysis.config.Fragment; +import com.google.devtools.build.lib.analysis.config.FragmentOptions; +import com.google.devtools.build.lib.analysis.config.RequiresOptions; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.testutil.Scratch; +import com.google.devtools.build.lib.testutil.TestRuleClassProvider; +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 java.util.Map; import org.junit.Test; import org.junit.runner.RunWith; @@ -30,6 +39,43 @@ */ @RunWith(JUnit4.class) public class StarlarkTransitionTest extends BuildViewTestCase { + + /** Extra options for this test. */ + public static class DummyTestOptions extends FragmentOptions { + public DummyTestOptions() {} + + @Option( + name = "immutable_option", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = {OptionEffectTag.NO_OP}, + defaultValue = "super secret", + metadataTags = {OptionMetadataTag.IMMUTABLE}) + public String immutableOption; + } + + /** Test fragment. */ + @RequiresOptions(options = {DummyTestOptions.class}) + public static final class DummyTestOptionsFragment extends Fragment { + private final BuildOptions buildOptions; + + public DummyTestOptionsFragment(BuildOptions buildOptions) { + this.buildOptions = buildOptions; + } + + // Getter required to satisfy AutoCodec. + public BuildOptions getBuildOptions() { + return buildOptions; + } + } + + @Override + protected ConfiguredRuleClassProvider createRuleClassProvider() { + ConfiguredRuleClassProvider.Builder builder = new ConfiguredRuleClassProvider.Builder(); + TestRuleClassProvider.addStandardRules(builder); + builder.addConfigurationFragment(DummyTestOptionsFragment.class); + return builder.build(); + } + static void writeAllowlistFile(Scratch scratch) throws Exception { scratch.overwriteFile( "tools/allowlists/function_transition_allowlist/BUILD", @@ -240,4 +286,30 @@ public void testAliasChangeRerunsTransitionTest() throws Exception { getConfiguration(getConfiguredTarget("//test:foo")).getOptions().getStarlarkOptions()) .containsExactly(Label.parseCanonicalUnchecked("//options:usually_orange"), "orange-eaten"); } + + @Test + public void testChangingImmutableOptionFails() throws Exception { + scratch.file( + "test/defs.bzl", + "def _transition_impl(settings, attr):", + " return {'//command_line_option:immutable_option': 'something_else'}", + "_transition = transition(", + " implementation = _transition_impl,", + " inputs = [],", + " outputs = ['//command_line_option:immutable_option'],", + ")", + "def _impl(ctx):", + " return []", + "state = rule(", + " implementation = _impl,", + " cfg = _transition,", + ")"); + scratch.file("test/BUILD", "load('//test:defs.bzl', 'state')", "state(name = 'arizona')"); + + reporter.removeHandler(failFastHandler); + getConfiguredTarget("//test:arizona"); + assertContainsEvent( + "transition outputs [//command_line_option:immutable_option] cannot be changed: they are" + + " immutable"); + } }