Skip to content

Commit 60bc017

Browse files
Silic0nS0ldiercopybara-github
Silic0nS0ldier
authored and
copybara-github
committed
Add --experimental_strict_repo_env option
This PR introduces a new flag `--experimental_strict_repo_env` which stops repository rules and module extensions from inheriting the client environment (making `--repo_env=NAME` more than just an advisory notice). When enabled up to 2 environment variables will still be forwarded (unless overridden or explicitly removed via `--repo_env==VARNAME`, see #26300 for more details); - `PATH` - All platforms - `PATHEXT` - Windows See `test_execute_environment_strict_vars` in `src/test/shell/bazel/starlark_repository_test.sh` for a demonstration. Note that the behavior is different to the similarly named `--incompatible_strict_action_env`, which stops _all_ environment variables (`--action_env` affects actions with `use_default_shell_env = True`) except those specified within the defining rule. This is by design as repository rules operate in an inherently non-hermetic domain, covering roles such as integrating with the C/C++ toolchain installed on the host. It does not make sense to lock down environment variables _by default_, this is best left up to individual projects and users. This flag is marked experimental to allow for testing and requirement discovery (e.g. env vars other than `PATH` that should be included). Closes #10996 Closes #24404. PiperOrigin-RevId: 836494750 Change-Id: Ic05e5ca47a14badb2cd23f810e775c3341ddfaa8
1 parent 08bc4d7 commit 60bc017

20 files changed

Lines changed: 195 additions & 52 deletions

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

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@
8484
import com.google.devtools.build.lib.runtime.BlazeModule;
8585
import com.google.devtools.build.lib.runtime.BlazeRuntime;
8686
import com.google.devtools.build.lib.runtime.CommandEnvironment;
87+
import com.google.devtools.build.lib.runtime.CommonCommandOptions;
8788
import com.google.devtools.build.lib.runtime.InfoItem;
8889
import com.google.devtools.build.lib.runtime.ProcessWrapper;
8990
import com.google.devtools.build.lib.runtime.RemoteRepoContentsCache;
@@ -132,6 +133,8 @@ public class BazelRepositoryModule extends BlazeModule {
132133
ImmutableMap.of();
133134

134135
private final RepositoryCache repositoryCache = new RepositoryCache();
136+
private final MutableSupplier<Map<String, String>> repoEnvironmentSupplier =
137+
new MutableSupplier<>();
135138
private final MutableSupplier<Map<String, String>> clientEnvironmentSupplier =
136139
new MutableSupplier<>();
137140
private boolean fetchDisabled = false;
@@ -213,9 +216,13 @@ public void workspaceInit(
213216

214217
repositoryFetchFunction =
215218
new RepositoryFetchFunction(
216-
clientEnvironmentSupplier, directories, repositoryCache.getRepoContentsCache());
219+
repoEnvironmentSupplier,
220+
clientEnvironmentSupplier,
221+
directories,
222+
repositoryCache.getRepoContentsCache());
217223
singleExtensionEvalFunction =
218-
new SingleExtensionEvalFunction(directories, clientEnvironmentSupplier);
224+
new SingleExtensionEvalFunction(
225+
directories, repoEnvironmentSupplier, clientEnvironmentSupplier);
219226

220227
if (builtinModules == null) {
221228
builtinModules = ModuleFileFunction.getBuiltinModules();
@@ -278,6 +285,12 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
278285
this.yankedVersionsFunction.setDownloadManager(downloadManager);
279286
this.vendorCommand.setDownloadManager(downloadManager);
280287

288+
CommonCommandOptions commandOptions = env.getOptions().getOptions(CommonCommandOptions.class);
289+
if (commandOptions.useStrictRepoEnv) {
290+
repoEnvironmentSupplier.set(env.getRepoEnvFromOptions());
291+
} else {
292+
repoEnvironmentSupplier.set(env.getRepoEnv());
293+
}
281294
clientEnvironmentSupplier.set(env.getRepoEnv());
282295
PackageOptions pkgOptions = env.getOptions().getOptions(PackageOptions.class);
283296
fetchDisabled = pkgOptions != null && !pkgOptions.fetch;

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,8 @@ protected ModuleExtensionContext(
5555
Path workingDirectory,
5656
BlazeDirectories directories,
5757
Environment env,
58-
Map<String, String> envVariables,
58+
Map<String, String> repoEnvVariables,
59+
Map<String, String> clientEnvVariables,
5960
DownloadManager downloadManager,
6061
double timeoutScaling,
6162
@Nullable ProcessWrapper processWrapper,
@@ -69,7 +70,8 @@ protected ModuleExtensionContext(
6970
workingDirectory,
7071
directories,
7172
env,
72-
envVariables,
73+
repoEnvVariables,
74+
clientEnvVariables,
7375
downloadManager,
7476
timeoutScaling,
7577
processWrapper,

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

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ final class RegularRunnableExtension implements RunnableExtension {
7373
private final ModuleExtension extension;
7474
private final ImmutableMap<String, Optional<String>> staticEnvVars;
7575
private final BlazeDirectories directories;
76+
private final Supplier<Map<String, String>> repoEnvironmentSupplier;
7677
private final Supplier<Map<String, String>> clientEnvironmentSupplier;
7778
private final double timeoutScaling;
7879
@Nullable private final ProcessWrapper processWrapper;
@@ -84,6 +85,7 @@ final class RegularRunnableExtension implements RunnableExtension {
8485
ModuleExtension extension,
8586
ImmutableMap<String, Optional<String>> staticEnvVars,
8687
BlazeDirectories directories,
88+
Supplier<Map<String, String>> repoEnvironmentSupplier,
8789
Supplier<Map<String, String>> clientEnvironmentSupplier,
8890
double timeoutScaling,
8991
@Nullable ProcessWrapper processWrapper,
@@ -93,6 +95,7 @@ final class RegularRunnableExtension implements RunnableExtension {
9395
this.extension = extension;
9496
this.staticEnvVars = staticEnvVars;
9597
this.directories = directories;
98+
this.repoEnvironmentSupplier = repoEnvironmentSupplier;
9699
this.clientEnvironmentSupplier = clientEnvironmentSupplier;
97100
this.timeoutScaling = timeoutScaling;
98101
this.processWrapper = processWrapper;
@@ -141,6 +144,7 @@ static RegularRunnableExtension load(
141144
StarlarkSemantics starlarkSemantics,
142145
Environment env,
143146
BlazeDirectories directories,
147+
Supplier<Map<String, String>> repoEnvironmentSupplier,
144148
Supplier<Map<String, String>> clientEnvironmentSupplier,
145149
double timeoutScaling,
146150
@Nullable ProcessWrapper processWrapper,
@@ -176,16 +180,17 @@ static RegularRunnableExtension load(
176180
SpellChecker.didYouMean(extensionId.extensionName(), exportedExtensions));
177181
}
178182

179-
ImmutableMap<String, Optional<String>> envVars =
183+
ImmutableMap<String, Optional<String>> staticEnvVars =
180184
RepositoryUtils.getEnvVarValues(env, ImmutableSet.copyOf(extension.envVariables()));
181-
if (envVars == null) {
185+
if (staticEnvVars == null) {
182186
return null;
183187
}
184188
return new RegularRunnableExtension(
185189
bzlLoadValue,
186190
extension,
187-
envVars,
191+
staticEnvVars,
188192
directories,
193+
repoEnvironmentSupplier,
189194
clientEnvironmentSupplier,
190195
timeoutScaling,
191196
processWrapper,
@@ -364,6 +369,7 @@ private ModuleExtensionContext createContext(
364369
workingDirectory,
365370
directories,
366371
env,
372+
repoEnvironmentSupplier.get(),
367373
clientEnvironmentSupplier.get(),
368374
downloadManager,
369375
timeoutScaling,

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@
6363
*/
6464
public class SingleExtensionEvalFunction implements SkyFunction {
6565
private final BlazeDirectories directories;
66+
private final Supplier<Map<String, String>> repoEnvironmentSupplier;
6667
private final Supplier<Map<String, String>> clientEnvironmentSupplier;
6768

6869
private double timeoutScaling = 1.0;
@@ -71,8 +72,11 @@ public class SingleExtensionEvalFunction implements SkyFunction {
7172
@Nullable private DownloadManager downloadManager = null;
7273

7374
public SingleExtensionEvalFunction(
74-
BlazeDirectories directories, Supplier<Map<String, String>> clientEnvironmentSupplier) {
75+
BlazeDirectories directories,
76+
Supplier<Map<String, String>> repoEnvironmentSupplier,
77+
Supplier<Map<String, String>> clientEnvironmentSupplier) {
7578
this.directories = directories;
79+
this.repoEnvironmentSupplier = repoEnvironmentSupplier;
7680
this.clientEnvironmentSupplier = clientEnvironmentSupplier;
7781
}
7882

@@ -124,6 +128,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
124128
starlarkSemantics,
125129
env,
126130
directories,
131+
repoEnvironmentSupplier,
127132
clientEnvironmentSupplier,
128133
timeoutScaling,
129134
processWrapper,

‎src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryFetchFunction.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,7 @@ public final class RepositoryFetchFunction implements SkyFunction {
9393

9494
private final BlazeDirectories directories;
9595
private final LocalRepoContentsCache repoContentsCache;
96+
private final Supplier<Map<String, String>> repoEnvironmentSupplier;
9697
private final Supplier<Map<String, String>> clientEnvironmentSupplier;
9798

9899
private double timeoutScaling = 1.0;
@@ -103,9 +104,11 @@ public final class RepositoryFetchFunction implements SkyFunction {
103104
@Nullable private SyscallCache syscallCache;
104105

105106
public RepositoryFetchFunction(
107+
Supplier<Map<String, String>> repoEnvironmentSupplier,
106108
Supplier<Map<String, String>> clientEnvironmentSupplier,
107109
BlazeDirectories directories,
108110
LocalRepoContentsCache repoContentsCache) {
111+
this.repoEnvironmentSupplier = repoEnvironmentSupplier;
109112
this.clientEnvironmentSupplier = clientEnvironmentSupplier;
110113
this.directories = directories;
111114
this.repoContentsCache = repoContentsCache;
@@ -607,6 +610,7 @@ private FetchResult fetchInternal(
607610
outputDirectory,
608611
ignoredSubdirectories.asIgnoredSubdirectories(),
609612
env,
613+
ImmutableMap.copyOf(repoEnvironmentSupplier.get()),
610614
ImmutableMap.copyOf(clientEnvironmentSupplier.get()),
611615
downloadManager,
612616
timeoutScaling,

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

Lines changed: 25 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,8 @@ private interface AsyncTask extends SilentCloseable {
158158
protected final Path workingDirectory;
159159
protected final BlazeDirectories directories;
160160
protected final Environment env;
161-
protected final ImmutableMap<String, String> envVariables;
161+
protected final ImmutableMap<String, String> repoEnvVariables;
162+
protected final ImmutableMap<String, String> clientEnvVariables;
162163
private final StarlarkOS osObject;
163164
protected final DownloadManager downloadManager;
164165
protected final double timeoutScaling;
@@ -180,7 +181,8 @@ protected StarlarkBaseExternalContext(
180181
Path workingDirectory,
181182
BlazeDirectories directories,
182183
Environment env,
183-
Map<String, String> envVariables,
184+
Map<String, String> repoEnvVariables,
185+
Map<String, String> clientEnvVariables,
184186
DownloadManager downloadManager,
185187
double timeoutScaling,
186188
@Nullable ProcessWrapper processWrapper,
@@ -191,8 +193,9 @@ protected StarlarkBaseExternalContext(
191193
this.workingDirectory = workingDirectory;
192194
this.directories = directories;
193195
this.env = env;
194-
this.envVariables = ImmutableMap.copyOf(envVariables);
195-
this.osObject = new StarlarkOS(this.envVariables);
196+
this.repoEnvVariables = ImmutableMap.copyOf(repoEnvVariables);
197+
this.clientEnvVariables = ImmutableMap.copyOf(clientEnvVariables);
198+
this.osObject = new StarlarkOS(this.repoEnvVariables);
196199
this.downloadManager = downloadManager;
197200
this.timeoutScaling = timeoutScaling;
198201
this.processWrapper = processWrapper;
@@ -823,7 +826,7 @@ public Object download(
823826
canonicalId,
824827
Optional.<String>empty(),
825828
outputPath.getPath(),
826-
envVariables,
829+
clientEnvVariables,
827830
identifyingStringForLogging,
828831
downloadPhaser,
829832
// The repo rule may modify the file after the download, so we cannot guarantee that
@@ -1063,7 +1066,7 @@ public StructImpl downloadAndExtract(
10631066
canonicalId,
10641067
Optional.of(type),
10651068
downloadDirectory,
1066-
envVariables,
1069+
clientEnvVariables,
10671070
identifyingStringForLogging,
10681071
downloadPhaser,
10691072
// 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) {
14301433
@Nullable
14311434
public String getEnvironmentValue(String name, Object defaultValue)
14321435
throws InterruptedException, NeedsSkyframeRestartException {
1433-
// Must look up via AEF, rather than solely copy from `this.envVariables`, in order to
1436+
// Must look up via AEF, rather than solely copy from `this.repoEnvVariables`, in order to
14341437
// establish a SkyKey dependency relationship.
14351438
if (env.getValue(ActionEnvironmentFunction.key(name)) == null) {
14361439
throw new NeedsSkyframeRestartException();
14371440
}
14381441

1439-
// However, to account for --repo_env we take the value from `this.envVariables`.
1442+
// However, to account for --repo_env we take the value from `this.repoEnvVariables`.
14401443
// See https://github.com/bazelbuild/bazel/pull/20787#discussion_r1445571248 .
1441-
String envVarValue = envVariables.get(name);
1444+
String envVarValue = repoEnvVariables.get(name);
14421445
accumulatedEnvKeys.add(name);
14431446
return envVarValue != null ? envVarValue : nullIfNone(defaultValue, String.class);
14441447
}
@@ -1911,17 +1914,17 @@ public StarlarkExecutionResult execute(
19111914
validateExecuteArguments(arguments);
19121915
int timeout = Starlark.toInt(timeoutI, "timeout");
19131916

1914-
Map<String, Object> forceEnvVariablesRaw =
1917+
Map<String, Object> forceRepoEnvVariablesRaw =
19151918
Dict.cast(uncheckedEnvironment, String.class, Object.class, "environment");
1916-
Map<String, String> forceEnvVariables = new LinkedHashMap<>();
1917-
Set<String> removeEnvVariables = new LinkedHashSet<>();
1918-
for (Map.Entry<String, Object> entry : forceEnvVariablesRaw.entrySet()) {
1919+
Map<String, String> forceRepoEnvVariables = new LinkedHashMap<>();
1920+
Set<String> removeRepoEnvVariables = new LinkedHashSet<>();
1921+
for (Map.Entry<String, Object> entry : forceRepoEnvVariablesRaw.entrySet()) {
19191922
String key = entry.getKey();
19201923
Object value = entry.getValue();
19211924
if (value == Starlark.NONE) {
1922-
removeEnvVariables.add(key);
1925+
removeRepoEnvVariables.add(key);
19231926
} else if (value instanceof String s) {
1924-
forceEnvVariables.put(key, s);
1927+
forceRepoEnvVariables.put(key, s);
19251928
} else {
19261929
throw Starlark.errorf("environment values must be strings or None, got %s", value);
19271930
}
@@ -1930,7 +1933,8 @@ public StarlarkExecutionResult execute(
19301933
if (canExecuteRemote()) {
19311934
// Remote execution only sees the explicitly set environment variables, so removing env vars
19321935
// isn't necessary.
1933-
return executeRemote(arguments, timeout, forceEnvVariables, quiet, overrideWorkingDirectory);
1936+
return executeRemote(
1937+
arguments, timeout, forceRepoEnvVariables, quiet, overrideWorkingDirectory);
19341938
}
19351939

19361940
// Execute on the local/host machine
@@ -1949,8 +1953,8 @@ public StarlarkExecutionResult execute(
19491953
WorkspaceRuleEvent.newExecuteEvent(
19501954
args,
19511955
timeout,
1952-
Maps.filterKeys(envVariables, k -> !removeEnvVariables.contains(k)),
1953-
forceEnvVariables,
1956+
Maps.filterKeys(repoEnvVariables, k -> !removeRepoEnvVariables.contains(k)),
1957+
forceRepoEnvVariables,
19541958
workingDirectory.getPathString(),
19551959
quiet,
19561960
identifyingStringForLogging,
@@ -1982,8 +1986,8 @@ public StarlarkExecutionResult execute(
19821986
return StarlarkExecutionResult.builder(osObject.getEnvironmentVariables())
19831987
.addArguments(args)
19841988
.setDirectory(workingDirectoryPath.getPathFile())
1985-
.addEnvironmentVariables(forceEnvVariables)
1986-
.removeEnvironmentVariables(removeEnvVariables)
1989+
.addEnvironmentVariables(forceRepoEnvVariables)
1990+
.removeEnvironmentVariables(removeRepoEnvVariables)
19871991
.setTimeout(timeoutMillis)
19881992
.setQuiet(quiet)
19891993
.execute();
@@ -2247,7 +2251,7 @@ public StarlarkPath which(String program, StarlarkThread thread) throws EvalExce
22472251

22482252
@Nullable
22492253
private StarlarkPath findCommandOnPath(String program) throws IOException {
2250-
String pathEnvVariable = envVariables.get("PATH");
2254+
String pathEnvVariable = repoEnvVariables.get("PATH");
22512255
if (pathEnvVariable == null) {
22522256
return null;
22532257
}

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,8 @@ public StarlarkRepositoryContext(
9090
Path outputDirectory,
9191
IgnoredSubdirectories ignoredSubdirectories,
9292
Environment environment,
93-
ImmutableMap<String, String> env,
93+
ImmutableMap<String, String> repoEnvVariables,
94+
ImmutableMap<String, String> clientEnvVariables,
9495
DownloadManager downloadManager,
9596
double timeoutScaling,
9697
@Nullable ProcessWrapper processWrapper,
@@ -103,7 +104,8 @@ public StarlarkRepositoryContext(
103104
outputDirectory,
104105
directories,
105106
environment,
106-
env,
107+
repoEnvVariables,
108+
clientEnvVariables,
107109
downloadManager,
108110
timeoutScaling,
109111
processWrapper,

‎src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,19 @@ public void exit(AbruptExitException exception) {
335335
: UUID.randomUUID().toString();
336336

337337
this.repoEnv.putAll(clientEnv);
338+
339+
Set<String> defaultRepoEnvInherited = new TreeSet<>();
340+
defaultRepoEnvInherited.add("PATH");
341+
if (OS.getCurrent() == OS.WINDOWS) {
342+
defaultRepoEnvInherited.add("PATHEXT");
343+
}
344+
for (String name : defaultRepoEnvInherited) {
345+
String value = clientEnv.get(name);
346+
if (value != null) {
347+
this.repoEnvFromOptions.put(name, value);
348+
}
349+
}
350+
338351
// TODO: This only needs to check for loads() rather than analyzes() due to
339352
// the effect of --action_env on the repository env. Revert back to
340353
// analyzes() when --action_env no longer affects it.
@@ -347,6 +360,7 @@ public void exit(AbruptExitException exception) {
347360
visibleActionEnv.remove(name);
348361
if (!options.getOptions(CommonCommandOptions.class).repoEnvIgnoresActionEnv) {
349362
repoEnv.put(name, value);
363+
repoEnvFromOptions.put(name, value);
350364
}
351365
}
352366
case Converters.EnvVar.Inherit(String name) -> {
@@ -356,6 +370,7 @@ public void exit(AbruptExitException exception) {
356370
visibleActionEnv.remove(name);
357371
if (!options.getOptions(CommonCommandOptions.class).repoEnvIgnoresActionEnv) {
358372
repoEnv.remove(name);
373+
repoEnvFromOptions.remove(name);
359374
}
360375
}
361376
}
@@ -981,6 +996,15 @@ public Map<String, String> getRepoEnv() {
981996
return Collections.unmodifiableMap(repoEnv);
982997
}
983998

999+
/**
1000+
* Returns the repository environment created from specific client environment variables ({@code
1001+
* PATH} and on Windows {@code PATH_EXT}), {@code --repo_env}, and {@code --action_env=NAME=VALUE}
1002+
* (when {@code --incompatible_repo_env_ignores_action_env=false}).
1003+
*/
1004+
public Map<String, String> getRepoEnvFromOptions() {
1005+
return Collections.unmodifiableMap(repoEnvFromOptions);
1006+
}
1007+
9841008
/** Returns the file cache to use during this build. */
9851009
public InputMetadataProvider getFileCache() {
9861010
if (fileCache == null) {

0 commit comments

Comments
 (0)