From d7371772a884a7e630eb158c0e2cb4f7f8c143db Mon Sep 17 00:00:00 2001 From: Cristian Hancila Date: Thu, 25 Mar 2021 07:53:47 -0700 Subject: [PATCH] Move --repo_env to common options 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 https://github.com/bazelbuild/bazel/issues/12689 Closes #13003. PiperOrigin-RevId: 365035772 --- .../lib/analysis/config/CoreOptions.java | 14 ------------ .../build/lib/runtime/CommandEnvironment.java | 22 +++++++++---------- .../lib/runtime/CommonCommandOptions.java | 14 ++++++++++++ .../skyframe/SequencedSkyframeExecutor.java | 2 ++ .../build/lib/skyframe/SkyframeExecutor.java | 13 ++--------- .../query2/testutil/SkyframeQueryHelper.java | 2 ++ ...ractCollectPackagesUnderDirectoryTest.java | 1 + .../shell/bazel/starlark_repository_test.sh | 4 ++-- 8 files changed, 34 insertions(+), 38 deletions(-) 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 index cb0d2a409f4ff3..c77d3db8295eaa 100644 --- 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> 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> 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 index d0fa01e817fb6e..ba4c6fd81cfb16 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 @@ -89,6 +89,7 @@ public class CommandEnvironment { private final Set visibleActionEnv = new TreeSet<>(); private final Set visibleTestEnv = new TreeSet<>(); private final Map repoEnv = new TreeMap<>(); + private final Map repoEnvFromOptions = new TreeMap<>(); private final TimestampGranularityMonitor timestampGranularityMonitor; private final Thread commandThread; private final Command command; @@ -239,17 +240,15 @@ public void exit(AbruptExitException exception) { } } - CoreOptions configOpts = options.getOptions(CoreOptions.class); - if (configOpts != null) { - for (Map.Entry 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 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()); } } } @@ -702,6 +701,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 index 675fdbd4ee60d5..0a19bfd97fc5d5 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java @@ -489,6 +489,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> repositoryEnvironment; + /** The option converter to check that the user can only specify legal profiler tasks. */ public static class ProfilerTaskConverter extends EnumConverter { 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 index 83d9c402ee43ad..0bdcd03e40aeb6 100644 --- 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 clientEnv, + Map 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 index 70bb0f43aaf725..ebd075d00ffab6 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java @@ -2643,11 +2643,12 @@ public WorkspaceInfoFromDiff sync( BuildLanguageOptions buildLanguageOptions, UUID commandId, Map clientEnv, + Map 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( @@ -2713,16 +2714,6 @@ public void setActionEnv(Map actionEnv) { PrecomputedValue.ACTION_ENV.set(injectable(), actionEnv); } - private void setRepoEnv(CoreOptions opt) { - LinkedHashMap repoEnv = new LinkedHashMap<>(); - if (opt != null) { - for (Map.Entry v : opt.repositoryEnvironment) { - repoEnv.put(v.getKey(), v.getValue()); - } - } - PrecomputedValue.REPO_ENV.set(injectable(), repoEnv); - } - public PathPackageLocator createPackageLocator( ExtendedEventHandler eventHandler, List 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 index 44831ffd235ba6..ba2bb7bfadb626 100644 --- 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.of(), + ImmutableMap.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 index 6dbdc76743937f..b13178aba42252 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/AbstractCollectPackagesUnderDirectoryTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/AbstractCollectPackagesUnderDirectoryTest.java @@ -309,6 +309,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 index e5ef96782993e4..084615a054424b 100755 --- 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 <