fix(toolchains): exclude pycache symlinks from runtime files (#4233) The runtime's `lib/**` glob matches the `__pycache__` symlinks added in #3661, so they end up in runfiles. With `RULES_PYTHON_PYCACHE_DIR=/dev/null` they are symlinks to a character device, and packaging a binary into a tar layer fails (#4198). The glob now excludes `__pycache__` and everything under it. The pyc cache is now keyed on the runtime archive's sha256 rather than a hash of the workspace root. The sources in an archive all share one mtime, so Python's mtime-and-size check can't tell a patch release's sources from the previous one's. The workspace hash didn't keep workspaces apart anyway: the repository contents cache shares one fetched runtime, symlinks included, across workspaces. The default root stops trying locations that are likely to be shared and instead tries a more reliably user-owned fallback: if `XDG_CACHE_HOME` is unset, then on non-Windows, `$HOME/.cache` is used (per [XDG][0]). On Windows, `%LOCALAPPDATA%` is used. (NB. [`platformdirs`][1] would choose `rules_python\Cache` rather than `rules_python_pycache`; the current change opts for one directory name everywhere.) Fixes #4198 [0]: https://specifications.freedesktop.org/basedir/latest/#variables [1]: https://platformdirs.readthedocs.io/en/latest/ --------- Co-authored-by: Richard Levasseur <richardlev@gmail.com>
diff --git a/docs/environment-variables.md b/docs/environment-variables.md index 3eb6222..9efa48c 100644 --- a/docs/environment-variables.md +++ b/docs/environment-variables.md
@@ -90,7 +90,7 @@ * 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. @@ -98,10 +98,10 @@ 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. @@ -109,15 +109,20 @@ 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`
diff --git a/news/4198.changed.md b/news/4198.changed.md new file mode 100644 index 0000000..542d03f --- /dev/null +++ b/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)).
diff --git a/news/4198.fixed.md b/news/4198.fixed.md new file mode 100644 index 0000000..bed5287 --- /dev/null +++ b/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)).
diff --git a/python/private/python_repository.bzl b/python/private/python_repository.bzl index 3fd0928..8517a3f 100644 --- a/python/private/python_repository.bzl +++ b/python/private/python_repository.bzl
@@ -63,37 +63,40 @@ 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 + if not res: + if is_windows: + res = rctx.getenv("LOCALAPPDATA") + else: + res = rctx.getenv("HOME") + if res: + res += "/.cache" + if not res: + return None - # 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) - + # 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 @@ # 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:
diff --git a/tests/integration/custom_commands_test.py b/tests/integration/custom_commands_test.py index 336937e..c186d57 100644 --- a/tests/integration/custom_commands_test.py +++ b/tests/integration/custom_commands_test.py
@@ -28,6 +28,19 @@ ) 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.