Move --repo_env to common options

chancila · copybara-github · commit c2bdd034014f · 2021-03-25T07:54:41.000-07:00
Move --repo_env into common options and inject the value through CommandEnvironment since CommonCommandOptions cannot be accessed in SkyframeExecutor directly due to a circular dependency in the build. Addresses #12689 Closes #13003. PiperOrigin-RevId: 365035772
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
@@ -366,20 +366,6 @@ public String getTypeDescription() {
               + " wins, options for different variables accumulate.")
   public List<Map.Entry<String, String>> hostActionEnvironment;
 
-  @Option(
-      name = "repo_env",
-      converter = Converters.OptionalAssignmentConverter.class,
-      allowMultiple = true,
-      defaultValue = "null",
-      documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
-      effectTags = {OptionEffectTag.ACTION_COMMAND_LINES},
-      help =
-          "Specifies additional environment variables to be available only for repository rules."
-              + " Note that repository rules see the full environment anyway, but in this way"
-              + " configuration information can be passed to repositories through options without"
-              + " invalidating the action graph.")
-  public List<Map.Entry<String, String>> repositoryEnvironment;
-
   @Option(
       name = "collect_code_coverage",
       defaultValue = "false",
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
@@ -91,6 +91,7 @@ public class CommandEnvironment {
   private final Set<String> visibleActionEnv = new TreeSet<>();
   private final Set<String> visibleTestEnv = new TreeSet<>();
   private final Map<String, String> repoEnv = new TreeMap<>();
+  private final Map<String, String> repoEnvFromOptions = new TreeMap<>();
   private final TimestampGranularityMonitor timestampGranularityMonitor;
   private final Thread commandThread;
   private final Command command;
@@ -245,17 +246,15 @@ public void exit(AbruptExitException exception) {
       }
     }
 
-    CoreOptions configOpts = options.getOptions(CoreOptions.class);
-    if (configOpts != null) {
-      for (Map.Entry<String, String> entry : configOpts.repositoryEnvironment) {
-        String name = entry.getKey();
-        String value = entry.getValue();
-        if (value == null) {
-          value = System.getenv(name);
-        }
-        if (value != null) {
-          repoEnv.put(name, value);
-        }
+    for (Map.Entry<String, String> entry : commandOptions.repositoryEnvironment) {
+      String name = entry.getKey();
+      String value = entry.getValue();
+      if (value == null) {
+        value = System.getenv(name);
+      }
+      if (value != null) {
+        repoEnv.put(entry.getKey(), entry.getValue());
+        repoEnvFromOptions.put(entry.getKey(), entry.getValue());
       }
     }
   }
@@ -709,6 +708,7 @@ public void syncPackageLoading(OptionsProvider options)
                 options.getOptions(BuildLanguageOptions.class),
                 getCommandId(),
                 clientEnv,
+                repoEnvFromOptions,
                 timestampGranularityMonitor,
                 options);
   }
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java
@@ -491,6 +491,20 @@ public String getTypeDescription() {
               + "one.")
   public boolean keepStateAfterBuild;
 
