Skylark repositories: propage value from --action_env to repository_ctx.environ

The environment is now computed with a mixture of the client environment and
the values specified by the --action_env flag. If a user want to overwrite its
environment for skylark repository, they can do `--action_env FOO=BAR` and
the repository will see FOO as having the value BAR, whichever value is
set in the client environment.

Also propagate it to all repository functions, and deduplicate the way
the client environment is passed to repository functions.

Design doc: https://bazel.build/designs/2016/10/18/repository-invalidation.html [step 1]

RELNOTES[INC]: repository_ctx environment is now affected by --action_env flag (value from the
client environment will be replaced by value given on the command line through --action_env).

--
Change-Id: I131a9695439aa9949d5001f820e2ae450e41332f
Reviewed-on: https://cr.bazel.build/7971
PiperOrigin-RevId: 144091492
MOS_MIGRATED_REVID=144091492

4 files changed, 22 insertions, 24 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java
index e946287dac..a8d72cc0ea 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java
@@ -71,7 +71,6 @@ import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 import com.google.devtools.common.options.OptionsBase;
 import com.google.devtools.common.options.OptionsProvider;
-
 import java.util.Map.Entry;
 import java.util.concurrent.atomic.AtomicBoolean;
 import javax.annotation.Nullable;
@@ -190,8 +189,7 @@ public class BazelRepositoryModule extends BlazeModule {
 
   @Override
   public void beforeCommand(Command command, CommandEnvironment env) throws AbruptExitException {
-    delegator.setClientEnvironment(env.getClientEnv());
-    skylarkRepositoryFunction.setCommandEnvironment(env);
+    delegator.setClientEnvironment(env.getActionClientEnv());
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java
index 3fd7c2009e..b3fea3db34 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java
@@ -23,7 +23,6 @@ import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue;
 import com.google.devtools.build.lib.rules.repository.RepositoryFunction;
-import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import com.google.devtools.build.lib.skyframe.FileValue;
 import com.google.devtools.build.lib.syntax.BaseFunction;
 import com.google.devtools.build.lib.syntax.EvalException;
@@ -34,10 +33,7 @@ import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.skyframe.SkyFunction.Environment;
 import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
 import com.google.devtools.build.skyframe.SkyValue;
-
 import java.io.IOException;
-import java.util.Map;
-
 import javax.annotation.Nullable;
 
 /**
@@ -71,18 +67,6 @@ public class SkylarkRepositoryFunction extends RepositoryFunction {
     return new SkylarkRepositoryMissingDependencyException();
   }
 
-  private CommandEnvironment commandEnvironment = null;
-
-  public void setCommandEnvironment(CommandEnvironment commandEnvironment) {
-    this.commandEnvironment = commandEnvironment;
-  }
-
-  private Map<String, String> getClientEnvironment() {
-    return commandEnvironment != null
-        ? commandEnvironment.getClientEnv()
-        : ImmutableMap.<String, String>of();
-  }
-
   @Nullable
   @Override
   public SkyValue fetch(
@@ -95,8 +79,9 @@ public class SkylarkRepositoryFunction extends RepositoryFunction {
               .setGlobals(rule.getRuleClassObject().getRuleDefinitionEnvironment().getGlobals())
               .setEventHandler(env.getListener())
               .build();
-      SkylarkRepositoryContext skylarkRepositoryContext = new SkylarkRepositoryContext(
-          rule, outputDirectory, env, getClientEnvironment(), httpDownloader);
+      SkylarkRepositoryContext skylarkRepositoryContext =
+          new SkylarkRepositoryContext(
+              rule, outputDirectory, env, clientEnvironment, httpDownloader);
 
       // This has side-effect, we don't care about the output.
       // Also we do a lot of stuff in there, maybe blocking operations and we should certainly make
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java
index f9ac1c1afe..ba7ceda876 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java
@@ -33,12 +33,10 @@ import com.google.devtools.build.skyframe.SkyFunctionException;
 import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
-
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicBoolean;
-
 import javax.annotation.Nullable;
 
 /**
@@ -111,6 +109,8 @@ public final class RepositoryDelegatorFunction implements SkyFunction {
           "Could not find handler for " + rule), Transience.PERSISTENT);
     }
 
+    handler.setClientEnvironment(clientEnvironment);
+
     Path repoRoot =
         RepositoryFunction.getExternalRepositoryDirectory(directories).getRelative(rule.getName());
     byte[] ruleSpecificData = handler.getRuleSpecificMarkerData(rule, env);
@@ -120,7 +120,6 @@ public final class RepositoryDelegatorFunction implements SkyFunction {
     byte[] ruleKey = computeRuleKey(rule, ruleSpecificData);
     Path markerPath = getMarkerPath(directories, rule);
 
-    handler.setClientEnvironment(clientEnvironment);
     if (handler.isLocal(rule)) {
       // Local repositories are always fetched because the operation is generally fast and they do
       // not depend on non-local data, so it does not make much sense to try to cache from across
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
index 264fa22195..cc473da5e6 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
@@ -29,6 +29,7 @@ import com.google.devtools.build.lib.analysis.SkyframePackageRootResolver;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.analysis.config.DefaultsPackage;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.Reporter;
@@ -82,6 +83,7 @@ public final class CommandEnvironment {
   private final BlazeModule.ModuleEnvironment blazeModuleEnvironment;
   private final Map<String, String> clientEnv = new TreeMap<>();
   private final Set<String> visibleClientEnv = new TreeSet<>();
+  private final Map<String, String> actionClientEnv = new TreeMap<>();
   private final TimestampGranularityMonitor timestampGranularityMonitor;
   private final Thread commandThread;
 
@@ -592,6 +594,11 @@ public final class CommandEnvironment {
     // Start the performance and memory profilers.
     runtime.beforeCommand(this, options, execStartTimeNanos);
 
+    // actionClientEnv contains the environment where values from actionEnvironment are
+    // overridden.
+    actionClientEnv.clear();
+    actionClientEnv.putAll(clientEnv);
+
     if (command.builds()) {
       Map<String, String> testEnv = new TreeMap<>();
       for (Map.Entry<String, String> entry :
@@ -607,6 +614,7 @@ public final class CommandEnvironment {
           visibleClientEnv.add(entry.getKey());
         } else {
           visibleClientEnv.remove(entry.getKey());
+          actionClientEnv.put(entry.getKey(), entry.getValue());
         }
       }
 
@@ -644,4 +652,12 @@ public final class CommandEnvironment {
     }
     return workspace.getOutputBaseFilesystemTypeName();
   }
+
+  /**
+   * Returns the client environment for which value specified in the command line with the flag
+   * --action_env have been enforced.
+   */
+  public Map<String, String> getActionClientEnv() {
+    return Collections.unmodifiableMap(actionClientEnv);
+  }
 }