[9.0.0] Fix and consolidate repo env handling#28205
Conversation
There was a problem hiding this comment.
Code Review
This pull request is a significant and valuable refactoring of repository environment handling. It successfully consolidates the logic for computing the effective environment for repository rules and module extensions into CommandEnvironment, which greatly improves clarity and maintainability. The introduction of RepoEnvironmentFunction and the consistent use of repoEnv and nonstrictRepoEnv terminology are excellent changes. The replacement of ClientEnvironmentValue with the more general EnvironmentVariableValue record is also a good modernization. The changes appear to fix the described invalidation bugs correctly. Overall, this is a high-quality change that improves the codebase. I have no specific comments as the implementation looks solid.
The current invalidation logic didn't take `--incompatible_repo_env_ignores_action_env` and `--experimental_strict_repo_env` into account, which resulted in incorrect invalidation of repo rules and module extensions. This is addressed by a larger refactoring that consolidates the logic that computes the effective environment for repository rules and module extensions in `CommandEnvironment`. This environment is then read and sliced by a new `RepoEnvironmentFunction` that operates analogously to `ClientEnvironmentFunction`. Along the way, the variety of terminology in `CommandEnvironment` and various `SkyFunction`s is cleaned up to consistently use: * `repoEnv` to refer to the environment seen by repo rules and module extensions, which may or may not see the full client env based on the value of `--experimental_strict_repo_env` and * `nonstrictRepoEnv`, which refers to same environment with, conceptually, `--experimental_strict_repo_env` forced to `false`. This is used for certain non-hermetic operations such as downloader and credential helper logic. Closes #28168. PiperOrigin-RevId: 854228202 Change-Id: I900f260dd5d0c6e20dcc32eaee0821567e60d5d1
0bb5b18 to
03d8beb
Compare
The current invalidation logic didn't take
--incompatible_repo_env_ignores_action_envand--experimental_strict_repo_envinto account, which resulted in incorrect invalidation of repo rules and module extensions.This is addressed by a larger refactoring that consolidates the logic that computes the effective environment for repository rules and module extensions in
CommandEnvironment. This environment is then read and sliced by a newRepoEnvironmentFunctionthat operates analogously toClientEnvironmentFunction.Along the way, the variety of terminology in
CommandEnvironmentand variousSkyFunctions is cleaned up to consistently use:repoEnvto refer to the environment seen by repo rules and module extensions, which may or may not see the full client env based on the value of--experimental_strict_repo_envandnonstrictRepoEnv, which refers to same environment with, conceptually,--experimental_strict_repo_envforced tofalse. This is used for certain non-hermetic operations such as downloader and credential helper logic.Closes #28168.
PiperOrigin-RevId: 854228202
Change-Id: I900f260dd5d0c6e20dcc32eaee0821567e60d5d1