+  @Option(
+      name = "repo_env",
+      converter = Converters.OptionalAssignmentConverter.class,
+      allowMultiple = true,
+      defaultValue = "null",
+      documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
+      effectTags = {OptionEffectTag.ACTION_COMMAND_LINES},
+      help =
+          "Specifies additional environment variables to be available only for repository rules."
+              + " Note that repository rules see the full environment anyway, but in this way"
+              + " configuration information can be passed to repositories through options without"
+              + " invalidating the action graph.")
+  public List<Map.Entry<String, String>> repositoryEnvironment;
+
   /** The option converter to check that the user can only specify legal profiler tasks. */
   public static class ProfilerTaskConverter extends EnumConverter<ProfilerTask> {
     public ProfilerTaskConverter() {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
@@ -236,6 +236,7 @@ public WorkspaceInfoFromDiff sync(
       BuildLanguageOptions buildLanguageOptions,
       UUID commandId,
       Map<String, String> clientEnv,
+      Map<String, String> repoEnvOption,
       TimestampGranularityMonitor tsgm,
       OptionsProvider options)
       throws InterruptedException, AbruptExitException {
@@ -261,6 +262,7 @@ public WorkspaceInfoFromDiff sync(
         buildLanguageOptions,
         commandId,
         clientEnv,
+        repoEnvOption,
         tsgm,
         options);
     long startTime = System.nanoTime();
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
@@ -2695,11 +2695,12 @@ public WorkspaceInfoFromDiff sync(
       BuildLanguageOptions buildLanguageOptions,
       UUID commandId,
       Map<String, String> clientEnv,
+      Map<String, String> repoEnvOption,
       TimestampGranularityMonitor tsgm,
       OptionsProvider options)
       throws InterruptedException, AbruptExitException {
     getActionEnvFromOptions(options.getOptions(CoreOptions.class));
-    setRepoEnv(options.getOptions(CoreOptions.class));
+    PrecomputedValue.REPO_ENV.set(injectable(), new LinkedHashMap<>(repoEnvOption));
     RemoteOptions remoteOptions = options.getOptions(RemoteOptions.class);
     setRemoteExecutionEnabled(remoteOptions != null && remoteOptions.isRemoteExecutionEnabled());
     syncPackageLoading(
@@ -2785,16 +2786,6 @@ public void setActionEnv(Map<String, String> actionEnv) {
     PrecomputedValue.ACTION_ENV.set(injectable(), actionEnv);
   }
 
-  private void setRepoEnv(CoreOptions opt) {
-    LinkedHashMap<String, String> repoEnv = new LinkedHashMap<>();
-    if (opt != null) {
-      for (Map.Entry<String, String> v : opt.repositoryEnvironment) {
-        repoEnv.put(v.getKey(), v.getValue());
-      }
-    }
-    PrecomputedValue.REPO_ENV.set(injectable(), repoEnv);
-  }
-
   public PathPackageLocator createPackageLocator(
       ExtendedEventHandler eventHandler, List<String> packagePaths, Path workingDirectory) {
     return PathPackageLocator.create(
diff --git a/src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java b/src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java
@@ -281,6 +281,7 @@ protected void initTargetPatternEvaluator(ConfiguredRuleClassProvider ruleClassP
     this.toolsRepository = ruleClassProvider.getToolsRepository();
     skyframeExecutor = createSkyframeExecutor(ruleClassProvider);
     PackageOptions packageOptions = Options.getDefaults(PackageOptions.class);
+
     packageOptions.defaultVisibility = ConstantRuleVisibility.PRIVATE;
     packageOptions.showLoadingProgress = true;
     packageOptions.globbingThreads = 7;
@@ -296,6 +297,7 @@ protected void initTargetPatternEvaluator(ConfiguredRuleClassProvider ruleClassP
           Options.getDefaults(BuildLanguageOptions.class),
           UUID.randomUUID(),
           ImmutableMap.<String, String>of(),
+          ImmutableMap.<String, String>of(),
           new TimestampGranularityMonitor(BlazeClock.instance()),
           OptionsProvider.EMPTY);
     } catch (InterruptedException | AbruptExitException e) {
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/AbstractCollectPackagesUnderDirectoryTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/AbstractCollectPackagesUnderDirectoryTest.java
@@ -310,6 +310,7 @@ private void initBuildDriver() throws AbruptExitException, InterruptedException,
         Options.getDefaults(BuildLanguageOptions.class),
         UUID.randomUUID(),
         /*clientEnv=*/ ImmutableMap.of(),
+        /*repoEnvOption=*/ ImmutableMap.of(),
         new TimestampGranularityMonitor(BlazeClock.instance()),
         OptionsProvider.EMPTY);
     buildDriver = skyframeExecutor.getDriver();
diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh
@@ -887,9 +887,9 @@ genrule(
 )
 EOF
   cat > .bazelrc <<EOF
-build:foo --repo_env=FOO=foo
+common:foo --repo_env=FOO=foo
 build:bar --repo_env=FOO=bar
-build:qux --repo_env=FOO
+common:qux --repo_env=FOO
 EOF
 
   bazel build --config=foo //:repoenv //:unrelated
