fix: _which_unchecked: don't watch PATH if binary exists. #2552
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This makes sense to me, I am happy to stamp it if you could add a more descriptive commit message about before and after behaviour and a Changelog.md item since having a self contained git history with the motivation for changes is always really god for digging the code later. |
|
Should we wait for this PR to release 1.1.0? @rickeylev, WDYT? |
Fixes bazelbuild#2551. Currently, the _which_unchecked helper unconditionally watches the `PATH` env var via repository_ctx.getenv. getenv is documented https://bazel.build/rules/lib/builtins/repository_ctx#getenv: > any change to the value of the variable named by name will cause > this repository to be re-fetched. Thus, any change to `PATH` will cause any repository rule that transitively calls _which_unchecked to be re-fetched. This includes python_repository and whl_library. There are reasonable development workflows that modify `PATH`. In particular, when git runs a hook, it adds the value of `GIT_EXEC_PATH` to `PATH` before invoking the hook. If the hook invokes bazel (for example, a pre-commit hook running `bazel build ...`), it will cause the Python repository rules to be re-fetched. This commit lowers the repository_ctx.getenv("PATH") call to its only use site in _which_unchecked, which happens to be a failure case (when the binary is not found). This allows the success case to not watch `PATH`, and therefore not to re-fetch the repository rule when it changes.
Ubehebe
commented
Jan 9, 2025
Done. I added the changelog entry under 1.1.0 (feel free to move). |
rickeylev
approved these changes
Jan 9, 2025
|
I'll include it in 1.1 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, the _which_unchecked helper unconditionally watches the
PATHenv var via repository_ctx.getenv. getenv is documented https://bazel.build/rules/lib/builtins/repository_ctx#getenv:Thus, any change to
PATHwill cause any repository rule that transitively calls _which_unchecked to be re-fetched. This includes python_repository and whl_library.There are reasonable development workflows that modify
PATH. In particular, when git runs a hook, it adds the value ofGIT_EXEC_PATHtoPATHbefore invoking the hook. If the hook invokes bazel (for example, a pre-commit hook runningbazel build ...), it will cause the Python repository rules to be re-fetched.This commit lowers the repository_ctx.getenv("PATH") call to its only use site in _which_unchecked, which happens to be a failure case (when the binary is not found). This allows the success case to not watch
PATH, and therefore not to re-fetch the repository rule when it changes.Fixes #2551.