From 2127e4595b1cad8baae9647dbf3cc5607bc4c8c1 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 7 Oct 2024 13:03:18 -0700 Subject: [PATCH] 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: 683295471 Change-Id: Ia707c2934ea34546fed3230acef62b504b24c8a2 --- src/main/cpp/option_processor.cc | 2 + .../devtools/build/lib/analysis/Project.java | 3 + .../lib/buildtool/AnalysisPhaseRunner.java | 2 + .../build/lib/buildtool/BuildRequest.java | 78 ++++----- .../build/lib/buildtool/BuildTool.java | 5 +- .../build/lib/skyframe/SkyframeExecutor.java | 5 + .../lib/skyframe/config/FlagSetFunction.java | 72 +++++++-- .../lib/skyframe/config/FlagSetValue.java | 20 ++- .../serialization/ImmutableSetCodec.java | 2 +- .../build/lib/testing/common/FakeOptions.java | 6 + .../common/options/GlobalRcUtils.java | 34 ++++ .../common/options/OptionsParser.java | 42 ++++- .../common/options/OptionsProvider.java | 27 +++- .../buildtool/util/BlazeRuntimeWrapper.java | 5 +- .../skyframe/config/FlagSetsFunctionTest.java | 149 ++++++++++++++++++ .../common/options/OptionsParserTest.java | 16 ++ src/test/shell/integration/BUILD | 11 ++ src/test/shell/integration/flagset_test.sh | 100 ++++++++++++ 18 files changed, 509 insertions(+), 70 deletions(-) create mode 100644 src/main/java/com/google/devtools/common/options/GlobalRcUtils.java create mode 100755 src/test/shell/integration/flagset_test.sh diff --git a/src/main/cpp/option_processor.cc b/src/main/cpp/option_processor.cc index 9fb942277771fa..df08aa0acd7725 100644 --- a/src/main/cpp/option_processor.cc +++ b/src/main/cpp/option_processor.cc @@ -657,7 +657,9 @@ std::vector OptionProcessor::GetBlazercAndEnvCommandArgs( const std::vector& env) { // Provide terminal options as coming from the least important rc file. std::vector 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 0353affda46b43..2facd4927bdabb 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 ImmutableMultimap findProjectFiles( public static FlagSetValue modifyBuildOptionsWithFlagSets( Label projectFile, BuildOptions targetOptions, + ImmutableSet userOptions, boolean enforceCanonicalConfigs, ExtendedEventHandler eventHandler, SkyframeExecutor skyframeExecutor) @@ -130,6 +132,7 @@ public static FlagSetValue modifyBuildOptionsWithFlagSets( projectFile, targetOptions.get(CoreOptions.class).sclConfig, targetOptions, + userOptions, enforceCanonicalConfigs); EvaluationResult 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 02f3a22bdbb5d1..b0ce8b443f4ae0 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 @@ enum ProjectFileFeature { static ProjectEvaluationResult evaluateProjectFile( BuildRequest request, BuildOptions buildOptions, + ImmutableSet userOptions, TargetPatternPhaseValue targetPatternPhaseValue, CommandEnvironment env) throws LoadingFailedException, InvalidConfigurationException { @@ -263,6 +264,7 @@ static ProjectEvaluationResult evaluateProjectFile( 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 34965720ca1145..067fa91a0a2cdf 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 @@ public BuildRequest build() { /** 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 @@ public BuildRequest build() { private final boolean runTests; private final boolean checkForActionConflicts; private final boolean reportIncompatibleTargets; + private final ImmutableSet userOptions; private BuildRequest( String commandName, @@ -224,6 +222,8 @@ private BuildRequest( 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 @@ public Map getExplicitStarlarkOptions( } /** - * 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 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 @@ boolean isRunningInEmacs() { 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 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 @@ public T getOptions(Class clazz) { 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 @@ public PackageOptions getPackageOptions() { 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 @@ public boolean getKeepGoing() { 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 @@ public List validateOptions() { 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 @@ public TopLevelArtifactContext getTopLevelArtifactContext() { getOptions(BuildEventProtocolOptions.class).expandFilesets, getOptions(BuildEventProtocolOptions.class).fullyResolveFilesetSymlinks, OutputGroupInfo.determineOutputGroups( - buildOptions.outputGroups, validationMode(), /*shouldRunTests=*/ shouldRunTests())); + buildOptions.outputGroups, validationMode(), /* shouldRunTests= */ shouldRunTests())); } public ImmutableList 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 7cb5062045baf2..5ac84b3494a3e3 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 @@ -236,7 +236,8 @@ public void buildTargets(BuildRequest request, BuildResult result, TargetValidat env.setWorkspaceName(targetPatternPhaseValue.getWorkspaceName()); ProjectEvaluationResult projectEvaluationResult = - evaluateProjectFile(request, buildOptions, targetPatternPhaseValue, env); + evaluateProjectFile( + request, buildOptions, request.getUserOptions(), targetPatternPhaseValue, env); var analysisCachingDeps = RemoteAnalysisCachingDependenciesProviderImpl.forAnalysis( @@ -1074,6 +1075,7 @@ public static PathFragmentPrefixTrie getWorkingSetMatcherForSkyfocus( /** Creates a BuildOptions class for the given options taken from an {@link OptionsProvider}. */ public static BuildOptions applySclConfigs( BuildOptions buildOptionsBeforeFlagSets, + ImmutableSet userOptions, Label projectFile, boolean enforceCanonicalConfigs, SkyframeExecutor skyframeExecutor, @@ -1084,6 +1086,7 @@ public static BuildOptions applySclConfigs( 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 da79fb421cd145..f541dbdc0729d3 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 @@ public ImmutableMap getExplicitStarlarkOptions( java.util.function.Predicate filter) { return ImmutableMap.of(); } + + @Override + public ImmutableSet 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 5b95668fbbed8c..b40d4b5687d4be 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; @@ -84,6 +90,9 @@ public SkyValue compute(SkyKey skyKey, Environment env) key.getSclConfig(), key.enforceCanonical(), env.getListener()); + if (key.enforceCanonical()) { + validateNoExtraFlagsSet(key.getTargetOptions(), key.getUserOptions()); + } ParsedFlagsValue parsedFlags = parseFlags(sclConfigAsStarlarkList, env); if (parsedFlags == null) { return null; @@ -119,18 +128,9 @@ private ImmutableList getSclConfig( // is silently consider a no-op. return sclConfigValue == null ? ImmutableList.of() : ImmutableList.copyOf(sclConfigValue); } else if (sclConfigName.isEmpty()) { + ImmutableList defaultConfigValue = ImmutableList.of(); try { - ImmutableList 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 +140,15 @@ private ImmutableList getSclConfig( e.getMessage(), supportedConfigsDesc(projectFile, supportedConfigs))), Transience.PERSISTENT); } + 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 +159,11 @@ sclConfigName, supportedConfigsDesc(projectFile, supportedConfigs))), Transience.PERSISTENT); } + eventHandler.handle( + Event.info( + String.format( + "Applying flags from the config defined in %s: %s ", projectFile, sclConfigValue))); + return ImmutableList.copyOf(sclConfigValue); } @@ -176,6 +190,42 @@ private static ImmutableList validateDefaultConfig( 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 userOptions) + throws FlagSetFunctionException { + ImmutableList.Builder allOptionsAsStringsBuilder = new ImmutableList.Builder<>(); + targetOptions.getStarlarkOptions().keySet().stream() + .map(Object::toString) + .forEach(allOptionsAsStringsBuilder::add); + for (FragmentOptions fragmentOptions : targetOptions.getNativeOptions()) { + fragmentOptions.asMap().keySet().forEach(allOptionsAsStringsBuilder::add); + } + ImmutableList allOptionsAsStrings = allOptionsAsStringsBuilder.build(); + ImmutableSet overlap = + userOptions.stream() + .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 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 cf58fb9ad1fc62..425163bf53f60d 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 @@ public static final class Key implements SkyKey { private final Label projectFile; private final String sclConfig; private final BuildOptions targetOptions; + private final ImmutableSet userOptions; private final boolean enforceCanonical; @@ -47,16 +49,23 @@ public Key( Label projectFile, @Nullable String sclConfig, BuildOptions targetOptions, + ImmutableSet 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 userOptions, + boolean enforceCanonical) { + return interner.intern( + new Key(projectFile, sclConfig, targetOptions, userOptions, enforceCanonical)); } public Label getProjectFile() { @@ -71,6 +80,10 @@ public BuildOptions getTargetOptions() { return targetOptions; } + public ImmutableSet 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 @@ public boolean equals(Object o) { 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 97a9109d0182fe..3ab6951076700a 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 { +public final class ImmutableSetCodec extends DeferredObjectCodec { // 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 71f4e4171dec17..8d5ae92d46f910 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 @@ public Map getExplicitStarlarkOptions( Predicate filter) { return ImmutableMap.of(); } + + @Override + public ImmutableSet 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 00000000000000..7d8da990f889f4 --- /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 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 0fa39eed4b51ec..8678e1070a2c83 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 @@ static OptionsData getOptionsDataInternal(Class optionsCl 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 @@ public Builder ignoreInternalOptions(boolean ignoreInternalOptions) { 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 @@ public Builder withConversionContext(Object conversionContext) { /** 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 @@ public Builder toBuilder() { private final List residue = new ArrayList<>(); private final List postDoubleDashResidue = new ArrayList<>(); private final boolean allowResidue; + private final boolean ignoreUserOptions; + private ImmutableSortedMap starlarkOptions = ImmutableSortedMap.of(); private final Map 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 @@ public List canonicalize() { return impl.asCanonicalizedList(); } + @Override + public ImmutableSet getUserOptions() { + if (ignoreUserOptions) { + return ImmutableSet.of(); + } + ImmutableSet.Builder 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 getOptionDefinitions( Class 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 b45f88798b3185..2974e7444f64d4 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 @@ public ImmutableMap getExplicitStarlarkOptions( Predicate filter) { return ImmutableMap.of(); } + + @Override + public ImmutableSet 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. * - *

The returned options should be treated by library code as immutable and - * a provider is permitted to return the same options instance multiple times. + *

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 getOptions(Class optionsClass); + @Nullable + O getOptions(Class optionsClass); /** * Returns the starlark options in a name:value map. @@ -68,4 +75,10 @@ public ImmutableMap getExplicitStarlarkOptions( * the given filter criteria. */ Map getExplicitStarlarkOptions(Predicate 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 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 c25f6b91032eca..a6d23d39ec9cf6 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 @@ private OptionsParser createOptionsParser(Command commandAnnotation) { 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 293d27151954aa..a61a2e32274844 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 @@ public void flagSetsFunction_returns_modified_buildOptions() throws Exception { Label.parseCanonical("//test:PROJECT.scl"), "test_config", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ false); FlagSetValue flagSetsValue = executeFunction(key); @@ -121,6 +123,7 @@ public void given_unknown_sclConfig_flagSetsFunction_returns_original_buildOptio Label.parseCanonical("//test:PROJECT.scl"), "unknown_config", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ false); FlagSetValue flagSetsValue = executeFunction(key); @@ -139,6 +142,7 @@ public void flagSetsFunction_returns_original_buildOptions() throws Exception { Label.parseCanonical("//test:PROJECT.scl"), "", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ false); FlagSetValue flagSetsValue = executeFunction(key); @@ -159,6 +163,7 @@ public void noEnforceCanonicalConfigs_unknownConfigisNoop() throws Exception { Label.parseCanonical("//test:PROJECT.scl"), "random_config_name", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ false); FlagSetValue flagSetsValue = executeFunction(key); @@ -191,6 +196,7 @@ public void enforceCanonicalConfigsSupportedConfig() throws Exception { Label.parseCanonical("//test:PROJECT.scl"), "test_config", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); FlagSetValue flagSetsValue = executeFunction(key); @@ -202,6 +208,140 @@ public void enforceCanonicalConfigsSupportedConfig() throws Exception { .isEqualTo("test_config_value"); } + @Test + public void enforceCanonicalConfigsExtraNativeFlag_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 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"); @@ -226,6 +366,7 @@ public void enforceCanonicalConfigsUnsupportedConfig() throws Exception { Label.parseCanonical("//test:PROJECT.scl"), "other_config", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); var thrown = assertThrows(Exception.class, () -> executeFunction(key)); @@ -258,6 +399,7 @@ public void enforceCanonicalConfigsNonExistentConfig() throws Exception { Label.parseCanonical("//test:PROJECT.scl"), "non_existent_config", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); var thrown = assertThrows(Exception.class, () -> executeFunction(key)); @@ -290,6 +432,7 @@ public void enforceCanonicalConfigsNoSclConfigFlagNoDefaultConfig() throws Excep Label.parseCanonical("//test:PROJECT.scl"), /* sclConfig= */ "", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); var thrown = assertThrows(Exception.class, () -> executeFunction(key)); @@ -324,6 +467,7 @@ public void enforceCanonicalConfigsNoSclConfigFlagUnsupportedDefaultConfig() thr Label.parseCanonical("//test:PROJECT.scl"), /* sclConfig= */ "", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); var thrown = assertThrows(Exception.class, () -> executeFunction(key)); @@ -359,6 +503,7 @@ public void enforceCanonicalConfigsNoSclConfigFlagNonexistentDefaultConfig() thr Label.parseCanonical("//test:PROJECT.scl"), /* sclConfig= */ "", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); var thrown = assertThrows(Exception.class, () -> executeFunction(key)); @@ -394,6 +539,7 @@ public void enforceCanonicalConfigsNoSclConfigFlagValidDefaultConfig() throws Ex Label.parseCanonical("//test:PROJECT.scl"), /* sclConfig= */ "", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); FlagSetValue flagSetsValue = executeFunction(key); @@ -428,6 +574,7 @@ public void enforceCanonicalConfigsNoSupportedConfigsWithNoSclConfig() throws Ex Label.parseCanonical("//test:PROJECT.scl"), /* sclConfig= */ "", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); FlagSetValue flagSetsValue = executeFunction(key); @@ -455,6 +602,7 @@ public void enforceCanonicalConfigsNoSupportedConfigsWithSclConfig() throws Exce Label.parseCanonical("//test:PROJECT.scl"), /* sclConfig= */ "test_config", buildOptions, + /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); FlagSetValue flagSetsValue = executeFunction(key); @@ -491,6 +639,7 @@ public void clearUserDocumentationOfSupportedConfigs() throws Exception { 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 846d2e7484e87b..a599712bcf89a3 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 @@ public void fallbackOptions_expansionToNegativeBooleanFlag() throws OptionsParsi 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 4342824b8c4e6a..cd530af42bed26 100644 --- a/src/test/shell/integration/BUILD +++ b/src/test/shell/integration/BUILD @@ -676,6 +676,17 @@ sh_test( ], ) +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 00000000000000..4aebbc61847184 --- /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 < test/PROJECT.scl < test/test.bzl <> 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