fix(toolchains): exclude pycache symlinks from runtime files

docs/environment-variables.md

@@ -90,34 +90,39 @@ Valid values:
 * Other non-empty values mean to use isolated mode.
 :::
 
-:::{envvar} RULES_PYTHON_PYCACHE_DIR
+::::{envvar} RULES_PYTHON_PYCACHE_DIR
 
 Determines the directory that runtime-generated pyc cache files will
 be stored in.
 
 This directory may be reused between invocations, depending on the sandboxing
 configuration. Setting it to `/dev/null` will, in effect, disable runtime
 pyc caching. By setting e.g.
-`--sandbox_add_mount_pair=/tmp/rules_python_pycache`, it's possible for pyc
-caching to persist across invocations.
+`--sandbox_add_mount_pair=$HOME/.cache/rules_python_pycache`, it's possible for
+pyc caching to persist across invocations.
 
-**Behavior specific to downloaded runtimes:** 
+**Behavior specific to downloaded runtimes:**
 First `RULES_PYTHON_PYCACHE_DIR` is checked. If set, it is used as-is for
 the root pycache directory.
 
 Otherwise, the following environment variables are checked in the following
 order. Their values will have `rules_python_pycache` appended to them to form
 the root pycache directory:
 1. `XDG_CACHE_HOME`.
-2. `TMP` (non-Windows) or `TEMP` (Windows).
-3. The common platform-specific temporary directory (`/tmp` (non-Windows) or
-   `C:\Temp` (Windows)).
+2. `$HOME/.cache` (non-Windows), the XDG default when `XDG_CACHE_HOME` is
+   unset, or `LOCALAPPDATA` (Windows).
 
 If such a diretory cannot be found, or created, then `/dev/null` will be used,
 which will effectively disable pyc caching.
 
+:::{versionchanged} VERSION_NEXT_PATCH
+Temporary directories (`TMP`, `TEMP`, `/tmp` and `C:\Temp`) are no longer used.
+They get cleaned out while the fetched runtime persists, which left the
+`__pycache__` symlinks dangling.
 :::
 
+::::
+
 :::{envvar} RULES_PYTHON_PYPI_HUB_RESERVED
 
 When `1`, any PyPI hub named `"pypi"` will be renamed to `<module_name>_pypi`

news/4198.changed.md

@@ -0,0 +1,4 @@
+(toolchains) Downloaded runtimes cache pyc files under `$HOME/.cache`
+(`LOCALAPPDATA` on Windows) instead of a temporary directory when
+`XDG_CACHE_HOME` is unset
+([#4198](https://github.com/bazel-contrib/rules_python/issues/4198)).

news/4198.fixed.md

@@ -0,0 +1,4 @@
+(toolchains) `__pycache__` symlinks in downloaded runtimes no longer end up in
+runfiles, and a runtime no longer loads pyc files compiled from another
+version's sources
+([#4198](https://github.com/bazel-contrib/rules_python/issues/4198)).

python/private/python_repository.bzl

@@ -63,37 +63,40 @@ def _get_pycache_root(rctx):
     os_name = repo_utils.get_platforms_os_name(rctx)
     is_windows = os_name == "windows"
 
-    # 1. RULES_PYTHON_PYCACHE_DIR
+    # When Python uses mtime/size checking of pycs, the mtimes of files in a
+    # runtime are fixed once the repository is created. In addition, pyc
+    # content is a function of hash mode and optimization level, which can vary
+    # by workspace. Keying on both the workspace and runtime archive keeps
+    # workspaces and runtime versions from colliding on pyc files.
+    runtime_hash = rctx.attr.sha256 or rctx.attr.python_version
+    suffix = "{}/{}".format(
+        rctx.name,
+        hash("{}:{}".format(rctx.workspace_root, runtime_hash)),
+    )
+
     res = rctx.getenv("RULES_PYTHON_PYCACHE_DIR")
     if res:
-        res = res + "/" + rctx.name
-        return repo_utils.mkdir(rctx, res)
+        return repo_utils.mkdir(rctx, rctx.path(res).get_child(suffix))
 
-    # Suffix for cases 2-4
-    # The first level directory is static and documented so that it is easy to
-    # use with e.g. --sandbox_add_mount_pair=/tmp/rules_python_pycache
-    suffix = "rules_python_pycache/{}/{}".format(hash(str(rctx.workspace_root)), rctx.name)
-
-    # 2. XDG_CACHE_HOME
+    # Global locations like /tmp or TMP/TEMP are intentionally not used:
+    # * Avoids cross-user contamination or prediction/interception of the pyc
+    #   cache.
+    # * Temporary directories get cleaned out while the fetched runtime
+    #   repository persists, leaving dangling symlinks.
     res = rctx.getenv("XDG_CACHE_HOME")
-    if res:
-        path = repo_utils.mkdir(rctx, rctx.path(res).get_child(suffix))
-        if path:
-            return path
-
-    # 3. TMP or TEMP
-    res = rctx.getenv("TMP") or rctx.getenv("TEMP")
-    if res:
-        path = repo_utils.mkdir(rctx, rctx.path(res).get_child(suffix))
-        if path:
-            return path
-
-    # 4. /tmp or Windows equivalent
-    if is_windows:
-        path = rctx.path("C:/Temp").get_child(suffix)
-    else:
-        path = rctx.path("/tmp").get_child(suffix)
+    if not res:
+        if is_windows:
+            res = rctx.getenv("LOCALAPPDATA")
+        else:
+            res = rctx.getenv("HOME")
+            if res:
+                res += "/.cache"
+    if not res:
+        return None
 
+    # The first level directory is static and documented so that it is easy to
+    # use with e.g. --sandbox_add_mount_pair=$HOME/.cache/rules_python_pycache
+    path = rctx.path(res).get_child("rules_python_pycache").get_child(suffix)
     return repo_utils.mkdir(rctx, path)
 
 def _create_pycache_symlinks(rctx, logger):
@@ -252,9 +255,12 @@ def _python_repository_impl(rctx):
         # Exclude them from the glob because otherwise between the first time and second time a python toolchain is used,"
         # the definition of this filegroup will change, and depending rules will get invalidated."
         # See https://github.com/bazel-contrib/rules_python/issues/1008 for unconditionally adding these to toolchains so we can stop ignoring them."
-        # pyc* is ignored because pyc creation creates temporary .pyc.NNNN files
-        "**/__pycache__/*.pyc*",
-        "**/__pycache__/*.pyo*",
+        # The __pycache__ directories themselves are symlinks out of the
+        # repository.
+        "**/__pycache__",
+        # Also ignore everything under __pycache__, just in case pre-population
+        # of the directories misses one and a regular directory is created.
+        "**/__pycache__/**",
     ]
 
     if "windows" in platform:

tests/integration/custom_commands_test.py

@@ -28,6 +28,19 @@ def test_run_build_python_zip_false(self):
         )
         self.assert_result_matches(result, "bazel-out")
 
+    # Regression test for https://github.com/bazel-contrib/rules_python/issues/4198
+    def test_runtime_files_exclude_pycache_symlinks(self):
+        result = self.run_bazel(
+            "cquery",
+            "--repo_env=RULES_PYTHON_PYCACHE_DIR=/dev/null",
+            "--output=files",
+            "deps(//:bin)",
+        )
+        files = result.stdout.splitlines()
+        self.assertTrue(any("bin/python" in f or "python.exe" in f for f in files))
+        pycache = [f for f in files if "__pycache__" in f]
+        self.assertEqual(pycache, [])
+
 
 if __name__ == "__main__":
     # Enabling this makes the runner log subprocesses as the test goes along.