When --enforce_project_configs is enabled for a valid --scl_config, d…

…o not allow any other flags to be set in the user blazerc (~/.blazerc or --blazerc) or command line. PiperOrigin-RevId: 683295471 Change-Id: Ia707c2934ea34546fed3230acef62b504b24c8a2
katre · Oct 7, 2024 · 2127e45 · 2127e45
1 parent 1c4e78a
commit 2127e45
Show file tree

Hide file tree

Showing 18 changed files with 509 additions and 70 deletions.
diff --git a/src/main/cpp/option_processor.cc b/src/main/cpp/option_processor.cc
@@ -657,7 +657,9 @@ 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,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<Label, Label> findProjectFiles(
   public static FlagSetValue modifyBuildOptionsWithFlagSets(
       Label projectFile,
       BuildOptions targetOptions,
+      ImmutableSet<String> 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<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,6 +202,7 @@ enum ProjectFileFeature {
   static ProjectEvaluationResult evaluateProjectFile(
       BuildRequest request,
       BuildOptions buildOptions,
+      ImmutableSet<String> 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
@@ -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<String> 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<String, Object> 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<String> getUserOptions() {
+    return userOptions;
+  }
+
+  /** Returns a unique identifier that universally identifies this build. */
   public UUID getId() {
     return id;
   }
 
-  /**
-   * Returns the name of the Blaze command that the user invoked.
-   */
+  /** Returns the name of the Blaze command that the user invoked. */
   public String getCommandName() {
     return commandName;
   }
@@ -295,24 +300,19 @@ 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,10 +324,7 @@ 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);
   }
@@ -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<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.
@@ -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<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,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<String> 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
@@ -3254,6 +3254,11 @@ 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,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<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 {
-        ImmutableList<String> defaultConfigValue =
-            validateDefaultConfig(defaultConfigName, configs, supportedConfigs);
-        if (!defaultConfigWarningShown.get()) {
-          eventHandler.handle(
-              Event.info(
-                  String.format(
-                      "Applying flags from the default config defined in %s: %s ",
-                      projectFile, defaultConfigValue)));
-          defaultConfigWarningShown.set(true);
-        }
-        return defaultConfigValue;
+        defaultConfigValue = validateDefaultConfig(defaultConfigName, configs, supportedConfigs);
       } catch (InvalidProjectFileException e) {
         // there's no default config set.
         throw new FlagSetFunctionException(
@@ -140,6 +140,15 @@ 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(
@@ -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<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) {