The CI failure flushed out a potential bug. --repo_env=NAME results in reading values from the server process with System.getenv when clientEnv.get should have been used.

With this issue, any changes to NAME in the client environment won't propagate until the server restarts.

With this issue, any changes to NAME in the client environment won't propagate until the server restarts.

Could you split the fix into a separate PR? That would make it easier to cherry-pick into patch releases and Bazel 8.

Since --strict_repo_env will require users to allowlist env variables, I think that we should also ignore --action_env for repo rules when the flag is enabled. Otherwise users migrating to the flag may accidentally hurt their cache hit ratio and require another migration down the line.

Stale environment issue split to #24433 (this PR is dependent on the fix for tests to pass, so I'll leave it here as well and rebase once merged).

Since --strict_repo_env will require users to allowlist env variables, I think that we should also ignore --action_env for repo rules when the flag is enabled. Otherwise users migrating to the flag may accidentally hurt their cache hit ratio and require another migration down the line.

My initial thoughts were to keep this as a project preference, since currently many repository rules are heavily influenced by the host environment (sometimes intentionally). But if this change is being viewed as something to move towards, it may make sense to classify this as an incompatible change like with --incompatible_strict_action_env to signify a potential flip in a later release.

On a related note, there are a few places that use the "old" repo environment in src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java.

1. The vendor command (used in downloader).
2. The registry (bzlmod) factory (used in downloader).

And for sake of completion, the client environment is directly used in these places.

1. The credential helpers. Used in many places, including remote execution/caching.
2. The download manager (one of the instances at least...).
3. SpawnAction and ExtraAction instances (most likely used for --action_env=NAME). Usage unfortunately allows for some ambiguity.
4. The volatile input map (date format via SOURCE_DATE_EPOCH).
5. The standalone test strategy (for ${inherited} which I'm not familiar with).
6. The symlink tree strategy helper (for symlink creation via a separate process).
7. WriteAdbArgsAction action, for user home directory (HOME).
8. CppCompileAction spawn.
9. JavaCompileAction spawn.
10. The "run" command (what you'd expect and as part of configuring test environment emulation).
11. MobileInstallCommand subprocess. Running the mobile-install deployer.
12. Passed into SkyframeExecutor (which likely ultimately is the source for most other things listed here).
13. The sandboxing spawn strategies (via special env provider).
14. The local spawn strategy (as above).
15. The worker spawn strategy (as above).

The only spot of concern for me here is the download manager. Seems like in its current state, this PR may introduce some fragmentation (though such fragmentation would have already been possible with --repo_env, the new flag just makes it much more substantial).

Thanks for compiling this list, I'll have to think about this more.

Since some repo rules do depend on the environment quite heavily, have you considered making this behavior configurable on the repository_rule call instead?

Since some repo rules do depend on the environment quite heavily, have you considered making this behavior configurable on the repository_rule call instead?

No, but only because I didn't think to.

I'm not sure what such an API would look like. There is repository_rule(..., environ = [...]) (to mark variables that will trigger a refetch) however that is deprecated in favor of repository_ctx.getenv. The same cannot be said for module_extension(..., environ = [...]) strangely.

Some questions (not necessarily directed at anyone, just to help with ideation).
Should this be implemented as a binary switch that limits variables to only those explicitly listed by --repo_env? Should they be able to give a strict list of env inputs? What of the existing dynamic dependency mechanism (.getenv)? Should --strict_repo_env be a part of the design, as a way to change default environment variable inclusion behavior? Should there be any default inclusions (like PATH) and should rules be able to opt into a completely bare environment?

Looking even further beyond this, a case could even be made for sandboxing certain repository rules. The download repos created by npm_translate_lock (Rules JS) come to mind, although such repos could hypothetically be implemented with a special "download action" (to allow cache hits with few/no downloads) or more likely a slightly different design to be compatible with vendoring. This is all well outside beyond the scope here though.

Keenly awaiting this one, thank you for this work 👍

What I'm currently thinking here is;

• PATH is always inherited.
• PATHEXT is always inherited on Windows (affects PATH lookup).
• --repo_env can override the latter and apply to all.
• environ (argument) and getenv can pull in additional env vars from the client env.
• environ (attribute) no longer works as a way to peak at the environment without making it a tracked dependency.

That represents a fairly significant expansion vs. the original scope, something that ought to go through the proposal process first.

That all said, I do have a need to fully lock-down the repo environment (an extreme that is not suited to all) and would rather not leave this in limbo. So here is my plan;

1. Incompatible flag to disable --[host_]action_env=NAME=VALUE side effect.
2. Flag to make repo env explicit (not layer over the client env).
3. (eventually) A proposal for env management. Maybe repo focused, maybe more.

This looks great! I did have one query:

environ (argument) and getenv can pull in additional env vars from the client env.

With --strict_repo_env enabled, will environ(argument) allow repo rules to pull in additional env vars that have not been 'permitted' via --repo_env?

We are being impacted by this (we have tools that modify the current shell environment, and they are invalidating our builds), so I wondered if you would update on plans?

In the meantime, is there any workaround, other than wrapping all bazel calls in a wrapper that sanitizes the environment?

For those interested in this change, #26300 may also be of interest.

I would still love to see this land pre 9.0

Would it be possible to get this in 8.x?

We don't want to delay 8.5 release any further so will mark this one for later 8.x minor release.

Any chance of backporting this to 8.6.0? Given it didn't make the cut for 8.5.0?

If we backport this, we should also backport #28168.

Yikes. I'm surprised --incompatible_repo_env_ignores_action_env (#25238) and --experimental_strict_repo_env (this) caused an issue. I thought CommandEnvironment (where these are handled) was sufficiently disconnected from Skyframe, such that changes would have triggered invalidations.

@bazel-io fork 8.6.0