From 89d850aab819eb2dea9d6340beab1ca810dbe18d Mon Sep 17 00:00:00 2001
From: Brendan Linn <brendan.linn@gmail.com>
Date: Wed, 8 Jan 2025 20:28:38 -0800
Subject: [PATCH] fix: _which_unchecked: don't watch PATH if binary exists.
 (#2552)

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.

Fixes https://github.com/bazelbuild/rules_python/issues/2551.
---
 CHANGELOG.md                  | 3 +++
 python/private/repo_utils.bzl | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d343ae255c..6ccc568d26 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -119,6 +119,9 @@ Unreleased changes template.
   change. Fixes [#2468](https://github.com/bazelbuild/rules_python/issues/2468).
 + (gazelle) Gazelle no longer ignores `setup.py` files by default. To restore
   this behavior, apply the `# gazelle:python_ignore_files setup.py` directive.
+* Don't re-fetch whl_library, python_repository, etc. repository rules
+  whenever `PATH` changes. Fixes
+  [#2551](https://github.com/bazelbuild/rules_python/issues/2551).
 
 [pep-695]: https://peps.python.org/pep-0695/
 
diff --git a/python/private/repo_utils.bzl b/python/private/repo_utils.bzl
index 0e3f7b024b..e5c78be815 100644
--- a/python/private/repo_utils.bzl
+++ b/python/private/repo_utils.bzl
@@ -256,7 +256,7 @@ def _which_checked(mrctx, binary_name):
 def _which_unchecked(mrctx, binary_name):
     """Tests to see if a binary exists.
 
-    This is also watch the `PATH` environment variable.
+    Watches the `PATH` environment variable if the binary doesn't exist.
 
     Args:
         binary_name: name of the binary to find.
@@ -268,12 +268,12 @@ def _which_unchecked(mrctx, binary_name):
         * `describe_failure`: `Callable | None`; takes no args. If the
           binary couldn't be found, provides a detailed error description.
     """
-    path = _getenv(mrctx, "PATH", "")
     binary = mrctx.which(binary_name)
     if binary:
         _watch(mrctx, binary)
         describe_failure = None
     else:
+        path = _getenv(mrctx, "PATH", "")
         describe_failure = lambda: _which_describe_failure(binary_name, path)
 
     return struct(