From a4d900db8c08bd0a6a0dadfa1de67c54bda5aa9f Mon Sep 17 00:00:00 2001 From: "bazel.build machine account" Date: Tue, 8 Oct 2024 17:00:19 +0200 Subject: [PATCH] [8.0.0] Rollback of commit 2127e45 (#23902) *** Reason for rollback *** Breaks blaze self-build *** Original change description *** 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: 683605569 Change-Id: I54cc05b9ccc91e18fc06c97bfaa1c6cb32c47e75 Commit https://github.com/bazelbuild/bazel/commit/a034aa60248f93141f359b9254aee8adf063bfb3 Co-authored-by: Googler --- 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, 70 insertions(+), 509 deletions(-) delete mode 100644 src/main/java/com/google/devtools/common/options/GlobalRcUtils.java delete 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 df08aa0acd7725..9fb942277771fa 100644 --- a/src/main/cpp/option_processor.cc +++ b/src/main/cpp/option_processor.cc @@ -657,9 +657,7 @@ 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 2facd4927bdabb..0353affda46b43 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,7 +18,6 @@ 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; @@ -121,7 +120,6 @@ public static ImmutableMultimap findProjectFiles( public static FlagSetValue modifyBuildOptionsWithFlagSets( Label projectFile, BuildOptions targetOptions, - ImmutableSet userOptions, boolean enforceCanonicalConfigs, ExtendedEventHandler eventHandler, SkyframeExecutor skyframeExecutor) @@ -132,7 +130,6 @@ 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 b0ce8b443f4ae0..02f3a22bdbb5d1 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,7 +202,6 @@ enum ProjectFileFeature { static ProjectEvaluationResult evaluateProjectFile( BuildRequest request, BuildOptions buildOptions, - ImmutableSet userOptions, TargetPatternPhaseValue targetPatternPhaseValue, CommandEnvironment env) throws LoadingFailedException, InvalidConfigurationException { @@ -264,7 +263,6 @@ 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 067fa91a0a2cdf..34965720ca1145 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,7 +21,6 @@ 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; @@ -53,9 +52,10 @@ 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,7 +189,10 @@ 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; @@ -202,7 +205,6 @@ public BuildRequest build() { private final boolean runTests; private final boolean checkForActionConflicts; private final boolean reportIncompatibleTargets; - private final ImmutableSet userOptions; private BuildRequest( String commandName, @@ -222,8 +224,6 @@ 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,20 +278,15 @@ public Map getExplicitStarlarkOptions( } /** - * Returns the list of options that were parsed from either a user blazerc file or the command - * line. + * Returns a unique identifier that universally identifies this build. */ - @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; } @@ -300,19 +295,24 @@ 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,7 +324,10 @@ 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); } @@ -334,12 +337,17 @@ 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); } @@ -353,20 +361,24 @@ 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; @@ -391,10 +403,8 @@ 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. @@ -413,7 +423,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 5ac84b3494a3e3..7cb5062045baf2 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,8 +236,7 @@ public void buildTargets(BuildRequest request, BuildResult result, TargetValidat env.setWorkspaceName(targetPatternPhaseValue.getWorkspaceName()); ProjectEvaluationResult projectEvaluationResult = - evaluateProjectFile( - request, buildOptions, request.getUserOptions(), targetPatternPhaseValue, env); + evaluateProjectFile(request, buildOptions, targetPatternPhaseValue, env); var analysisCachingDeps = RemoteAnalysisCachingDependenciesProviderImpl.forAnalysis( @@ -1075,7 +1074,6 @@ 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, @@ -1086,7 +1084,6 @@ 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 f541dbdc0729d3..da79fb421cd145 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,11 +3254,6 @@ 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 b40d4b5687d4be..5b95668fbbed8c 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,16 +13,10 @@ // 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; @@ -90,9 +84,6 @@ 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; @@ -128,9 +119,18 @@ 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 { - defaultConfigValue = validateDefaultConfig(defaultConfigName, configs, supportedConfigs); + 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; } catch (InvalidProjectFileException e) { // there's no default config set. throw new FlagSetFunctionException( @@ -140,15 +140,6 @@ 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( @@ -159,11 +150,6 @@ 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); } @@ -190,42 +176,6 @@ 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 425163bf53f60d..cf58fb9ad1fc62 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,7 +16,6 @@ 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; @@ -41,7 +40,6 @@ 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; @@ -49,23 +47,16 @@ 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, - ImmutableSet userOptions, - boolean enforceCanonical) { - return interner.intern( - new Key(projectFile, sclConfig, targetOptions, userOptions, enforceCanonical)); + Label projectFile, String sclConfig, BuildOptions targetOptions, boolean enforceCanonical) { + return interner.intern(new Key(projectFile, sclConfig, targetOptions, enforceCanonical)); } public Label getProjectFile() { @@ -80,10 +71,6 @@ 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}. @@ -114,13 +101,12 @@ 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, userOptions, enforceCanonical); + return Objects.hash(projectFile, sclConfig, targetOptions, 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 3ab6951076700a..97a9109d0182fe 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"}) -public final class ImmutableSetCodec extends DeferredObjectCodec { +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 8d5ae92d46f910..71f4e4171dec17 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,7 +15,6 @@ 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; @@ -115,9 +114,4 @@ 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 deleted file mode 100644 index 7d8da990f889f4..00000000000000 --- a/src/main/java/com/google/devtools/common/options/GlobalRcUtils.java +++ /dev/null @@ -1,34 +0,0 @@ -// 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 8678e1070a2c83..0fa39eed4b51ec 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParser.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java @@ -160,7 +160,6 @@ 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; @@ -233,13 +232,6 @@ 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) { @@ -264,7 +256,7 @@ public Builder withConversionContext(Object conversionContext) { /** Returns a new {@link OptionsParser}. */ public OptionsParser build() { - return new OptionsParser(implBuilder.build(), allowResidue, ignoreUserOptions); + return new OptionsParser(implBuilder.build(), allowResidue); } } @@ -281,16 +273,13 @@ 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, boolean ignoreUserOptions) { + private OptionsParser(OptionsParserImpl impl, boolean allowResidue) { this.impl = impl; this.allowResidue = allowResidue; - this.ignoreUserOptions = ignoreUserOptions; } public Object getConversionContext() { @@ -875,33 +864,6 @@ 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 2974e7444f64d4..b45f88798b3185 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsProvider.java +++ b/src/main/java/com/google/devtools/common/options/OptionsProvider.java @@ -14,14 +14,13 @@ 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 = @@ -42,22 +41,16 @@ 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. @@ -75,10 +68,4 @@ public ImmutableSet getUserOptions() { * 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 a6d23d39ec9cf6..c25f6b91032eca 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,10 +310,7 @@ private OptionsParser createOptionsParser(Command commandAnnotation) { Iterables.addAll(options, module.getCommandOptions(commandAnnotation)); } options.addAll(runtime.getRuleClassProvider().getFragmentRegistry().getOptionsClasses()); - // 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(); + return OptionsParser.builder().optionsClasses(options).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 a61a2e32274844..293d27151954aa 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,7 +17,6 @@ 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; @@ -92,7 +91,6 @@ 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); @@ -123,7 +121,6 @@ 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); @@ -142,7 +139,6 @@ public void flagSetsFunction_returns_original_buildOptions() throws Exception { Label.parseCanonical("//test:PROJECT.scl"), "", buildOptions, - /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ false); FlagSetValue flagSetsValue = executeFunction(key); @@ -163,7 +159,6 @@ public void noEnforceCanonicalConfigs_unknownConfigisNoop() throws Exception { Label.parseCanonical("//test:PROJECT.scl"), "random_config_name", buildOptions, - /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ false); FlagSetValue flagSetsValue = executeFunction(key); @@ -196,7 +191,6 @@ public void enforceCanonicalConfigsSupportedConfig() throws Exception { Label.parseCanonical("//test:PROJECT.scl"), "test_config", buildOptions, - /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); FlagSetValue flagSetsValue = executeFunction(key); @@ -208,140 +202,6 @@ 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"); @@ -366,7 +226,6 @@ 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)); @@ -399,7 +258,6 @@ 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)); @@ -432,7 +290,6 @@ public void enforceCanonicalConfigsNoSclConfigFlagNoDefaultConfig() throws Excep Label.parseCanonical("//test:PROJECT.scl"), /* sclConfig= */ "", buildOptions, - /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); var thrown = assertThrows(Exception.class, () -> executeFunction(key)); @@ -467,7 +324,6 @@ public void enforceCanonicalConfigsNoSclConfigFlagUnsupportedDefaultConfig() thr Label.parseCanonical("//test:PROJECT.scl"), /* sclConfig= */ "", buildOptions, - /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); var thrown = assertThrows(Exception.class, () -> executeFunction(key)); @@ -503,7 +359,6 @@ public void enforceCanonicalConfigsNoSclConfigFlagNonexistentDefaultConfig() thr Label.parseCanonical("//test:PROJECT.scl"), /* sclConfig= */ "", buildOptions, - /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); var thrown = assertThrows(Exception.class, () -> executeFunction(key)); @@ -539,7 +394,6 @@ public void enforceCanonicalConfigsNoSclConfigFlagValidDefaultConfig() throws Ex Label.parseCanonical("//test:PROJECT.scl"), /* sclConfig= */ "", buildOptions, - /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); FlagSetValue flagSetsValue = executeFunction(key); @@ -574,7 +428,6 @@ public void enforceCanonicalConfigsNoSupportedConfigsWithNoSclConfig() throws Ex Label.parseCanonical("//test:PROJECT.scl"), /* sclConfig= */ "", buildOptions, - /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); FlagSetValue flagSetsValue = executeFunction(key); @@ -602,7 +455,6 @@ public void enforceCanonicalConfigsNoSupportedConfigsWithSclConfig() throws Exce Label.parseCanonical("//test:PROJECT.scl"), /* sclConfig= */ "test_config", buildOptions, - /* userOptions= */ ImmutableSet.of(), /* enforceCanonical= */ true); FlagSetValue flagSetsValue = executeFunction(key); @@ -639,7 +491,6 @@ 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 a599712bcf89a3..846d2e7484e87b 100644 --- a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java +++ b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java @@ -2625,22 +2625,6 @@ 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 cd530af42bed26..4342824b8c4e6a 100644 --- a/src/test/shell/integration/BUILD +++ b/src/test/shell/integration/BUILD @@ -676,17 +676,6 @@ 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 deleted file mode 100755 index 4aebbc61847184..00000000000000 --- a/src/test/shell/integration/flagset_test.sh +++ /dev/null @@ -1,100 +0,0 @@ -#!/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