Skip to content

Commit

Permalink
Move --repo_env to common options
Browse files Browse the repository at this point in the history
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
  • Loading branch information
chancila authored and philwo committed Apr 21, 2021
1 parent 4ecf049 commit d737177
Show file tree
Hide file tree
Showing 8 changed files with 34 additions and 38 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,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;
Expand Down Expand Up @@ -239,17 +240,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());
}
}
}
Expand Down Expand Up @@ -702,6 +701,7 @@ public void syncPackageLoading(OptionsProvider options)
options.getOptions(BuildLanguageOptions.class),
getCommandId(),
clientEnv,
repoEnvFromOptions,
timestampGranularityMonitor,
options);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -261,6 +262,7 @@ public WorkspaceInfoFromDiff sync(
buildLanguageOptions,
commandId,
clientEnv,
repoEnvOption,
tsgm,
options);
long startTime = System.nanoTime();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2643,11 +2643,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(
Expand Down Expand Up @@ -2713,16 +2714,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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
4 changes: 2 additions & 2 deletions src/test/shell/bazel/starlark_repository_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit d737177

Please sign in to comment.