[8.6.0] Add --experimental_strict_repo_env option

src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java

@@ -96,6 +96,7 @@
 import com.google.devtools.build.lib.runtime.BlazeModule;
 import com.google.devtools.build.lib.runtime.BlazeRuntime;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
+import com.google.devtools.build.lib.runtime.CommonCommandOptions;
 import com.google.devtools.build.lib.runtime.InfoItem;
 import com.google.devtools.build.lib.runtime.ProcessWrapper;
 import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor;
@@ -151,6 +152,8 @@ public class BazelRepositoryModule extends BlazeModule {
   private final AtomicBoolean isFetch = new AtomicBoolean(false);
   private final StarlarkRepositoryFunction starlarkRepositoryFunction;
   private final RepositoryCache repositoryCache = new RepositoryCache();
+  private final MutableSupplier<Map<String, String>> repoEnvironmentSupplier =
+      new MutableSupplier<>();
   private final MutableSupplier<Map<String, String>> clientEnvironmentSupplier =
       new MutableSupplier<>();
   private ImmutableMap<RepositoryName, PathFragment> overrides = ImmutableMap.of();
@@ -251,12 +254,14 @@ public void workspaceInit(
             repositoryHandlers,
             starlarkRepositoryFunction,
             isFetch,
+            repoEnvironmentSupplier,
             clientEnvironmentSupplier,
             directories,
             BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER,
             repositoryCache.getRepoContentsCache());
     singleExtensionEvalFunction =
-        new SingleExtensionEvalFunction(directories, clientEnvironmentSupplier);
+        new SingleExtensionEvalFunction(
+            directories, repoEnvironmentSupplier, clientEnvironmentSupplier);
 
     if (builtinModules == null) {
       builtinModules = ModuleFileFunction.getBuiltinModules(directories.getEmbeddedBinariesRoot());
@@ -332,6 +337,12 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
     this.yankedVersionsFunction.setDownloadManager(downloadManager);
     this.vendorCommand.setDownloadManager(downloadManager);
 
+    CommonCommandOptions commandOptions = env.getOptions().getOptions(CommonCommandOptions.class);
+    if (commandOptions.useStrictRepoEnv) {
+      repoEnvironmentSupplier.set(env.getRepoEnvFromOptions());
+    } else {
+      repoEnvironmentSupplier.set(env.getRepoEnv());
+    }
     clientEnvironmentSupplier.set(env.getRepoEnv());
     PackageOptions pkgOptions = env.getOptions().getOptions(PackageOptions.class);
     isFetch.set(pkgOptions != null && pkgOptions.fetch);

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java

@@ -55,7 +55,8 @@ protected ModuleExtensionContext(
       Path workingDirectory,
       BlazeDirectories directories,
       Environment env,
-      Map<String, String> envVariables,
+      Map<String, String> repoEnvVariables,
+      Map<String, String> clientEnvVariables,
       DownloadManager downloadManager,
       double timeoutScaling,
       @Nullable ProcessWrapper processWrapper,
@@ -69,7 +70,8 @@ protected ModuleExtensionContext(
         workingDirectory,
         directories,
         env,
-        envVariables,
+        repoEnvVariables,
+        clientEnvVariables,
         downloadManager,
         timeoutScaling,
         processWrapper,

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegularRunnableExtension.java

@@ -73,6 +73,7 @@ final class RegularRunnableExtension implements RunnableExtension {
   private final ModuleExtension extension;
   private final ImmutableMap<String, Optional<String>> staticEnvVars;
   private final BlazeDirectories directories;
+  private final Supplier<Map<String, String>> repoEnvironmentSupplier;
   private final Supplier<Map<String, String>> clientEnvironmentSupplier;
   private final double timeoutScaling;
   @Nullable private final ProcessWrapper processWrapper;
@@ -84,6 +85,7 @@ final class RegularRunnableExtension implements RunnableExtension {
       ModuleExtension extension,
       ImmutableMap<String, Optional<String>> staticEnvVars,
       BlazeDirectories directories,
+      Supplier<Map<String, String>> repoEnvironmentSupplier,
       Supplier<Map<String, String>> clientEnvironmentSupplier,
       double timeoutScaling,
       @Nullable ProcessWrapper processWrapper,
@@ -93,6 +95,7 @@ final class RegularRunnableExtension implements RunnableExtension {
     this.extension = extension;
     this.staticEnvVars = staticEnvVars;
     this.directories = directories;
+    this.repoEnvironmentSupplier = repoEnvironmentSupplier;
     this.clientEnvironmentSupplier = clientEnvironmentSupplier;
     this.timeoutScaling = timeoutScaling;
     this.processWrapper = processWrapper;
@@ -141,6 +144,7 @@ static RegularRunnableExtension load(
       StarlarkSemantics starlarkSemantics,
       Environment env,
       BlazeDirectories directories,
+      Supplier<Map<String, String>> repoEnvironmentSupplier,
       Supplier<Map<String, String>> clientEnvironmentSupplier,
       double timeoutScaling,
       @Nullable ProcessWrapper processWrapper,
@@ -176,16 +180,17 @@ static RegularRunnableExtension load(
           SpellChecker.didYouMean(extensionId.extensionName(), exportedExtensions));
     }
 
-    ImmutableMap<String, Optional<String>> envVars =
+    ImmutableMap<String, Optional<String>> staticEnvVars =
         RepositoryFunction.getEnvVarValues(env, ImmutableSet.copyOf(extension.envVariables()));
-    if (envVars == null) {
+    if (staticEnvVars == null) {
       return null;
     }
     return new RegularRunnableExtension(
         bzlLoadValue,
         extension,
-        envVars,
+        staticEnvVars,
         directories,
+        repoEnvironmentSupplier,
         clientEnvironmentSupplier,
         timeoutScaling,
         processWrapper,
@@ -361,6 +366,7 @@ private ModuleExtensionContext createContext(
         workingDirectory,
         directories,
         env,
+        repoEnvironmentSupplier.get(),
         clientEnvironmentSupplier.get(),
         downloadManager,
         timeoutScaling,

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java

@@ -63,6 +63,7 @@
  */
 public class SingleExtensionEvalFunction implements SkyFunction {
   private final BlazeDirectories directories;
+  private final Supplier<Map<String, String>> repoEnvironmentSupplier;
   private final Supplier<Map<String, String>> clientEnvironmentSupplier;
 
   private double timeoutScaling = 1.0;
@@ -71,8 +72,11 @@ public class SingleExtensionEvalFunction implements SkyFunction {
   @Nullable private DownloadManager downloadManager = null;
 
   public SingleExtensionEvalFunction(
-      BlazeDirectories directories, Supplier<Map<String, String>> clientEnvironmentSupplier) {
+      BlazeDirectories directories,
+      Supplier<Map<String, String>> repoEnvironmentSupplier,
+      Supplier<Map<String, String>> clientEnvironmentSupplier) {
     this.directories = directories;
+    this.repoEnvironmentSupplier = repoEnvironmentSupplier;
     this.clientEnvironmentSupplier = clientEnvironmentSupplier;
   }
 
@@ -127,6 +131,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
                 starlarkSemantics,
                 env,
                 directories,
+                repoEnvironmentSupplier,
                 clientEnvironmentSupplier,
                 timeoutScaling,
                 processWrapper,

src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java

@@ -158,7 +158,8 @@ private interface AsyncTask extends SilentCloseable {
   protected final Path workingDirectory;
   protected final BlazeDirectories directories;
   protected final Environment env;
-  protected final ImmutableMap<String, String> envVariables;
+  protected final ImmutableMap<String, String> repoEnvVariables;
+  protected final ImmutableMap<String, String> clientEnvVariables;
   private final StarlarkOS osObject;
   protected final DownloadManager downloadManager;
   protected final double timeoutScaling;
@@ -179,7 +180,8 @@ protected StarlarkBaseExternalContext(
       Path workingDirectory,
       BlazeDirectories directories,
       Environment env,
-      Map<String, String> envVariables,
+      Map<String, String> repoEnvVariables,
+      Map<String, String> clientEnvVariables,
       DownloadManager downloadManager,
       double timeoutScaling,
       @Nullable ProcessWrapper processWrapper,
@@ -190,8 +192,9 @@ protected StarlarkBaseExternalContext(
     this.workingDirectory = workingDirectory;
     this.directories = directories;
     this.env = env;
-    this.envVariables = ImmutableMap.copyOf(envVariables);
-    this.osObject = new StarlarkOS(this.envVariables);
+    this.repoEnvVariables = ImmutableMap.copyOf(repoEnvVariables);
+    this.clientEnvVariables = ImmutableMap.copyOf(clientEnvVariables);
+    this.osObject = new StarlarkOS(this.repoEnvVariables);
     this.downloadManager = downloadManager;
     this.timeoutScaling = timeoutScaling;
     this.processWrapper = processWrapper;
@@ -823,7 +826,7 @@ public Object download(
               canonicalId,
               Optional.<String>empty(),
               outputPath.getPath(),
-              envVariables,
+              clientEnvVariables,
               identifyingStringForLogging,
               downloadPhaser,
               // The repo rule may modify the file after the download, so we cannot guarantee that
@@ -1063,7 +1066,7 @@ public StructImpl downloadAndExtract(
               canonicalId,
               Optional.of(type),
               downloadDirectory,
-              envVariables,
+              clientEnvVariables,
               identifyingStringForLogging,
               downloadPhaser,
               // The archive is not going to be modified and not accessible to the user, so its safe
@@ -1430,15 +1433,15 @@ private static <T> T nullIfNone(Object object, Class<T> type) {
   @Nullable
   public String getEnvironmentValue(String name, Object defaultValue)
       throws InterruptedException, NeedsSkyframeRestartException {
-    // Must look up via AEF, rather than solely copy from `this.envVariables`, in order to
+    // Must look up via AEF, rather than solely copy from `this.repoEnvVariables`, in order to
     // establish a SkyKey dependency relationship.
     if (env.getValue(ActionEnvironmentFunction.key(name)) == null) {
       throw new NeedsSkyframeRestartException();
     }
 
-    // However, to account for --repo_env we take the value from `this.envVariables`.
+    // However, to account for --repo_env we take the value from `this.repoEnvVariables`.
     // See https://github.com/bazelbuild/bazel/pull/20787#discussion_r1445571248 .
-    String envVarValue = envVariables.get(name);
+    String envVarValue = repoEnvVariables.get(name);
     accumulatedEnvKeys.add(name);
     return envVarValue != null ? envVarValue : nullIfNone(defaultValue, String.class);
   }
@@ -1908,17 +1911,17 @@ public StarlarkExecutionResult execute(
     validateExecuteArguments(arguments);
     int timeout = Starlark.toInt(timeoutI, "timeout");
 
-    Map<String, Object> forceEnvVariablesRaw =
+    Map<String, Object> forceRepoEnvVariablesRaw =
         Dict.cast(uncheckedEnvironment, String.class, Object.class, "environment");
-    Map<String, String> forceEnvVariables = new LinkedHashMap<>();
-    Set<String> removeEnvVariables = new LinkedHashSet<>();
-    for (Map.Entry<String, Object> entry : forceEnvVariablesRaw.entrySet()) {
+    Map<String, String> forceRepoEnvVariables = new LinkedHashMap<>();
+    Set<String> removeRepoEnvVariables = new LinkedHashSet<>();
+    for (Map.Entry<String, Object> entry : forceRepoEnvVariablesRaw.entrySet()) {
       String key = entry.getKey();
       Object value = entry.getValue();
       if (value == Starlark.NONE) {
-        removeEnvVariables.add(key);
+        removeRepoEnvVariables.add(key);
       } else if (value instanceof String s) {
-        forceEnvVariables.put(key, s);
+        forceRepoEnvVariables.put(key, s);
       } else {
         throw Starlark.errorf("environment values must be strings or None, got %s", value);
       }
@@ -1927,7 +1930,8 @@ public StarlarkExecutionResult execute(
     if (canExecuteRemote()) {
       // Remote execution only sees the explicitly set environment variables, so removing env vars
       // isn't necessary.
-      return executeRemote(arguments, timeout, forceEnvVariables, quiet, overrideWorkingDirectory);
+      return executeRemote(
+          arguments, timeout, forceRepoEnvVariables, quiet, overrideWorkingDirectory);
     }
 
     // Execute on the local/host machine
@@ -1946,8 +1950,8 @@ public StarlarkExecutionResult execute(
         WorkspaceRuleEvent.newExecuteEvent(
             args,
             timeout,
-            Maps.filterKeys(envVariables, k -> !removeEnvVariables.contains(k)),
-            forceEnvVariables,
+            Maps.filterKeys(repoEnvVariables, k -> !removeRepoEnvVariables.contains(k)),
+            forceRepoEnvVariables,
             workingDirectory.getPathString(),
             quiet,
             identifyingStringForLogging,
@@ -1979,8 +1983,8 @@ public StarlarkExecutionResult execute(
       return StarlarkExecutionResult.builder(osObject.getEnvironmentVariables())
           .addArguments(args)
           .setDirectory(workingDirectoryPath.getPathFile())
-          .addEnvironmentVariables(forceEnvVariables)
-          .removeEnvironmentVariables(removeEnvVariables)
+          .addEnvironmentVariables(forceRepoEnvVariables)
+          .removeEnvironmentVariables(removeRepoEnvVariables)
           .setTimeout(timeoutMillis)
           .setQuiet(quiet)
           .execute();
@@ -2244,7 +2248,7 @@ public StarlarkPath which(String program, StarlarkThread thread) throws EvalExce
 
   @Nullable
   private StarlarkPath findCommandOnPath(String program) throws IOException {
-    String pathEnvVariable = envVariables.get("PATH");
+    String pathEnvVariable = repoEnvVariables.get("PATH");
     if (pathEnvVariable == null) {
       return null;
     }

src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java

@@ -96,7 +96,8 @@ public class StarlarkRepositoryContext extends StarlarkBaseExternalContext {
       Path outputDirectory,
       IgnoredSubdirectories ignoredSubdirectories,
       Environment environment,
-      ImmutableMap<String, String> env,
+      ImmutableMap<String, String> repoEnvVariables,
+      ImmutableMap<String, String> clientEnvVariables,
       DownloadManager downloadManager,
       double timeoutScaling,
       @Nullable ProcessWrapper processWrapper,
@@ -109,7 +110,8 @@ public class StarlarkRepositoryContext extends StarlarkBaseExternalContext {
         outputDirectory,
         directories,
         environment,
-        env,
+        repoEnvVariables,
+        clientEnvVariables,
         downloadManager,
         timeoutScaling,
         processWrapper,

src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java

@@ -255,6 +255,7 @@ private FetchResult fetchInternal(
                 outputDirectory,
                 ignoredSubdirectories,
                 env,
+                ImmutableMap.copyOf(repoEnvironment),
                 ImmutableMap.copyOf(clientEnvironment),
                 downloadManager,
                 timeoutScaling,

src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java

@@ -108,20 +108,23 @@ public final class RepositoryDelegatorFunction implements SkyFunction {
   private final AtomicBoolean isFetch;
   private final BlazeDirectories directories;
   private final ExternalPackageHelper externalPackageHelper;
+  private final Supplier<Map<String, String>> repoEnvironmentSupplier;
   private final Supplier<Map<String, String>> clientEnvironmentSupplier;
   private final RepoContentsCache repoContentsCache;
 
   public RepositoryDelegatorFunction(
       ImmutableMap<String, RepositoryFunction> handlers,
       @Nullable RepositoryFunction starlarkHandler,
       AtomicBoolean isFetch,
+      Supplier<Map<String, String>> repoEnvironmentSupplier,
       Supplier<Map<String, String>> clientEnvironmentSupplier,
       BlazeDirectories directories,
       ExternalPackageHelper externalPackageHelper,
       RepoContentsCache repoContentsCache) {
     this.handlers = handlers;
     this.starlarkHandler = starlarkHandler;
     this.isFetch = isFetch;
+    this.repoEnvironmentSupplier = repoEnvironmentSupplier;
     this.clientEnvironmentSupplier = clientEnvironmentSupplier;
     this.directories = directories;
     this.externalPackageHelper = externalPackageHelper;
@@ -441,6 +444,7 @@ private RepositoryFunction getHandler(Rule rule) {
       handler = handlers.get(rule.getRuleClass());
     }
     if (handler != null) {
+      handler.setRepoEnvironment(repoEnvironmentSupplier.get());
       handler.setClientEnvironment(clientEnvironmentSupplier.get());
     }
 

src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java

@@ -85,6 +85,7 @@
  */
 public abstract class RepositoryFunction {
 
+  protected Map<String, String> repoEnvironment;
   protected Map<String, String> clientEnvironment;
 
   /**
@@ -420,7 +421,12 @@ public static void addExternalFilesDependencies(
     env.getValue(RepositoryDirectoryValue.key(repositoryName));
   }
 
-  /** Sets up a mapping of environment variables to use. */
+  /** Sets up a mapping of environment variables to use for repository operations. */
+  public void setRepoEnvironment(Map<String, String> repoEnvironment) {
+    this.repoEnvironment = repoEnvironment;
+  }
+
+  /** Sets up a mapping of environment variables to use for client operations (e.g. auth). */
   public void setClientEnvironment(Map<String, String> clientEnvironment) {
     this.clientEnvironment = clientEnvironment;
   }