[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 a034aa6 Co-authored-by: Googler <[email protected]>
bazelbuild · Oct 8, 2024 · a4d900d · a4d900d
1 parent a05fac3
commit a4d900d
Show file tree

Hide file tree

Showing 18 changed files with 70 additions and 509 deletions.
diff --git a/src/main/cpp/option_processor.cc b/src/main/cpp/option_processor.cc
@@ -657,9 +657,7 @@ std::vector<std::string> OptionProcessor::GetBlazercAndEnvCommandArgs(
     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
@@ -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<Label, Label> findProjectFiles(
   public static FlagSetValue modifyBuildOptionsWithFlagSets(
       Label projectFile,
       BuildOptions targetOptions,
-      ImmutableSet<String> 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<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
@@ -202,7 +202,6 @@ enum ProjectFileFeature {
   static ProjectEvaluationResult evaluateProjectFile(
       BuildRequest request,
       BuildOptions buildOptions,
-      ImmutableSet<String> 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
@@ -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<String> 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<String, Object> 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<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;
   }
@@ -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<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,7 +324,10 @@ public <T extends OptionsBase> T getOptions(Class<T> 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<String> 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<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
@@ -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<String> 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
@@ -3254,11 +3254,6 @@ public ImmutableMap<String, Object> getExplicitStarlarkOptions(
               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
@@ -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<String> getSclConfig(
       // is silently consider a no-op.
       return sclConfigValue == null ? ImmutableList.of() : ImmutableList.copyOf(sclConfigValue);
     } else if (sclConfigName.isEmpty()) {
-      ImmutableList<String> defaultConfigValue = ImmutableList.of();
       try {
-        defaultConfigValue = validateDefaultConfig(defaultConfigName, configs, supportedConfigs);
+        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;
       } catch (InvalidProjectFileException e) {
         // there's no default config set.
         throw new FlagSetFunctionException(
@@ -140,15 +140,6 @@ private ImmutableList<String> 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<String> 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<String> userOptions)
-      throws FlagSetFunctionException {
-    ImmutableList.Builder<String> 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<String> allOptionsAsStrings = allOptionsAsStringsBuilder.build();
-    ImmutableSet<String> 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<String, String> supportedConfigs) {