When --enforce_project_configs is enabled for a valid --scl_config, do not allow any other flags to be set in the user blazerc (~/.blazerc or --blazerc) or command line. PiperOrigin-RevId: 686252332 Change-Id: I20a1de3a261b9b75141ebe1661ee08e6e6e732d3
diff --git a/src/main/cpp/option_processor.cc b/src/main/cpp/option_processor.cc index 9fb9422..df08aa0 100644 --- a/src/main/cpp/option_processor.cc +++ b/src/main/cpp/option_processor.cc
@@ -657,7 +657,9 @@ const std::vector<std::string>& env) { // Provide terminal options as coming from the least important rc file. std::vector<std::string> result = { + // LINT.IfChange "--rc_source=client", + // LINT.ThenChange(src/main/java/com/google/devtools/common/options/GlobalRcUtils.java) "--default_override=0:common=--isatty=" + blaze_util::ToString(IsStandardTerminal()), "--default_override=0:common=--terminal_columns=" +
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/Project.java b/src/main/java/com/google/devtools/build/lib/analysis/Project.java index 0353aff..2facd49 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/Project.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/Project.java
@@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableMultimap; +import com.google.common.collect.ImmutableSet; 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.InvalidConfigurationException; @@ -120,6 +121,7 @@ public static FlagSetValue modifyBuildOptionsWithFlagSets( Label projectFile, BuildOptions targetOptions, + ImmutableSet<String> userOptions, boolean enforceCanonicalConfigs, ExtendedEventHandler eventHandler, SkyframeExecutor skyframeExecutor) @@ -130,6 +132,7 @@ projectFile, targetOptions.get(CoreOptions.class).sclConfig, targetOptions, + userOptions, enforceCanonicalConfigs); EvaluationResult<SkyValue> result =
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java index 02f3a22..b0ce8b4 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java
@@ -202,6 +202,7 @@ static ProjectEvaluationResult evaluateProjectFile( BuildRequest request, BuildOptions buildOptions, + ImmutableSet<String> userOptions, TargetPatternPhaseValue targetPatternPhaseValue, CommandEnvironment env) throws LoadingFailedException, InvalidConfigurationException { @@ -263,6 +264,7 @@ buildOptions = BuildTool.applySclConfigs( buildOptions, + userOptions, projectFile, request.getBuildOptions().enforceProjectConfigs, env.getSkyframeExecutor(),
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequest.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequest.java index 3496572..067fa91 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequest.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequest.java
@@ -21,6 +21,7 @@ import com.google.common.base.Preconditions; 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.AnalysisOptions; import com.google.devtools.build.lib.analysis.AspectCollection; import com.google.devtools.build.lib.analysis.OutputGroupInfo; @@ -52,10 +53,9 @@ import javax.annotation.Nullable; /** - * A BuildRequest represents a single invocation of the build tool by a user. - * A request specifies a list of targets to be built for a single - * configuration, a pair of output/error streams, and additional options such - * as --keep_going, --jobs, etc. + * A BuildRequest represents a single invocation of the build tool by a user. A request specifies a + * list of targets to be built for a single configuration, a pair of output/error streams, and + * additional options such as --keep_going, --jobs, etc. */ public class BuildRequest implements OptionsProvider { @@ -189,10 +189,7 @@ /** A human-readable description of all the non-default option settings. */ private final String optionsDescription; - /** - * The name of the Blaze command that the user invoked. - * Used for --announce. - */ + /** The name of the Blaze command that the user invoked. Used for --announce. */ private final String commandName; private final OutErr outErr; @@ -205,6 +202,7 @@ private final boolean runTests; private final boolean checkForActionConflicts; private final boolean reportIncompatibleTargets; + private final ImmutableSet<String> userOptions; private BuildRequest( String commandName, @@ -224,6 +222,8 @@ this.targets = targets; this.id = id; this.startTimeMillis = startTimeMillis; + this.userOptions = + options.getUserOptions() == null ? ImmutableSet.of() : options.getUserOptions(); this.optionsCache = Caffeine.newBuilder() .build( @@ -278,15 +278,20 @@ } /** - * Returns a unique identifier that universally identifies this build. + * Returns the list of options that were parsed from either a user blazerc file or the command + * line. */ + @Override + public ImmutableSet<String> getUserOptions() { + return userOptions; + } + + /** Returns a unique identifier that universally identifies this build. */ public UUID getId() { return id; } - /** - * Returns the name of the Blaze command that the user invoked. - */ + /** Returns the name of the Blaze command that the user invoked. */ public String getCommandName() { return commandName; } @@ -295,24 +300,19 @@ return runningInEmacs; } - /** - * Returns true if tests should be run by the build tool. - */ + /** Returns true if tests should be run by the build tool. */ public boolean shouldRunTests() { return runTests; } - /** - * Returns the (immutable) list of targets to build in commandline - * form. - */ + /** Returns the (immutable) list of targets to build in commandline form. */ public List<String> getTargets() { return targets; } /** - * Returns the output/error streams to which errors and progress messages - * should be sent during the fulfillment of this request. + * Returns the output/error streams to which errors and progress messages should be sent during + * the fulfillment of this request. */ public OutErr getOutErr() { return outErr; @@ -324,10 +324,7 @@ return (T) optionsCache.get(clazz).orNull(); } - - /** - * Returns the set of command-line options specified for this request. - */ + /** Returns the set of command-line options specified for this request. */ public BuildRequestOptions getBuildOptions() { return getOptions(BuildRequestOptions.class); } @@ -337,17 +334,12 @@ return getOptions(PackageOptions.class); } - /** - * Returns the set of options related to the loading phase. - */ + /** Returns the set of options related to the loading phase. */ public LoadingOptions getLoadingOptions() { return getOptions(LoadingOptions.class); } - /** - * Returns the set of command-line options related to the view specified for - * this request. - */ + /** Returns the set of command-line options related to the view specified for this request. */ public AnalysisOptions getViewOptions() { return getOptions(AnalysisOptions.class); } @@ -361,24 +353,20 @@ int getLoadingPhaseThreadCount() { return getOptions(LoadingPhaseThreadsOption.class).threads; } - /** - * Returns the set of execution options specified for this request. - */ + + /** Returns the set of execution options specified for this request. */ public ExecutionOptions getExecutionOptions() { return getOptions(ExecutionOptions.class); } - /** - * Returns the human-readable description of the non-default options - * for this build request. - */ + /** Returns the human-readable description of the non-default options for this build request. */ public String getOptionsDescription() { return optionsDescription; } /** - * Return the time (according to System.currentTimeMillis()) at which the - * service of this request was started. + * Return the time (according to System.currentTimeMillis()) at which the service of this request + * was started. */ public long getStartTime() { return startTimeMillis; @@ -403,8 +391,10 @@ int jobs = getBuildOptions().jobs; if (localTestJobs > jobs) { warnings.add( - String.format("High value for --local_test_jobs: %d. This exceeds the value for --jobs: " - + "%d. Only up to %d local tests will run concurrently.", localTestJobs, jobs, jobs)); + String.format( + "High value for --local_test_jobs: %d. This exceeds the value for --jobs: " + + "%d. Only up to %d local tests will run concurrently.", + localTestJobs, jobs, jobs)); } // Validate other BuildRequest options. @@ -423,7 +413,7 @@ getOptions(BuildEventProtocolOptions.class).expandFilesets, getOptions(BuildEventProtocolOptions.class).fullyResolveFilesetSymlinks, OutputGroupInfo.determineOutputGroups( - buildOptions.outputGroups, validationMode(), /*shouldRunTests=*/ shouldRunTests())); + buildOptions.outputGroups, validationMode(), /* shouldRunTests= */ shouldRunTests())); } public ImmutableList<String> getAspects() {
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java index 3092930..55ac41e 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
@@ -235,7 +235,8 @@ env.setWorkspaceName(targetPatternPhaseValue.getWorkspaceName()); ProjectEvaluationResult projectEvaluationResult = - evaluateProjectFile(request, buildOptions, targetPatternPhaseValue, env); + evaluateProjectFile( + request, buildOptions, request.getUserOptions(), targetPatternPhaseValue, env); var analysisCachingDeps = RemoteAnalysisCachingDependenciesProviderImpl.forAnalysis( @@ -1050,6 +1051,7 @@ /** Creates a BuildOptions class for the given options taken from an {@link OptionsProvider}. */ public static BuildOptions applySclConfigs( BuildOptions buildOptionsBeforeFlagSets, + ImmutableSet<String> userOptions, Label projectFile, boolean enforceCanonicalConfigs, SkyframeExecutor skyframeExecutor, @@ -1060,6 +1062,7 @@ Project.modifyBuildOptionsWithFlagSets( projectFile, buildOptionsBeforeFlagSets, + userOptions, enforceCanonicalConfigs, eventHandler, skyframeExecutor);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index 550dcad..2d38b60 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -3254,6 +3254,11 @@ java.util.function.Predicate<? super ParsedOptionDescription> filter) { return ImmutableMap.of(); } + + @Override + public ImmutableSet<String> getUserOptions() { + return ImmutableSet.of(); + } }); }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/config/FlagSetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/config/FlagSetFunction.java index 5b95668..6703112 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/config/FlagSetFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/config/FlagSetFunction.java
@@ -13,10 +13,16 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe.config; +import static com.google.common.collect.ImmutableSet.toImmutableSet; + import com.google.common.base.Preconditions; +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.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.CoreOptions; +import com.google.devtools.build.lib.analysis.config.FragmentOptions; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.Label.RepoContext; import com.google.devtools.build.lib.cmdline.RepositoryName; @@ -83,7 +89,9 @@ projectValue, key.getSclConfig(), key.enforceCanonical(), - env.getListener()); + env.getListener(), + key.getTargetOptions(), + key.getUserOptions()); ParsedFlagsValue parsedFlags = parseFlags(sclConfigAsStarlarkList, env); if (parsedFlags == null) { return null; @@ -102,7 +110,9 @@ ProjectValue sclContent, String sclConfigName, boolean enforceCanonical, - ExtendedEventHandler eventHandler) + ExtendedEventHandler eventHandler, + BuildOptions targetOptions, + ImmutableSet<String> userOptions) throws FlagSetFunctionException { var configs = (Dict<String, Collection<String>>) sclContent.getResidualGlobal(CONFIGS); var sclConfigValue = configs == null ? null : configs.get(sclConfigName); @@ -119,18 +129,9 @@ // is silently consider a no-op. return sclConfigValue == null ? ImmutableList.of() : ImmutableList.copyOf(sclConfigValue); } else if (sclConfigName.isEmpty()) { + ImmutableList<String> defaultConfigValue = ImmutableList.of(); try { - ImmutableList<String> defaultConfigValue = - validateDefaultConfig(defaultConfigName, configs, supportedConfigs); - if (!defaultConfigWarningShown.get()) { - eventHandler.handle( - Event.info( - String.format( - "Applying flags from the default config defined in %s: %s ", - projectFile, defaultConfigValue))); - defaultConfigWarningShown.set(true); - } - return defaultConfigValue; + defaultConfigValue = validateDefaultConfig(defaultConfigName, configs, supportedConfigs); } catch (InvalidProjectFileException e) { // there's no default config set. throw new FlagSetFunctionException( @@ -140,6 +141,16 @@ e.getMessage(), supportedConfigsDesc(projectFile, supportedConfigs))), Transience.PERSISTENT); } + validateNoExtraFlagsSet(targetOptions, userOptions); + if (!defaultConfigWarningShown.get()) { + eventHandler.handle( + Event.info( + String.format( + "Applying flags from the default config defined in %s: %s ", + projectFile, defaultConfigValue))); + defaultConfigWarningShown.set(true); + } + return defaultConfigValue; } else if (!supportedConfigs.containsKey(sclConfigName)) { // This project declares supported configs and user set --scl_config to an unsupported config. throw new FlagSetFunctionException( @@ -150,6 +161,14 @@ Transience.PERSISTENT); } + if (enforceCanonical) { + validateNoExtraFlagsSet(targetOptions, userOptions); + } + eventHandler.handle( + Event.info( + String.format( + "Applying flags from the config defined in %s: %s ", projectFile, sclConfigValue))); + return ImmutableList.copyOf(sclConfigValue); } @@ -176,6 +195,44 @@ return ImmutableList.copyOf(configs.get(defaultConfigName)); } + /** + * Enforces that the user did not set any output-affecting options in a blazerc or on the command + * line. Flags set in global RC files and flags that do not affect outputs are allowed. + */ + // TODO(steinman): Allow user options if they are also part of the --scl_config. + private void validateNoExtraFlagsSet(BuildOptions targetOptions, ImmutableSet<String> userOptions) + throws FlagSetFunctionException { + ImmutableList.Builder<String> allOptionsAsStringsBuilder = new ImmutableList.Builder<>(); + // All potentially conflicting user options also appear in targetOptions + targetOptions.getStarlarkOptions().keySet().stream() + .map(Object::toString) + .forEach(allOptionsAsStringsBuilder::add); + for (FragmentOptions fragmentOptions : targetOptions.getNativeOptions()) { + fragmentOptions.asMap().keySet().forEach(allOptionsAsStringsBuilder::add); + } + ImmutableList<String> allOptionsAsStrings = allOptionsAsStringsBuilder.build(); + ImmutableSet<String> overlap = + userOptions.stream() + // Remove options that aren't part of BuildOptions + .filter( + option -> + allOptionsAsStrings.contains( + Iterables.get(Splitter.on("=").split(option), 0).replaceFirst("--", ""))) + .filter(option -> !option.startsWith("--scl_config")) + .collect(toImmutableSet()); + // TODO(b/341930725): Allow user options if they are also part of the --scl_config. + if (!overlap.isEmpty()) { + throw new FlagSetFunctionException( + new UnsupportedConfigException( + String.format( + "When --enforce_project_configs is set, --scl_config must be the only" + + " configuration-affecting flag in the build. Found %s in the command line" + + " or user blazerc", + overlap)), + Transience.PERSISTENT); + } + } + /** Returns a user-friendly description of project-supported configurations. */ private static String supportedConfigsDesc( Label projectFile, Dict<String, String> supportedConfigs) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/config/FlagSetValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/config/FlagSetValue.java index cf58fb9..425163b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/config/FlagSetValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/config/FlagSetValue.java
@@ -16,6 +16,7 @@ import static com.google.common.base.Strings.nullToEmpty; import com.google.common.base.Verify; +import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.concurrent.ThreadSafety; @@ -40,6 +41,7 @@ private final Label projectFile; private final String sclConfig; private final BuildOptions targetOptions; + private final ImmutableSet<String> userOptions; private final boolean enforceCanonical; @@ -47,16 +49,23 @@ Label projectFile, @Nullable String sclConfig, BuildOptions targetOptions, + ImmutableSet<String> userOptions, boolean enforceCanonical) { this.projectFile = Verify.verifyNotNull(projectFile); this.sclConfig = nullToEmpty(sclConfig); this.targetOptions = Verify.verifyNotNull(targetOptions); + this.userOptions = Verify.verifyNotNull(userOptions); this.enforceCanonical = enforceCanonical; } public static Key create( - Label projectFile, String sclConfig, BuildOptions targetOptions, boolean enforceCanonical) { - return interner.intern(new Key(projectFile, sclConfig, targetOptions, enforceCanonical)); + Label projectFile, + String sclConfig, + BuildOptions targetOptions, + ImmutableSet<String> userOptions, + boolean enforceCanonical) { + return interner.intern( + new Key(projectFile, sclConfig, targetOptions, userOptions, enforceCanonical)); } public Label getProjectFile() { @@ -71,6 +80,10 @@ return targetOptions; } + public ImmutableSet<String> getUserOptions() { + return userOptions; + } + /** * Whether {@code --scl_config} must match an officially supported project configuration. See * {@link com.google.devtools.build.lib.buildtool.BuildRequestOptions#enforceProjectConfigs}. @@ -101,12 +114,13 @@ return Objects.equals(projectFile, key.projectFile) && Objects.equals(sclConfig, key.sclConfig) && Objects.equals(targetOptions, key.targetOptions) + && Objects.equals(userOptions, key.userOptions) && (enforceCanonical == key.enforceCanonical); } @Override public int hashCode() { - return Objects.hash(projectFile, sclConfig, targetOptions, enforceCanonical); + return Objects.hash(projectFile, sclConfig, targetOptions, userOptions, enforceCanonical); } }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/ImmutableSetCodec.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/ImmutableSetCodec.java index 97a9109..3ab6951 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/ImmutableSetCodec.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/ImmutableSetCodec.java
@@ -30,7 +30,7 @@ /** {@link ObjectCodec} for {@link ImmutableSet} and other sets that should be immutable. */ @SuppressWarnings({"rawtypes", "unchecked"}) -final class ImmutableSetCodec extends DeferredObjectCodec<Set> { +public final class ImmutableSetCodec extends DeferredObjectCodec<Set> { // Conversion of the types below to ImmutableSet is sound because the underlying types are hidden // and only referenceable as the Set type.
diff --git a/src/main/java/com/google/devtools/build/lib/testing/common/FakeOptions.java b/src/main/java/com/google/devtools/build/lib/testing/common/FakeOptions.java index 71f4e41..8d5ae92 100644 --- a/src/main/java/com/google/devtools/build/lib/testing/common/FakeOptions.java +++ b/src/main/java/com/google/devtools/build/lib/testing/common/FakeOptions.java
@@ -15,6 +15,7 @@ import com.google.common.collect.ImmutableClassToInstanceMap; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.devtools.common.options.Options; import com.google.devtools.common.options.OptionsBase; import com.google.devtools.common.options.OptionsProvider; @@ -114,4 +115,9 @@ Predicate<? super ParsedOptionDescription> filter) { return ImmutableMap.of(); } + + @Override + public ImmutableSet<String> getUserOptions() { + return ImmutableSet.of(); + } }
diff --git a/src/main/java/com/google/devtools/common/options/GlobalRcUtils.java b/src/main/java/com/google/devtools/common/options/GlobalRcUtils.java new file mode 100644 index 0000000..7d8da99 --- /dev/null +++ b/src/main/java/com/google/devtools/common/options/GlobalRcUtils.java
@@ -0,0 +1,34 @@ +// Copyright 2024 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.common.options; + +import java.util.function.Predicate; + +/** Utility functions for global RC files. */ +final class GlobalRcUtils { + + private GlobalRcUtils() {} + + /** No global RC files in Bazel. Consider "client" options to be global. */ + static final Predicate<ParsedOptionDescription> IS_GLOBAL_RC_OPTION = + // LINT.IfChange + (option) -> { + if (option.getOrigin().getSource() != null + && option.getOrigin().getSource().equals("client")) { + return true; + } + return false; + }; + // LINT.ThenChange(//src/main/cpp/option_processor.cc) +}
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParser.java b/src/main/java/com/google/devtools/common/options/OptionsParser.java index 0fa39ee..8678e10 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParser.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java
@@ -160,6 +160,7 @@ public static final class Builder { private final OptionsParserImpl.Builder implBuilder; private boolean allowResidue = true; + private boolean ignoreUserOptions = false; private Builder(OptionsParserImpl.Builder implBuilder) { this.implBuilder = implBuilder; @@ -232,6 +233,13 @@ return this; } + /** Sets whether the parser should ignore user options. If true, returns no user options. */ + @CanIgnoreReturnValue + public Builder ignoreUserOptions() { + this.ignoreUserOptions = true; + return this; + } + /** Sets the string the parser should look for as an identifier for flag aliases. */ @CanIgnoreReturnValue public Builder withAliasFlag(@Nullable String aliasFlag) { @@ -256,7 +264,7 @@ /** Returns a new {@link OptionsParser}. */ public OptionsParser build() { - return new OptionsParser(implBuilder.build(), allowResidue); + return new OptionsParser(implBuilder.build(), allowResidue, ignoreUserOptions); } } @@ -273,13 +281,16 @@ private final List<String> residue = new ArrayList<>(); private final List<String> postDoubleDashResidue = new ArrayList<>(); private final boolean allowResidue; + private final boolean ignoreUserOptions; + private ImmutableSortedMap<String, Object> starlarkOptions = ImmutableSortedMap.of(); private final Map<String, String> aliases = new HashMap<>(); private boolean success = true; - private OptionsParser(OptionsParserImpl impl, boolean allowResidue) { + private OptionsParser(OptionsParserImpl impl, boolean allowResidue, boolean ignoreUserOptions) { this.impl = impl; this.allowResidue = allowResidue; + this.ignoreUserOptions = ignoreUserOptions; } public Object getConversionContext() { @@ -864,6 +875,33 @@ return impl.asCanonicalizedList(); } + @Override + public ImmutableSet<String> getUserOptions() { + if (ignoreUserOptions) { + return ImmutableSet.of(); + } + ImmutableSet.Builder<String> userOptions = ImmutableSet.builder(); + + return userOptions + .addAll( + asListOfExplicitOptions().stream() + .filter(GlobalRcUtils.IS_GLOBAL_RC_OPTION.negate()) + .filter(option -> !option.getCanonicalForm().contains("default_override")) + .map(option -> option.getCanonicalForm()) + .collect(toImmutableSet())) + .addAll( + impl.getSkippedOptions().stream() + .filter(GlobalRcUtils.IS_GLOBAL_RC_OPTION.negate()) + .map(option -> option.getUnconvertedValue()) + .filter( + o -> + getStarlarkOptions() + .containsKey( + Iterables.get(Splitter.on('=').split(o.replace("--", "")), 0))) + .collect(toImmutableSet())) + .build(); + } + /** Returns all options fields of the given options class, in alphabetic order. */ public static ImmutableList<? extends OptionDefinition> getOptionDefinitions( Class<? extends OptionsBase> optionsClass) {
diff --git a/src/main/java/com/google/devtools/common/options/OptionsProvider.java b/src/main/java/com/google/devtools/common/options/OptionsProvider.java index b45f887..2974e74 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsProvider.java +++ b/src/main/java/com/google/devtools/common/options/OptionsProvider.java
@@ -14,13 +14,14 @@ package com.google.devtools.common.options; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import java.util.Map; import java.util.function.Predicate; import javax.annotation.Nullable; /** - * A read-only interface for options parser results, which only allows to query the options of - * a specific class, but not e.g. the residue any other information pertaining to the command line. + * A read-only interface for options parser results, which only allows to query the options of a + * specific class, but not e.g. the residue any other information pertaining to the command line. */ public interface OptionsProvider { public static final OptionsProvider EMPTY = @@ -41,16 +42,22 @@ Predicate<? super ParsedOptionDescription> filter) { return ImmutableMap.of(); } + + @Override + public ImmutableSet<String> getUserOptions() { + return ImmutableSet.of(); + } }; /** - * Returns the options instance for the given {@code optionsClass}, that is, - * the parsed options, or null if it is not among those available. + * Returns the options instance for the given {@code optionsClass}, that is, the parsed options, + * or null if it is not among those available. * - * <p>The returned options should be treated by library code as immutable and - * a provider is permitted to return the same options instance multiple times. + * <p>The returned options should be treated by library code as immutable and a provider is + * permitted to return the same options instance multiple times. */ - @Nullable <O extends OptionsBase> O getOptions(Class<O> optionsClass); + @Nullable + <O extends OptionsBase> O getOptions(Class<O> optionsClass); /** * Returns the starlark options in a name:value map. @@ -68,4 +75,10 @@ * the given filter criteria. */ Map<String, Object> getExplicitStarlarkOptions(Predicate<? super ParsedOptionDescription> filter); + + /** + * Returns the options that were parsed from either a user blazerc file or the command line as a + * map of option name to option value. + */ + ImmutableSet<String> getUserOptions(); }
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/util/BlazeRuntimeWrapper.java b/src/test/java/com/google/devtools/build/lib/buildtool/util/BlazeRuntimeWrapper.java index c25f6b9..a6d23d3 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/util/BlazeRuntimeWrapper.java +++ b/src/test/java/com/google/devtools/build/lib/buildtool/util/BlazeRuntimeWrapper.java
@@ -310,7 +310,10 @@ Iterables.addAll(options, module.getCommandOptions(commandAnnotation)); } options.addAll(runtime.getRuleClassProvider().getFragmentRegistry().getOptionsClasses()); - return OptionsParser.builder().optionsClasses(options).build(); + // Because the tests that use this class don't set sources for their options, the normal logic + // for determining user options assumes that all options are user options. This causes tests + // that enable PROJECT.scl files to fail, so ignore user options instead. + return OptionsParser.builder().optionsClasses(options).ignoreUserOptions().build(); } void executeNonBuildCommand() throws Exception {
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/config/FlagSetsFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/config/FlagSetsFunctionTest.java index 293d271..1edda61 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/config/FlagSetsFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/config/FlagSetsFunctionTest.java
@@ -17,6 +17,7 @@ import static org.junit.Assert.assertThrows; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; import com.google.devtools.build.lib.analysis.PlatformOptions; import com.google.devtools.build.lib.analysis.config.BuildOptions; @@ -91,6 +92,7 @@ Label.parseCanonical("//test:PROJECT.scl"), "test_config", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ false); FlagSetValue flagSetsValue = executeFunction(key); @@ -121,6 +123,7 @@ Label.parseCanonical("//test:PROJECT.scl"), "unknown_config", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ false); FlagSetValue flagSetsValue = executeFunction(key); @@ -139,6 +142,7 @@ Label.parseCanonical("//test:PROJECT.scl"), "", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ false); FlagSetValue flagSetsValue = executeFunction(key); @@ -159,6 +163,7 @@ Label.parseCanonical("//test:PROJECT.scl"), "random_config_name", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ false); FlagSetValue flagSetsValue = executeFunction(key); @@ -191,6 +196,7 @@ Label.parseCanonical("//test:PROJECT.scl"), "test_config", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); FlagSetValue flagSetsValue = executeFunction(key); @@ -203,6 +209,179 @@ } @Test + public void enforceCanonicalConfigsExtraNativeFlag_withSclConfig_fails() throws Exception { + scratch.file( + "test/build_settings.bzl", + """ +string_flag = rule(implementation = lambda ctx: [], build_setting = config.string(flag = True)) +"""); + scratch.file( + "test/BUILD", + """ + load("//test:build_settings.bzl", "string_flag") + string_flag( + name = "myflag", + build_setting_default = "default", + ) + """); + scratch.file( + "test/PROJECT.scl", + """ + configs = { + "test_config": ['--//test:myflag=test_config_value'], + "other_config": ['--//test:myflag=other_config_value'], + } + supported_configs = { + "test_config": "User documentation for what this config means", + } + """); + setBuildLanguageOptions("--experimental_enable_scl_dialect=true"); + BuildOptions buildOptions = createBuildOptions("--define=foo=bar"); + + FlagSetValue.Key key = + FlagSetValue.Key.create( + Label.parseCanonical("//test:PROJECT.scl"), + "test_config", + buildOptions, + /* userOptions= */ ImmutableSet.of("--define=foo=bar"), + /* enforceCanonical= */ true); + + var thrown = assertThrows(Exception.class, () -> executeFunction(key)); + assertThat(thrown).hasMessageThat().contains("Found [--define=foo=bar]"); + } + + @Test + public void enforceCanonicalConfigsExtraNativeFlag_noSupportedConfigs_passes() throws Exception { + scratch.file( + "test/build_settings.bzl", + """ +string_flag = rule(implementation = lambda ctx: [], build_setting = config.string(flag = True)) +"""); + scratch.file( + "test/BUILD", + """ + load("//test:build_settings.bzl", "string_flag") + string_flag( + name = "myflag", + build_setting_default = "default", + ) + """); + scratch.file( + "test/PROJECT.scl", + """ + configs = { + "test_config": ['--//test:myflag=test_config_value'], + "other_config": ['--//test:myflag=other_config_value'], + } + """); + setBuildLanguageOptions("--experimental_enable_scl_dialect=true"); + BuildOptions buildOptions = createBuildOptions("--define=foo=bar"); + + FlagSetValue.Key key = + FlagSetValue.Key.create( + Label.parseCanonical("//test:PROJECT.scl"), + null, + buildOptions, + /* userOptions= */ ImmutableSet.of("--define=foo=bar"), + /* enforceCanonical= */ true); + + var unused = executeFunction(key); + assertDoesNotContainEvent("--scl_config must be the only configuration-affecting flag"); + } + + @Test + public void enforceCanonicalConfigsExtraStarlarkFlag_fails() throws Exception { + scratch.file( + "test/build_settings.bzl", + """ +string_flag = rule(implementation = lambda ctx: [], build_setting = config.string(flag = True)) +"""); + scratch.file( + "test/BUILD", + """ + load("//test:build_settings.bzl", "string_flag") + string_flag( + name = "myflag", + build_setting_default = "default", + ) + string_flag( + name = "starlark_flags_always_affect_configuration", + build_setting_default = "default", + ) + """); + scratch.file( + "test/PROJECT.scl", + """ + configs = { + "test_config": ['--//test:myflag=test_config_value'], + "other_config": ['--//test:myflag=other_config_value'], + } + supported_configs = { + "test_config": "User documentation for what this config means", + } + """); + setBuildLanguageOptions("--experimental_enable_scl_dialect=true"); + BuildOptions buildOptions = + createBuildOptions("--//test:starlark_flags_always_affect_configuration=yes_they_do"); + + FlagSetValue.Key key = + FlagSetValue.Key.create( + Label.parseCanonical("//test:PROJECT.scl"), + "test_config", + buildOptions, + /* userOptions= */ ImmutableSet.of( + "--//test:starlark_flags_always_affect_configuration=yes_they_do"), + /* enforceCanonical= */ true); + + var thrown = assertThrows(Exception.class, () -> executeFunction(key)); + assertThat(thrown) + .hasMessageThat() + .contains("Found [--//test:starlark_flags_always_affect_configuration=yes_they_do]"); + } + + @Test + public void noEnforceCanonicalConfigsExtraFlag_passes() throws Exception { + scratch.file( + "test/build_settings.bzl", + """ +string_flag = rule(implementation = lambda ctx: [], build_setting = config.string(flag = True)) +"""); + scratch.file( + "test/BUILD", + """ + load("//test:build_settings.bzl", "string_flag") + string_flag( + name = "myflag", + build_setting_default = "default", + ) + """); + scratch.file( + "test/PROJECT.scl", + """ + configs = { + "test_config": ['--//test:myflag=test_config_value'], + "other_config": ['--//test:myflag=other_config_value'], + } + supported_configs = { + "test_config": "User documentation for what this config means", + } + """); + setBuildLanguageOptions("--experimental_enable_scl_dialect=true"); + BuildOptions buildOptions = createBuildOptions("--define=foo=bar"); + + FlagSetValue.Key key = + FlagSetValue.Key.create( + Label.parseCanonical("//test:PROJECT.scl"), + "test_config", + buildOptions, + /* userOptions= */ ImmutableSet.of("--define=foo=bar"), + /* enforceCanonical= */ false); + + var unused = executeFunction(key); + assertDoesNotContainEvent("--scl_config must be the only configuration-affecting flag"); + } + + @Test public void enforceCanonicalConfigsUnsupportedConfig() throws Exception { createStringFlag("//test:myflag", /* defaultValue= */ "default"); scratch.file( @@ -226,6 +405,7 @@ Label.parseCanonical("//test:PROJECT.scl"), "other_config", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); var thrown = assertThrows(Exception.class, () -> executeFunction(key)); @@ -258,6 +438,7 @@ Label.parseCanonical("//test:PROJECT.scl"), "non_existent_config", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); var thrown = assertThrows(Exception.class, () -> executeFunction(key)); @@ -290,6 +471,7 @@ Label.parseCanonical("//test:PROJECT.scl"), /* sclConfig= */ "", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); var thrown = assertThrows(Exception.class, () -> executeFunction(key)); @@ -324,6 +506,7 @@ Label.parseCanonical("//test:PROJECT.scl"), /* sclConfig= */ "", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); var thrown = assertThrows(Exception.class, () -> executeFunction(key)); @@ -359,6 +542,7 @@ Label.parseCanonical("//test:PROJECT.scl"), /* sclConfig= */ "", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); var thrown = assertThrows(Exception.class, () -> executeFunction(key)); @@ -394,6 +578,7 @@ Label.parseCanonical("//test:PROJECT.scl"), /* sclConfig= */ "", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); FlagSetValue flagSetsValue = executeFunction(key); @@ -428,6 +613,7 @@ Label.parseCanonical("//test:PROJECT.scl"), /* sclConfig= */ "", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); FlagSetValue flagSetsValue = executeFunction(key); @@ -455,6 +641,7 @@ Label.parseCanonical("//test:PROJECT.scl"), /* sclConfig= */ "test_config", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); FlagSetValue flagSetsValue = executeFunction(key); @@ -491,6 +678,7 @@ Label.parseCanonical("//test:PROJECT.scl"), /* sclConfig= */ "", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); var thrown = assertThrows(Exception.class, () -> executeFunction(key));
diff --git a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java index 846d2e7..a599712 100644 --- a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java +++ b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java
@@ -2625,6 +2625,22 @@ assertThat(parser.getOptions(ExpandingOptionsFallback.class)).isNull(); } + @Test + public void testOptionsParser_getUserOptions_excludesClientOptions() throws Exception { + OptionsParser parser = + OptionsParser.builder() + .optionsClasses(ExpandingOptions.class, ExpandingOptionsFallback.class) + .build(); + parser.parseWithSourceFunction( + PriorityCategory.RC_FILE, o -> "client", ImmutableList.of("--foo"), null); + assertThat(parser.getUserOptions()).isEmpty(); + + parser.parseWithSourceFunction( + PriorityCategory.RC_FILE, o -> ".bazelrc", ImmutableList.of("--foo"), null); + + assertThat(parser.getUserOptions()).containsExactly("--foo"); + } + private static OptionInstanceOrigin createInvocationPolicyOrigin() { return createInvocationPolicyOrigin(/*implicitDependent=*/ null, /*expandedFrom=*/ null); }
diff --git a/src/test/shell/integration/BUILD b/src/test/shell/integration/BUILD index 4342824..cd530af 100644 --- a/src/test/shell/integration/BUILD +++ b/src/test/shell/integration/BUILD
@@ -676,6 +676,17 @@ ], ) +sh_test( + name = "flagset_test", + size = "large", + srcs = ["flagset_test.sh"], + data = [ + ":test-deps", + "@bazel_tools//tools/bash/runfiles", + ], + shard_count = 3, +) + # Copy protoc into a known location, since //third_party/protobuf:protoc # might be an alias. This is referenced from testenv.sh. genrule(
diff --git a/src/test/shell/integration/flagset_test.sh b/src/test/shell/integration/flagset_test.sh new file mode 100755 index 0000000..4aebbc6 --- /dev/null +++ b/src/test/shell/integration/flagset_test.sh
@@ -0,0 +1,100 @@ +#!/bin/bash +# +# Copyright 2024 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. +# +# An end-to-end test for flagsets. + +# --- begin runfiles.bash initialization --- +set -euo pipefail +if [[ ! -d "${RUNFILES_DIR:-/dev/null}" && ! -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then + if [[ -f "$0.runfiles_manifest" ]]; then + export RUNFILES_MANIFEST_FILE="$0.runfiles_manifest" + elif [[ -f "$0.runfiles/MANIFEST" ]]; then + export RUNFILES_MANIFEST_FILE="$0.runfiles/MANIFEST" + elif [[ -f "$0.runfiles/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then + export RUNFILES_DIR="$0.runfiles" + fi +fi +if [[ -f "${RUNFILES_DIR:-/dev/null}/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then + source "${RUNFILES_DIR}/bazel_tools/tools/bash/runfiles/runfiles.bash" +elif [[ -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then + source "$(grep -m1 "^bazel_tools/tools/bash/runfiles/runfiles.bash " \ + "$RUNFILES_MANIFEST_FILE" | cut -d ' ' -f 2-)" +else + echo >&2 "ERROR: cannot find @bazel_tools//tools/bash/runfiles:runfiles.bash" + exit 1 +fi +# --- end runfiles.bash initialization --- + +source "$(rlocation "io_bazel/src/test/shell/integration_test_setup.sh")" \ + || { echo "integration_test_setup.sh not found!" >&2; exit 1; } + +function set_up_project_file() { + mkdir -p test + cat > test/BUILD <<EOF +load("//test:test.bzl", "string_flag") + +genrule(name='test', outs=['test.txt'], cmd='echo hi > \$@') + +string_flag( + name = "starlark_flags_always_affect_configuration", + build_setting_default = "default", +) +EOF + cat > test/PROJECT.scl <<EOF +configs = { + "test_config": ["--define=foo=bar"], +} +supported_configs = { + "test_config": "User documentation for what this config means", +} +EOF + + touch test/test.bzl + cat > test/test.bzl <<EOF +string_flag = rule(implementation = lambda ctx: [], build_setting = config.string(flag = True)) +EOF +} + +function test_scl_config_plus_user_bazelrc_fails(){ + set_up_project_file + add_to_bazelrc "build '--//test:starlark_flags_always_affect_configuration=yes'" + add_to_bazelrc "build --define=bar=baz" + cat .bazelrc >> test/test.bazelrc + bazel --bazelrc=test/test.bazelrc build --nobuild //test:test --enforce_project_configs --scl_config=test_config --experimental_enable_scl_dialect &> "$TEST_log" && \ + fail "Scl enabled build expected to fail with starlark flag in user bazelrc" + expect_log "--scl_config must be the only configuration-affecting flag" + expect_log "--//test:starlark_flags_always_affect_configuration=yes" + expect_log "--define=bar=baz" +} + +function test_scl_config_plus_command_line_flag_fails(){ + set_up_project_file + bazel build --nobuild //test:test --enforce_project_configs --scl_config=test_config --experimental_enable_scl_dialect --//test:starlark_flags_always_affect_configuration=yes --define=bar=baz &> "$TEST_log" && \ + fail "Scl enabled build expected to fail with command-line flags" + expect_log "--scl_config must be the only configuration-affecting flag" + expect_log "--//test:starlark_flags_always_affect_configuration=yes" + expect_log "--define=bar=baz" +} + +function test_scl_config_plus_expanded_command_line_flag_fails(){ + set_up_project_file + bazel build --nobuild //test:test --enforce_project_configs --scl_config=test_config --experimental_enable_scl_dialect -c opt &> "$TEST_log" && \ + fail "Scl enabled build expected to fail with command line flag" + expect_log "--scl_config must be the only configuration-affecting flag" + expect_log "--compilation_mode=opt" +} + +run_suite "Integration tests for flagsets/scl_config" \ No newline at end of file