From a56064b84344268883a2a046ac78c8682564e0cd Mon Sep 17 00:00:00 2001 From: Jeremy Volkman Date: Sat, 31 Oct 2026 20:07:09 +0000 Subject: [PATCH 1/7] feat(py_info): support TreeArtifact venv entries via link_to_subpaths Targets that install a Python package into a directory artifact (TreeArtifact) during the build phase cannot populate individual File objects in VenvSymlinkEntry.files when constructing virtual environment symlinks. Without sub-path metadata, overlapping package directories across multiple dependencies cannot be merged at file granularity. Add an experimental VenvSymlinkEntry.link_to_subpaths field (and builder support) so directory-backed entries can list their relative file paths for conflict merging, and teach virtual environment link generation to produce and merge TreeArtifact-backed entries. Single-file entries whose files holds an ancestor directory of link_to_path are now linked during conflict merging rather than skipped. Also prepare internal rule and tool helpers for build-action wheel installation with no change in default behavior: * Support multi-file bootstrap interpreter programs by staging sibling Python modules into a private directory with bytecode writing disabled. * Extract pure in-memory helpers for wheel RECORD and console-script shebang rewriting so build-time installers can reuse them directly. --- news/venv-symlink-link-to-subpaths.added.md | 2 + python/private/py_info.bzl | 30 +- python/private/py_interpreter_program.bzl | 72 +++- python/private/pypi/BUILD.bazel | 10 + python/private/pypi/venv_shebang_rewriter.py | 76 +++-- python/private/pypi/wheel_record_rewriter.py | 127 ++++--- python/private/venv_runfiles.bzl | 165 +++++---- tests/base_rules/py_info/py_info_tests.bzl | 2 + .../py_interpreter_program/BUILD.bazel | 18 + .../py_interpreter_program/helper_mod.py | 1 + .../py_interpreter_program/main_prog.py | 24 ++ .../py_interpreter_program_tests.bzl | 130 +++++++ .../py_interpreter_program/run_prog_test.py | 19 ++ .../py_interpreter_program/sub/main_prog.py | 1 + tests/pypi/whl_extract/BUILD.bazel | 7 + tests/pypi/whl_extract/rewriters_test.py | 102 ++++++ .../app_files_building/BUILD.bazel | 54 ++- .../app_files_building_tests.bzl | 322 +++++++++++++++++- .../app_files_building/populate_tree.py | 38 +++ .../tree_venv_merge_test.py | 46 +++ 20 files changed, 1091 insertions(+), 155 deletions(-) create mode 100644 news/venv-symlink-link-to-subpaths.added.md create mode 100644 tests/base_rules/py_interpreter_program/BUILD.bazel create mode 100644 tests/base_rules/py_interpreter_program/helper_mod.py create mode 100644 tests/base_rules/py_interpreter_program/main_prog.py create mode 100644 tests/base_rules/py_interpreter_program/py_interpreter_program_tests.bzl create mode 100644 tests/base_rules/py_interpreter_program/run_prog_test.py create mode 100644 tests/base_rules/py_interpreter_program/sub/main_prog.py create mode 100644 tests/pypi/whl_extract/rewriters_test.py create mode 100644 tests/venv_site_packages_libs/app_files_building/populate_tree.py create mode 100644 tests/venv_site_packages_libs/app_files_building/tree_venv_merge_test.py diff --git a/news/venv-symlink-link-to-subpaths.added.md b/news/venv-symlink-link-to-subpaths.added.md new file mode 100644 index 0000000000..6773b0b620 --- /dev/null +++ b/news/venv-symlink-link-to-subpaths.added.md @@ -0,0 +1,2 @@ +(rules) Custom rules can now provide directory-backed (`TreeArtifact`) virtual +environment symlink entries that merge with overlapping packages. diff --git a/python/private/py_info.bzl b/python/private/py_info.bzl index baaace1949..4ff8fd806f 100644 --- a/python/private/py_info.bzl +++ b/python/private/py_info.bzl @@ -56,6 +56,11 @@ VenvSymlinkKind = struct( def _VenvSymlinkEntry_init(**kwargs): kwargs.setdefault("link_to_file", None) + link_to_subpaths = kwargs.get("link_to_subpaths") + if link_to_subpaths == None: + kwargs["link_to_subpaths"] = depset() + return kwargs + _check_arg_type("link_to_subpaths", "depset", link_to_subpaths) return kwargs # A provider is used for memory efficiency. @@ -69,7 +74,9 @@ An entry in `PyInfo.venv_symlinks` "files": """ :type: depset[File] -Files under `link_to_path`. +Files under `link_to_path` that are expected to exist in runfiles. May instead +contain a directory artifact (`TreeArtifact`) that *contains* `link_to_path`; +see {obj}`link_to_subpaths`. This is only used when multiple targets have overlapping `venv_path` paths. e.g. if one adds files to `venv_path=a/` and another adds files to `venv_path=a/b/`. @@ -94,6 +101,18 @@ A file that `venv_path` should point to. The file to link to should also be in A runfiles-root relative path that `venv_path` will symlink to (if `link_to_file` is `None`). If `None`, it means to not create it in the venv. +""", + "link_to_subpaths": """ +:type: depset[str] + +:::{include} /_includes/experimental_api.md +::: + +Paths of the files inside the directory artifact in `files`, relative to +`link_to_path`; used instead of `files` when merging conflicting venv entries. + +:::{versionadded} VERSION_NEXT_FEATURE +::: """, "package": """ :type: str | None @@ -125,6 +144,13 @@ def _VenvSymlinkEntryBuilder_typedef(): :type: DepsetBuilder[File] ::: + :::{field} link_to_subpaths + :type: DepsetBuilder[str] + + ::::{versionadded} VERSION_NEXT_FEATURE + :::: + ::: + :::{versionadded} 2.2.0 ::: """ @@ -141,6 +167,7 @@ def _VenvSymlinkEntryBuilder(): "version": None, }, files = builders.DepsetBuilder(), + link_to_subpaths = builders.DepsetBuilder(), get_kind = lambda: _VenvSymlinkEntryBuilder_get_kind(self), set_kind = lambda k: _VenvSymlinkEntryBuilder_set_kind(self, k), get_link_to_file = lambda: _VenvSymlinkEntryBuilder_get_link_to_file(self), @@ -327,6 +354,7 @@ def _VenvSymlinkEntryBuilder_build(self): kind = self._state["kind"], link_to_file = self._state["link_to_file"], link_to_path = self._state["link_to_path"], + link_to_subpaths = self.link_to_subpaths.build(), package = self._state["package"], venv_path = self._state["venv_path"], version = self._state["version"], diff --git a/python/private/py_interpreter_program.bzl b/python/private/py_interpreter_program.bzl index 1f1abb1bfe..feff9433c0 100644 --- a/python/private/py_interpreter_program.bzl +++ b/python/private/py_interpreter_program.bzl @@ -27,15 +27,52 @@ PyInterpreterProgramInfo = provider( }, ) +def _stage_program_files(ctx): + if not ctx.files.srcs: + executable = ctx.actions.declare_file( + "{}_.{}".format(ctx.label.name, ctx.file.main.extension), + ) + ctx.actions.symlink(output = executable, target_file = ctx.file.main) + return executable, [executable] + + # When additional `srcs` are present, stage `main` and all `srcs` as + # siblings inside a private `_/` directory so `Path(__file__).parent` + # locates sibling modules under remote execution and on Windows without + # symlinks, without resolving into the source tree. + seen_basenames = {ctx.file.main.basename: ctx.file.main.short_path} + executable = ctx.actions.declare_file( + "{}_/{}".format(ctx.label.name, ctx.file.main.basename), + ) + ctx.actions.symlink(output = executable, target_file = ctx.file.main) + staged_srcs = [] + for src in ctx.files.srcs: + if src.basename in seen_basenames: + fail( + "py_interpreter_program duplicate basename '{}' from '{}' and '{}'".format( + src.basename, + seen_basenames[src.basename], + src.short_path, + ), + ) + seen_basenames[src.basename] = src.short_path + staged_src = ctx.actions.declare_file( + "{}_/{}".format(ctx.label.name, src.basename), + ) + ctx.actions.symlink(output = staged_src, target_file = src) + staged_srcs.append(staged_src) + + return executable, [executable] + staged_srcs + def _py_interpreter_program_impl(ctx): # Bazel requires the executable file to be an output created by this target. - # To avoid colliding with the source file (e.g. target=foo, main=foo.py), - # we append an underscore to the name, but keep the extension so that - # the original extension is preserved. - extension = ctx.file.main.extension - executable_name = "{}_.{}".format(ctx.label.name, extension) - executable = ctx.actions.declare_file(executable_name) - ctx.actions.symlink(output = executable, target_file = ctx.file.main) + executable, all_files = _stage_program_files(ctx) + env = dict(ctx.attr.env) + interpreter_args = list(ctx.attr.interpreter_args) + if ctx.files.srcs: + env = {"PYTHONDONTWRITEBYTECODE": "1"} | env + if "-B" not in interpreter_args: + interpreter_args = ["-B"] + interpreter_args + execution_requirements = {} if BuildSettingInfo in ctx.attr.execution_requirements: execution_requirements.update([ @@ -47,14 +84,12 @@ def _py_interpreter_program_impl(ctx): return [ DefaultInfo( executable = executable, - files = depset([executable]), - runfiles = ctx.runfiles(files = [ - executable, - ]), + files = depset(all_files), + runfiles = ctx.runfiles(files = all_files), ), PyInterpreterProgramInfo( - env = ctx.attr.env, - interpreter_args = ctx.attr.interpreter_args, + env = env, + interpreter_args = interpreter_args, main = ctx.file.main, ), testing.ExecutionInfo( @@ -103,6 +138,17 @@ ctx.actions.run( doc = "The entry point Python file.", allow_single_file = True, ), + "srcs": attr.label_list( + doc = """ +Additional Python source files imported by `main`. + +When non-empty, `main` and `srcs` are staged flat as siblings inside a private +`_/` directory (basenames must be unique), and `-B` plus +`PYTHONDONTWRITEBYTECODE=1` are enabled automatically so sibling imports do not +write `__pycache__` into the staging directory. +""", + allow_files = [".py"], + ), }, # This is set to False because this isn't a binary/executable in the usual # Bazel sense (even though it sets DefaultInfo.files_to_run). It just holds diff --git a/python/private/pypi/BUILD.bazel b/python/private/pypi/BUILD.bazel index 1cf0cad986..94de02c454 100644 --- a/python/private/pypi/BUILD.bazel +++ b/python/private/pypi/BUILD.bazel @@ -14,6 +14,7 @@ load("@bazel_skylib//:bzl_library.bzl", "bzl_library") load("//python:py_binary.bzl", "py_binary") +load("//python:py_library.bzl", "py_library") load("//python/private:distribution_filegroup.bzl", "distribution_filegroup") package(default_visibility = ["//:__subpackages__"]) @@ -25,6 +26,15 @@ exports_files( visibility = ["//visibility:public"], ) +py_library( + name = "rewriters_lib", + srcs = [ + "venv_shebang_rewriter.py", + "wheel_record_rewriter.py", + ], + visibility = ["//tests:__subpackages__"], +) + alias( name = "venv_entry_point_template", actual = select({ diff --git a/python/private/pypi/venv_shebang_rewriter.py b/python/private/pypi/venv_shebang_rewriter.py index 48e615bbf1..3ad62fcaac 100644 --- a/python/private/pypi/venv_shebang_rewriter.py +++ b/python/private/pypi/venv_shebang_rewriter.py @@ -1,42 +1,64 @@ -"""Rewrites a console_script wrapper's shebang into a batch/shell-Python polyglot.""" +"""Rewrites a console_script wrapper's shebang into a batch/shell-Python polyglot. + +Usage: + venv_shebang_rewriter.py + +Input file format: + A wheel `.data/scripts/*` script whose first line starts with `#!python` + (or `#!pythonw`). +""" + +from __future__ import annotations import os import sys +from collections.abc import Sequence + + +def rewrite_shebang_bytes(content: bytes, target_os: str) -> bytes: + """Replace the leading `#!python` shebang line with a portable polyglot header.""" + nl = content.find(b"\n") + if nl == -1: + first_line = content + rest = b"" + else: + first_line = content[: nl + 1] + rest = content[nl + 1 :] + + if target_os == "windows": + python_exe = ( + b"pythonw.exe" if first_line.startswith(b"#!pythonw") else b"python.exe" + ) + # A Batch-Python polyglot. Batch executes the first line and exits, + # while Python (via -x) ignores the first line and executes the rest. + header = ( + b'@setlocal enabledelayedexpansion & "%~dp0' + + python_exe + + b'" -x "%~f0" %* & exit /b !ERRORLEVEL!\r\n' + ) + else: + # A Shell-Python polyglot. The shell executes the triple-quoted 'exec' + # command, re-running the script with python3 from the scripts directory. + # Python ignores the triple-quoted string and continues. + header = ( + b"#!/bin/sh\n'''exec' \"$(dirname \"$0\")/python3\" \"$0\" \"$@\"\n' '''\n" + ) + return header + rest -def main(argv): - in_path, out_path, target_os = argv[1:4] +def main(argv: Sequence[str] | None = None) -> None: + args = sys.argv[1:] if argv is None else argv + in_path, out_path, target_os = args[:3] with open(in_path, "rb") as in_file: - first_line = in_file.readline() - rest = in_file.read() + content = in_file.read() with open(out_path, "wb") as out_file: - if target_os == "windows": - python_exe = ( - b"pythonw.exe" if first_line.startswith(b"#!pythonw") else b"python.exe" - ) - # A Batch-Python polyglot. Batch executes the first line and exits, - # while Python (via -x) ignores the first line and executes the rest. - out_file.write( - b'@setlocal enabledelayedexpansion & "%~dp0' - + python_exe - + b'" -x "%~f0" %* & exit /b !ERRORLEVEL!\r\n', - ) - else: - out_file.write(b"#!/bin/sh\n") - # A Shell-Python polyglot. The shell executes the triple-quoted 'exec' - # command, re-running the script with python3 from the scripts directory. - # Python ignores the triple-quoted string and continues. - out_file.write( - b"'''exec' \"$(dirname \"$0\")/python3\" \"$0\" \"$@\"\n' '''\n" - ) - - out_file.write(rest) + out_file.write(rewrite_shebang_bytes(content, target_os)) mode = os.stat(out_path).st_mode os.chmod(out_path, mode | 0o111) if __name__ == "__main__": - main(sys.argv) + main() diff --git a/python/private/pypi/wheel_record_rewriter.py b/python/private/pypi/wheel_record_rewriter.py index 8931b30360..deaad65be9 100644 --- a/python/private/pypi/wheel_record_rewriter.py +++ b/python/private/pypi/wheel_record_rewriter.py @@ -1,9 +1,26 @@ -"""Rewrites a wheel's RECORD file to reflect its final installed layout.""" +"""Rewrites a wheel's RECORD file to reflect its final installed layout. +Usage: + wheel_record_rewriter.py [ ...] + +Input file format: + A UTF-8 PEP 376 `RECORD` CSV file from `-.dist-info/RECORD`. +""" + +from __future__ import annotations + +import os import sys +from collections.abc import Iterable, Sequence, Set -def _rewrite(in_path, out_path, target_os, data_dir_basename, rewritten_scripts): +def rewrite_record_lines( + lines: Iterable[str], + target_os: str, + data_dir_basename: str, + rewritten_scripts: Set[str], +) -> list[str]: + """Rewrite `.data//...` RECORD lines to installed paths.""" data_prefix = data_dir_basename + "/" quoted_data_prefix = '"' + data_prefix @@ -20,56 +37,70 @@ def _rewrite(in_path, out_path, target_os, data_dir_basename, rewritten_scripts) purelib_repl = "" scripts_repl = "../../../bin/" + out_lines = [] + for raw_line in lines: + line = raw_line.rstrip("\r\n") + + if line.startswith(quoted_data_prefix): + quote = '"' + rest = line[len(quoted_data_prefix) :] + elif line.startswith(data_prefix): + quote = "" + rest = line[len(data_prefix) :] + else: + out_lines.append(line + "\n") + continue + + if rest.startswith("purelib/"): + out_lines.append(quote + purelib_repl + rest[len("purelib/") :] + "\n") + elif rest.startswith("platlib/"): + out_lines.append(quote + platlib_repl + rest[len("platlib/") :] + "\n") + elif rest.startswith("scripts/"): + entry = rest[len("scripts/") :] + if target_os == "windows": + idx = entry.index('"' if quote else ",") + spath, suffix = entry[:idx], entry[idx:] + if spath in rewritten_scripts: + spath += ".bat" + entry = spath + suffix + out_lines.append(quote + scripts_repl + entry + "\n") + elif rest.startswith("headers/"): + out_lines.append(quote + headers_repl + rest[len("headers/") :] + "\n") + elif rest.startswith("data/"): + out_lines.append(quote + data_repl + rest[len("data/") :] + "\n") + else: + out_lines.append(line + "\n") + return out_lines + + +def rewrite_record_file( + in_path: str | os.PathLike[str], + out_path: str | os.PathLike[str], + target_os: str, + data_dir_basename: str, + rewritten_scripts: Set[str], +) -> None: + """Read `in_path`, rewrite `.data/` entries, and write LF-terminated UTF-8 to `out_path`.""" # PEP 427 specifies archive filenames are UTF-8, and while the encoding of # dist-info files isn't formally standardized in PEP 376, packaging tools # like pip and importlib.metadata treat them as UTF-8 in practice. # See: https://discuss.python.org/t/encoding-of-files-in-the-dist-info-directory/6734/4 - with open(in_path, encoding="utf-8") as in_file, open( - out_path, "w", encoding="utf-8", newline="\n" - ) as out_file: - for raw_line in in_file: - line = raw_line.rstrip("\r\n") - - if line.startswith(quoted_data_prefix): - quote = '"' - rest = line[len(quoted_data_prefix) :] - elif line.startswith(data_prefix): - quote = "" - rest = line[len(data_prefix) :] - else: - out_file.write(line + "\n") - continue - - if rest.startswith("purelib/"): - out_file.write(quote + purelib_repl + rest[len("purelib/") :] + "\n") - elif rest.startswith("platlib/"): - out_file.write(quote + platlib_repl + rest[len("platlib/") :] + "\n") - elif rest.startswith("scripts/"): - entry = rest[len("scripts/") :] - if target_os == "windows": - if quote == '"': - idx = entry.index('"') - else: - idx = entry.index(",") - spath, suffix = entry[:idx], entry[idx:] - if spath in rewritten_scripts: - spath += ".bat" - out_file.write(quote + scripts_repl + spath + suffix + "\n") - else: - out_file.write(quote + scripts_repl + entry + "\n") - elif rest.startswith("headers/"): - out_file.write(quote + headers_repl + rest[len("headers/") :] + "\n") - elif rest.startswith("data/"): - out_file.write(quote + data_repl + rest[len("data/") :] + "\n") - else: - out_file.write(line + "\n") - - -def main(argv): - in_path, out_path, target_os, data_dir_basename = argv[1:5] - rewritten_scripts = set(argv[5:]) - _rewrite(in_path, out_path, target_os, data_dir_basename, rewritten_scripts) + with open(in_path, encoding="utf-8") as in_file: + out_lines = rewrite_record_lines( + in_file, target_os, data_dir_basename, rewritten_scripts + ) + with open(out_path, "w", encoding="utf-8", newline="\n") as out_file: + out_file.writelines(out_lines) + + +def main(argv: Sequence[str] | None = None) -> None: + args = sys.argv[1:] if argv is None else argv + in_path, out_path, target_os, data_dir_basename = args[:4] + rewritten_scripts = set(args[4:]) + rewrite_record_file( + in_path, out_path, target_os, data_dir_basename, rewritten_scripts + ) if __name__ == "__main__": - main(sys.argv) + main() diff --git a/python/private/venv_runfiles.bzl b/python/private/venv_runfiles.bzl index 45d4d24848..4b07ef3f94 100644 --- a/python/private/venv_runfiles.bzl +++ b/python/private/venv_runfiles.bzl @@ -247,56 +247,77 @@ def _merge_venv_path_group(ctx, group, keep_map): # dynamic linker symlink resolution semantics on Linux. for entry in group: root_venv_path = entry.venv_path + link_to_subpaths = entry.link_to_subpaths.to_list() + if link_to_subpaths: + # TreeArtifact-backed directory entry: expand to per-file links via link_to_subpaths. + for rel_venv_path in link_to_subpaths: + venv_path = paths.join(root_venv_path, rel_venv_path) + if venv_path not in keep_map: + keep_map[venv_path] = paths.join(entry.link_to_path, rel_venv_path) + continue + anchored_link_to_path = entry.link_to_path + "/" for file in entry.files.to_list(): rf_root_path = runfiles_root_path(ctx, file.short_path) + is_dir = file.is_directory # It's a file (or directory) being directly linked and # must be directly linked. - if rf_root_path == entry.link_to_path: + # TreeArtifact-backed single-file entry: files holds the tree root + # (an ancestor of link_to_path). + is_direct_link = rf_root_path == entry.link_to_path + is_tree_backed_file = is_dir and entry.link_to_path.startswith(rf_root_path + "/") + if is_direct_link or is_tree_backed_file: # For lack of a better option, first added wins. if entry.venv_path not in keep_map: - keep_map[entry.venv_path] = file + keep_map[entry.venv_path] = ( + file if is_direct_link else entry.link_to_path + ) # Skip anything remaining: anything left is either # the same path (first set wins), a suffix (violates # preconditions and can't link anyways), or not under # the prefix (violates preconditions). break - else: - # Compute the file-specific venv path. i.e. the relative - # path of the file under entry.venv_path, joined with - # entry.venv_path - head, match, rel_venv_path = rf_root_path.partition(anchored_link_to_path) - if not match or head: - # If link_to_path didn't match, then obviously skip. - # If head is non-empty, it means link_to_path wasn't - # found at the start - # This shouldn't occur in practice, but guard against it - # just in case - continue - venv_path = paths.join(root_venv_path, rel_venv_path) + # Compute the file-specific venv path. i.e. the relative + # path of the file under entry.venv_path, joined with + # entry.venv_path + head, match, rel_venv_path = rf_root_path.partition(anchored_link_to_path) + if not match or head: + # If link_to_path didn't match, then obviously skip. + # If head is non-empty, it means link_to_path wasn't + # found at the start + # This shouldn't occur in practice, but guard against it + # just in case + continue - # For lack of a better option, first added wins. - if venv_path not in keep_map: - keep_map[venv_path] = file + venv_path = paths.join(root_venv_path, rel_venv_path) + + # For lack of a better option, first added wins. + if venv_path not in keep_map: + keep_map[venv_path] = file -def _get_file_venv_path(ctx, f, site_packages_root): +def _get_file_venv_path(ctx, f, site_packages_root, tree_rf_root = None): """Computes a file's venv_path if it's under the site_packages_root. Args: ctx: The current ctx. - f: The file to compute the venv_path for. + f: The file (or tree-relative path string) to compute the venv_path for. site_packages_root: The site packages root path; repo-relative path. + tree_rf_root: Optional runfiles root path of a backing directory artifact. Returns: A tuple `(venv_path, rf_root_path)` if the file is under `site_packages_root` or data/, bin/, include/ otherwise `(None, None)`. """ - rf_root_path = runfiles_root_path(ctx, f.short_path) - _, _, repo_rel_path = rf_root_path.partition("/") + if tree_rf_root != None: + repo_rel_path = f + rf_root_path = "{}/{}".format(tree_rf_root, f) + else: + rf_root_path = runfiles_root_path(ctx, f.short_path) + _, _, repo_rel_path = rf_root_path.partition("/") # Check for wheel data/bin/include folders first for prefix in ["data/", "bin/", "include/"]: @@ -316,21 +337,25 @@ def get_venv_symlinks( package, version_str, site_packages_root, - namespace_package_files = []): + namespace_package_files = [], + tree_artifact = None): """Compute the VenvSymlinkEntry objects for a library. Args: ctx: {type}`ctx` the current ctx. - files: {type}`list[File]` the underlying files that are under - `site_packages_root` and intended to be part of the venv + files: {type}`list[File] | list[str]` the underlying files (or + tree-relative path strings when `tree_artifact` is given) that are + under `site_packages_root` and intended to be part of the venv contents. package: {type}`str` the Python distribution name. version_str: {type}`str` the distribution's version. site_packages_root: {type}`str` prefix under which files are considered to be part of the installed files. - namespace_package_files: {type}`list[File]` a list of files + namespace_package_files: {type}`list[File] | list[str]` a list of files that are pkgutil-style namespace packages and cannot be directly linked. + tree_artifact: {type}`File | None` optional directory artifact backing + the relative path strings in `files`. Returns: {type}`list[VenvSymlinkEntry]` the entries that describe how @@ -346,7 +371,15 @@ def get_venv_symlinks( # Append slash to prevent incorrect prefix-string matches site_packages_root += "/" - all_files = sorted(files, key = lambda f: f.short_path) + is_tree = tree_artifact != None + if is_tree: + tree_rf_root = runfiles_root_path(ctx, tree_artifact.short_path) + tree_files_depset = depset([tree_artifact]) + all_files = sorted(files) + else: + tree_rf_root = None + tree_files_depset = None + all_files = sorted(files, key = lambda f: f.short_path) cannot_be_linked_directly = {} for dirname in [ @@ -379,7 +412,7 @@ def get_venv_symlinks( for dirname in namespace_package_dirs.keys() }) for f in namespace_package_files: - venv_path, _ = _get_file_venv_path(ctx, f, site_packages_root) + venv_path, _ = _get_file_venv_path(ctx, f, site_packages_root, tree_rf_root) if venv_path == None: continue ns_dir = paths.dirname(venv_path) @@ -390,7 +423,7 @@ def get_venv_symlinks( # Where path is the venv path (i.e. relative to site_packages_prefix) venv_symlinks = {} - # List of (File, str venv_path) tuples + # List of (File | str, str venv_path) tuples files_left_to_link = [] # We want to minimize the number of files symlinked. Ideally, only the @@ -402,19 +435,22 @@ def get_venv_symlinks( # all the parent directories of the shared library can't be linked # directly. for src in all_files: - venv_path, rf_root_path = _get_file_venv_path(ctx, src, site_packages_root) + venv_path, rf_root_path = _get_file_venv_path(ctx, src, site_packages_root, tree_rf_root) if venv_path == None: continue filename = paths.basename(venv_path) if _is_linker_loaded_library(filename): + # Tree-backed single-file entries set link_to_file = None because + # ctx.runfiles(root_symlinks=...) requires individual File objects, + # so consuming binaries materialize them via UnresolvedSymlink. venv_symlinks[venv_path] = VenvSymlinkEntry( kind = VenvSymlinkKind.LIB, link_to_path = rf_root_path, - link_to_file = src, + link_to_file = None if is_tree else src, package = package, version = version_str, - files = depset([src]), + files = tree_files_depset if is_tree else depset([src]), venv_path = venv_path, ) parent = paths.dirname(venv_path) @@ -464,34 +500,24 @@ def get_venv_symlinks( optimized_groups = {} for src, venv_path in files_left_to_link: + item = venv_path if is_tree else src parent = paths.dirname(venv_path) - if not parent: - # File in root, must be linked directly - optimized_groups.setdefault(venv_path, []) - optimized_groups[venv_path].append(src) + if not parent or parent in cannot_be_linked_directly: + # File in root or in a directory that cannot be directly linked, + # so link the file directly. + optimized_groups.setdefault(venv_path, []).append(item) continue - if parent in cannot_be_linked_directly: - # File in a directory that cannot be directly linked, - # so link the file directly - optimized_groups.setdefault(venv_path, []) - optimized_groups[venv_path].append(src) - else: - # This path can be grouped. Find the highest-level directory to link. - venv_path = parent - next_parent = paths.dirname(parent) - for _ in range(len(venv_path) + 1): # Iterate enough times - if next_parent: - if next_parent not in cannot_be_linked_directly: - venv_path = next_parent - next_parent = paths.dirname(next_parent) - else: - break - else: - break + # This path can be grouped. Find the highest-level directory to link. + venv_path = parent + next_parent = paths.dirname(parent) + for _ in range(len(venv_path) + 1): # Iterate enough times + if not next_parent or next_parent in cannot_be_linked_directly: + break + venv_path = next_parent + next_parent = paths.dirname(next_parent) - optimized_groups.setdefault(venv_path, []) - optimized_groups[venv_path].append(src) + optimized_groups.setdefault(venv_path, []).append(item) # Finally, for each group, we create the VenvSymlinkEntry objects for venv_path, files in optimized_groups.items(): @@ -512,20 +538,31 @@ def get_venv_symlinks( kind = VenvSymlinkKind.LIB prefix = site_packages_root - link_to_path = ( - _get_label_runfiles_repo(ctx, files[0].owner) + - "/" + - prefix + - venv_path - ) + if is_tree: + link_to_path = tree_rf_root + "/" + prefix + venv_path + link_to_file = None + entry_files = tree_files_depset + link_to_subpaths = depset([p[len(venv_path) + 1:] for p in files if p != venv_path]) + else: + link_to_path = ( + _get_label_runfiles_repo(ctx, files[0].owner) + + "/" + + prefix + + venv_path + ) + link_to_file = files[0] if kind == VenvSymlinkKind.BIN and len(files) == 1 else None + entry_files = depset(files) + link_to_subpaths = None + venv_symlinks[venv_path] = VenvSymlinkEntry( kind = kind, link_to_path = link_to_path, - link_to_file = files[0] if kind == VenvSymlinkKind.BIN and len(files) == 1 else None, + link_to_file = link_to_file, package = package, version = version_str, venv_path = out_venv_path, - files = depset(files), + files = entry_files, + link_to_subpaths = link_to_subpaths, ) return venv_symlinks.values() diff --git a/tests/base_rules/py_info/py_info_tests.bzl b/tests/base_rules/py_info/py_info_tests.bzl index 087b115eab..d27a336e80 100644 --- a/tests/base_rules/py_info/py_info_tests.bzl +++ b/tests/base_rules/py_info/py_info_tests.bzl @@ -181,6 +181,7 @@ def _test_py_info_builder_impl(env, targets): symlink.set_venv_path("test_path") symlink.set_link_to_path("test_target") symlink.files.add(trans) + symlink.link_to_subpaths.add("sub/mod.py") builder.merge_target(targets.py1) builder.merge_targets([targets.py2]) @@ -307,6 +308,7 @@ def _test_py_info_builder_impl(env, targets): env.expect.that_str(entry.link_to_path).equals("test_target") env.expect.that_str(entry.kind).equals(VenvSymlinkKind.LIB) env.expect.that_collection(entry.files.to_list()).contains_exactly([trans]) + env.expect.that_collection(entry.link_to_subpaths.to_list()).contains_exactly(["sub/mod.py"]) env.expect.that_bool(entry.package == None).equals(True) env.expect.that_bool(entry.version == None).equals(True) env.expect.that_bool(entry.link_to_file == None).equals(True) diff --git a/tests/base_rules/py_interpreter_program/BUILD.bazel b/tests/base_rules/py_interpreter_program/BUILD.bazel new file mode 100644 index 0000000000..4a689fd3d0 --- /dev/null +++ b/tests/base_rules/py_interpreter_program/BUILD.bazel @@ -0,0 +1,18 @@ +load("//python:py_test.bzl", "py_test") +load(":py_interpreter_program_tests.bzl", "py_interpreter_program_test_suite", "run_interpreter_program") + +py_interpreter_program_test_suite( + name = "py_interpreter_program_tests", +) + +run_interpreter_program( + name = "prog_output", + tags = ["manual"], +) + +py_test( + name = "run_prog_test", + srcs = ["run_prog_test.py"], + data = [":prog_output"], + env = {"PROG_OUTPUT": "$(rootpath :prog_output)"}, +) diff --git a/tests/base_rules/py_interpreter_program/helper_mod.py b/tests/base_rules/py_interpreter_program/helper_mod.py new file mode 100644 index 0000000000..b823658192 --- /dev/null +++ b/tests/base_rules/py_interpreter_program/helper_mod.py @@ -0,0 +1 @@ +VALUE = "ok-from-sibling" diff --git a/tests/base_rules/py_interpreter_program/main_prog.py b/tests/base_rules/py_interpreter_program/main_prog.py new file mode 100644 index 0000000000..0085a6d05d --- /dev/null +++ b/tests/base_rules/py_interpreter_program/main_prog.py @@ -0,0 +1,24 @@ +"""Entry point used to verify sibling staging in py_interpreter_program.""" + +from __future__ import annotations + +import sys +from pathlib import Path + +# Intentionally avoid `.resolve()` so imports come from the staged `_/` +# directory rather than following symlinks back to the source tree. +_STAGE_DIR = Path(__file__).parent +sys.path.insert(0, str(_STAGE_DIR)) + +import helper_mod # noqa: E402 + + +def main() -> None: + out_path = Path(sys.argv[1]) + assert sys.dont_write_bytecode, "Expected -B / PYTHONDONTWRITEBYTECODE=1" + assert not (_STAGE_DIR / "__pycache__").exists() + out_path.write_text(helper_mod.VALUE + "\n", encoding="utf-8") + + +if __name__ == "__main__": + main() diff --git a/tests/base_rules/py_interpreter_program/py_interpreter_program_tests.bzl b/tests/base_rules/py_interpreter_program/py_interpreter_program_tests.bzl new file mode 100644 index 0000000000..71c4fda5d9 --- /dev/null +++ b/tests/base_rules/py_interpreter_program/py_interpreter_program_tests.bzl @@ -0,0 +1,130 @@ +"""Tests for py_interpreter_program.""" + +load("@rules_testing//lib:analysis_test.bzl", "analysis_test") +load("@rules_testing//lib:test_suite.bzl", "test_suite") +load("@rules_testing//lib:truth.bzl", "matching") +load("@rules_testing//lib:util.bzl", rt_util = "util") +load("//python/private:common.bzl", "actions_run") # buildifier: disable=bzl-visibility +load("//python/private:py_interpreter_program.bzl", "PyInterpreterProgramInfo", "py_interpreter_program") # buildifier: disable=bzl-visibility +load("//python/private:toolchain_types.bzl", "EXEC_TOOLS_TOOLCHAIN_TYPE") # buildifier: disable=bzl-visibility + +_tests = [] + +def _run_interpreter_program_impl(ctx): + out = ctx.actions.declare_file(ctx.label.name + ".txt") + args = ctx.actions.args() + args.add(out) + actions_run( + ctx, + executable = ctx.attr._program, + arguments = [args], + outputs = [out], + mnemonic = "TestPyInterpreterProgram", + progress_message = "Testing py_interpreter_program with srcs: %{label}", + ) + return [DefaultInfo(files = depset([out]))] + +_run_interpreter_program_rule = rule( + implementation = _run_interpreter_program_impl, + attrs = { + "_program": attr.label( + default = ":sample_program_with_srcs", + cfg = "exec", + ), + }, + toolchains = [EXEC_TOOLS_TOOLCHAIN_TYPE], +) + +def run_interpreter_program(name, **kwargs): + py_interpreter_program( + name = "sample_program_with_srcs", + main = "main_prog.py", + srcs = ["helper_mod.py"], + tags = ["manual"], + ) + _run_interpreter_program_rule( + name = name, + **kwargs + ) + +def _test_empty_srcs_unchanged(name): + rt_util.helper_target( + py_interpreter_program, + name = name + "_prog", + main = "main_prog.py", + ) + analysis_test( + name = name, + impl = _test_empty_srcs_unchanged_impl, + target = name + "_prog", + ) + +_tests.append(_test_empty_srcs_unchanged) + +def _test_empty_srcs_unchanged_impl(env, target): + files = [f.short_path for f in target[DefaultInfo].files.to_list()] + env.expect.that_collection(files).contains_exactly([ + "{}/{}_prog_.py".format(env.ctx.label.package, env.ctx.label.name), + ]) + info = target[PyInterpreterProgramInfo] + env.expect.that_dict(info.env).contains_exactly({}) + env.expect.that_collection(info.interpreter_args).contains_exactly([]) + +def _test_srcs_staged_as_siblings(name): + rt_util.helper_target( + py_interpreter_program, + name = name + "_prog", + main = "main_prog.py", + srcs = ["helper_mod.py"], + ) + analysis_test( + name = name, + impl = _test_srcs_staged_as_siblings_impl, + target = name + "_prog", + ) + +_tests.append(_test_srcs_staged_as_siblings) + +def _test_srcs_staged_as_siblings_impl(env, target): + pkg = env.ctx.label.package + prefix = "{}/{}_prog_".format(pkg, env.ctx.label.name) + files = [f.short_path for f in target[DefaultInfo].files.to_list()] + env.expect.that_collection(files).contains_exactly([ + prefix + "/helper_mod.py", + prefix + "/main_prog.py", + ]) + env.expect.that_str(target[DefaultInfo].files_to_run.executable.short_path).equals( + prefix + "/main_prog.py", + ) + info = target[PyInterpreterProgramInfo] + env.expect.that_dict(info.env).contains_exactly({ + "PYTHONDONTWRITEBYTECODE": "1", + }) + env.expect.that_collection(info.interpreter_args).contains_exactly(["-B"]) + +def _test_duplicate_basename_fails(name): + rt_util.helper_target( + py_interpreter_program, + name = name + "_prog", + main = "main_prog.py", + srcs = ["sub/main_prog.py"], + ) + analysis_test( + name = name, + impl = _test_duplicate_basename_fails_impl, + target = name + "_prog", + expect_failure = True, + ) + +_tests.append(_test_duplicate_basename_fails) + +def _test_duplicate_basename_fails_impl(env, target): + env.expect.that_target(target).failures().contains_predicate( + matching.contains("duplicate basename 'main_prog.py'"), + ) + +def py_interpreter_program_test_suite(name): + test_suite( + name = name, + tests = _tests, + ) diff --git a/tests/base_rules/py_interpreter_program/run_prog_test.py b/tests/base_rules/py_interpreter_program/run_prog_test.py new file mode 100644 index 0000000000..02e7003c88 --- /dev/null +++ b/tests/base_rules/py_interpreter_program/run_prog_test.py @@ -0,0 +1,19 @@ +"""Verifies execution of a py_interpreter_program target with sibling srcs.""" + +from __future__ import annotations + +import os +import unittest +from pathlib import Path + + +class RunInterpreterProgramTest(unittest.TestCase): + def test_sibling_import_and_no_bytecode(self) -> None: + out_path = Path(os.environ["PROG_OUTPUT"]) + self.assertEqual( + out_path.read_text(encoding="utf-8").strip(), "ok-from-sibling" + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/base_rules/py_interpreter_program/sub/main_prog.py b/tests/base_rules/py_interpreter_program/sub/main_prog.py new file mode 100644 index 0000000000..272b3c9185 --- /dev/null +++ b/tests/base_rules/py_interpreter_program/sub/main_prog.py @@ -0,0 +1 @@ +"""Duplicate basename fixture for py_interpreter_program negative test.""" diff --git a/tests/pypi/whl_extract/BUILD.bazel b/tests/pypi/whl_extract/BUILD.bazel index 3983477f2b..cfbafe2fa2 100644 --- a/tests/pypi/whl_extract/BUILD.bazel +++ b/tests/pypi/whl_extract/BUILD.bazel @@ -1,4 +1,5 @@ load("@rules_shell//shell:sh_test.bzl", "sh_test") +load("//python:py_test.bzl", "py_test") load(":whl_extract_tests.bzl", "whl_extract_test_suite") whl_extract_test_suite(name = "whl_extract_tests") @@ -9,3 +10,9 @@ sh_test( args = ["$(location //python/private/pypi:wheel_record_rewriter)"], data = ["//python/private/pypi:wheel_record_rewriter"], ) + +py_test( + name = "rewriters_test", + srcs = ["rewriters_test.py"], + deps = ["//python/private/pypi:rewriters_lib"], +) diff --git a/tests/pypi/whl_extract/rewriters_test.py b/tests/pypi/whl_extract/rewriters_test.py new file mode 100644 index 0000000000..4f4988c982 --- /dev/null +++ b/tests/pypi/whl_extract/rewriters_test.py @@ -0,0 +1,102 @@ +"""Unit tests for venv_shebang_rewriter and wheel_record_rewriter helpers.""" + +from __future__ import annotations + +import tempfile +import unittest +from pathlib import Path + +from python.private.pypi import venv_shebang_rewriter, wheel_record_rewriter + + +class RewritersTest(unittest.TestCase): + def test_rewrite_shebang_posix_and_windows(self) -> None: + src = b"#!python\nimport sys\nprint(sys.executable)\n" + posix_out = venv_shebang_rewriter.rewrite_shebang_bytes(src, "linux") + self.assertTrue(posix_out.startswith(b"#!/bin/sh\n'''exec' ")) + self.assertTrue(posix_out.endswith(b"import sys\nprint(sys.executable)\n")) + + win_out = venv_shebang_rewriter.rewrite_shebang_bytes(src, "windows") + self.assertIn(b"python.exe", win_out) + self.assertTrue(win_out.endswith(b"import sys\nprint(sys.executable)\n")) + + gui_out = venv_shebang_rewriter.rewrite_shebang_bytes( + b"#!pythonw\nprint(1)\n", "windows" + ) + self.assertIn(b"pythonw.exe", gui_out) + + def test_shebang_main_writes_executable_file(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + in_file = Path(tmp) / "in_script" + out_file = Path(tmp) / "out_script" + in_file.write_bytes(b"#!python\nprint('hi')\n") + venv_shebang_rewriter.main([str(in_file), str(out_file), "linux"]) + self.assertIn(b"#!/bin/sh\n", out_file.read_bytes()) + + def test_rewrite_record_lines_posix_and_windows(self) -> None: + lines = [ + "pkg/__init__.py,sha256=abc,10\r\n", + "pkg-1.0.data/purelib/pure.py,sha256=def,20\n", + "pkg-1.0.data/scripts/mytool,sha256=ghi,30\n", + "pkg-1.0.data/scripts/native_bin,sha256=jkl,40\n", + '"pkg-1.0.data/scripts/quoted,tool",sha256=mno,50\n', + "pkg-1.0.data/headers/hdr.h,sha256=pqr,60\n", + "pkg-1.0.data/data/share/doc.txt,sha256=stu,70\n", + ] + posix = wheel_record_rewriter.rewrite_record_lines( + lines, "linux", "pkg-1.0.data", {"mytool", "quoted,tool"} + ) + self.assertEqual( + posix, + [ + "pkg/__init__.py,sha256=abc,10\n", + "pure.py,sha256=def,20\n", + "../../../bin/mytool,sha256=ghi,30\n", + "../../../bin/native_bin,sha256=jkl,40\n", + '"../../../bin/quoted,tool",sha256=mno,50\n', + "../../../include/hdr.h,sha256=pqr,60\n", + "../../../share/doc.txt,sha256=stu,70\n", + ], + ) + + windows = wheel_record_rewriter.rewrite_record_lines( + lines, "windows", "pkg-1.0.data", {"mytool", "quoted,tool"} + ) + self.assertEqual( + windows, + [ + "pkg/__init__.py,sha256=abc,10\n", + "pure.py,sha256=def,20\n", + "../../Scripts/mytool.bat,sha256=ghi,30\n", + "../../Scripts/native_bin,sha256=jkl,40\n", + '"../../Scripts/quoted,tool.bat",sha256=mno,50\n', + "../../Include/hdr.h,sha256=pqr,60\n", + "../../share/doc.txt,sha256=stu,70\n", + ], + ) + + def test_record_main_writes_lf_newlines(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + in_file = Path(tmp) / "RECORD.in" + out_file = Path(tmp) / "RECORD" + in_file.write_bytes(b"pkg-1.0.data/scripts/tool,sha256=a,1\r\n") + wheel_record_rewriter.main( + [str(in_file), str(out_file), "windows", "pkg-1.0.data", "tool"] + ) + self.assertEqual( + out_file.read_bytes(), + b"../../Scripts/tool.bat,sha256=a,1\n", + ) + + def test_malformed_quoted_scripts_entry_raises(self) -> None: + with self.assertRaises(ValueError): + wheel_record_rewriter.rewrite_record_lines( + ['"pkg-1.0.data/scripts/broken,sha256=x,1\n'], + "windows", + "pkg-1.0.data", + set(), + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/venv_site_packages_libs/app_files_building/BUILD.bazel b/tests/venv_site_packages_libs/app_files_building/BUILD.bazel index 60afd34c38..1a0b470ff6 100644 --- a/tests/venv_site_packages_libs/app_files_building/BUILD.bazel +++ b/tests/venv_site_packages_libs/app_files_building/BUILD.bazel @@ -1,5 +1,57 @@ -load(":app_files_building_tests.bzl", "app_files_building_test_suite") +load("//python:py_binary.bzl", "py_binary") +load("//tests/support:py_reconfig.bzl", "py_reconfig_test") +load(":app_files_building_tests.bzl", "app_files_building_test_suite", "tree_venv_lib") + +py_binary( + name = "populate_tree", + srcs = ["populate_tree.py"], + main = "populate_tree.py", +) app_files_building_test_suite( name = "app_files_building_tests", ) + +_PKGUTIL_INIT = "__path__ = __import__('pkgutil').extend_path(__path__, __name__)\n" + +tree_venv_lib( + name = "tree_venv_runtime_a", + contents = { + "site-packages/foo/__init__.py": "ORIGIN = 'tree_a'\n", + "site-packages/foo/a.py": "VALUE = 'from_a'\n", + "site-packages/foo/plugins/__init__.py": "", + "site-packages/foo/plugins/shared.py": "WINNER = 'tree_a'\n", + "site-packages/pkgutil_ns/__init__.py": _PKGUTIL_INIT, + "site-packages/pkgutil_ns/one.py": "VALUE = 1\n", + }, + namespace_package_files = ["site-packages/pkgutil_ns/__init__.py"], + package = "tree-a", +) + +tree_venv_lib( + name = "tree_venv_runtime_b", + contents = { + "site-packages/foo/plugins/__init__.py": "", + "site-packages/foo/plugins/b.py": "VALUE = 'from_b'\n", + "site-packages/foo/plugins/shared.py": "WINNER = 'tree_b'\n", + "site-packages/pkgutil_ns/__init__.py": _PKGUTIL_INIT, + "site-packages/pkgutil_ns/two.py": "VALUE = 2\n", + }, + namespace_package_files = ["site-packages/pkgutil_ns/__init__.py"], + package = "tree-b", +) + +py_reconfig_test( + name = "tree_venv_merge_test", + srcs = ["tree_venv_merge_test.py"], + bootstrap_impl = select({ + "@platforms//os:windows": "system_python", + "//conditions:default": "script", + }), + main = "tree_venv_merge_test.py", + venvs_site_packages = "yes", + deps = [ + ":tree_venv_runtime_a", + ":tree_venv_runtime_b", + ], +) diff --git a/tests/venv_site_packages_libs/app_files_building/app_files_building_tests.bzl b/tests/venv_site_packages_libs/app_files_building/app_files_building_tests.bzl index 663ab6963e..097381721b 100644 --- a/tests/venv_site_packages_libs/app_files_building/app_files_building_tests.bzl +++ b/tests/venv_site_packages_libs/app_files_building/app_files_building_tests.bzl @@ -1,10 +1,13 @@ "" +load("@bazel_skylib//lib:types.bzl", "types") load("@rules_testing//lib:analysis_test.bzl", "analysis_test") load("@rules_testing//lib:test_suite.bzl", "test_suite") +load("@rules_testing//lib:util.bzl", rt_util = "util") load("//python:py_info.bzl", "PyInfo", "VenvSymlinkEntry", "VenvSymlinkKind") load("//python:py_library.bzl", "py_library") load("//python/private:common_labels.bzl", "labels") # buildifier: disable=bzl-visibility +load("//python/private:py_info.bzl", "PyInfoBuilder") # buildifier: disable=bzl-visibility load("//python/private:venv_runfiles.bzl", "build_link_map", "get_venv_symlinks") # buildifier: disable=bzl-visibility load("//tests/support:support.bzl", "SUPPORTS_BZLMOD") @@ -39,9 +42,11 @@ def _ctx(workspace_name = "_main"): label = Label("@@FAKE-CTX//:fake_ctx"), ) -def _file(short_path): +def _file(short_path, is_directory = False): return struct( short_path = short_path, + is_directory = is_directory, + owner = Label("//:fake_ctx"), ) def _venv_symlink(venv_path, *, link_to_path = None, files = []): @@ -74,6 +79,78 @@ def _entry(venv_path, link_to_path, files, **kwargs): **kwargs ) +def _assert_tree_artifact_equivalence( + env, + test_ctx, + files, + file_entries, + package, + version_str, + namespace_package_files = []): + pkg_prefix = env.ctx.label.package + "/" + tree_artifact = _file(pkg_prefix + "srcs", is_directory = True) + tree_files = [f.short_path[len(pkg_prefix):] for f in files] + tree_ns_files = [f.short_path[len(pkg_prefix):] for f in namespace_package_files] + tree_entries = get_venv_symlinks( + test_ctx, + tree_files, + package = package, + version_str = version_str, + site_packages_root = "site-packages", + namespace_package_files = tree_ns_files, + tree_artifact = tree_artifact, + ) + file_prefix = "{}/{}site-packages/".format(test_ctx.workspace_name, pkg_prefix) + tree_prefix = "{}/{}srcs/site-packages/".format(test_ctx.workspace_name, pkg_prefix) + file_summary = sorted([ + ( + e.kind, + e.venv_path, + e.package, + e.version, + e.link_to_path.replace(file_prefix, tree_prefix), + ) + for e in file_entries + ]) + tree_summary = sorted([ + ( + e.kind, + e.venv_path, + e.package, + e.version, + e.link_to_path, + ) + for e in tree_entries + ]) + env.expect.that_collection(tree_summary).contains_exactly(file_summary) + + other_entries = [ + _entry( + e.venv_path, + "+other/site-packages/" + e.venv_path, + [_file("../+other/site-packages/" + e.venv_path + "/__other__.py")], + kind = e.kind, + ) + for e in file_entries + ] + file_merged = build_link_map(test_ctx, list(file_entries) + other_entries) + tree_merged = build_link_map(test_ctx, list(tree_entries) + other_entries) + for kind, kind_map in file_merged.items(): + expected_resolved = {} + for k, v in kind_map.items(): + if types.is_string(v): + expected_resolved[k] = v.replace(file_prefix, tree_prefix) + else: + expected_resolved[k] = "{}/{}".format( + test_ctx.workspace_name, + v.short_path, + ).replace(file_prefix, tree_prefix) + actual_resolved = { + k: v if types.is_string(v) else "{}/{}".format(test_ctx.workspace_name, v.short_path) + for k, v in tree_merged[kind].items() + } + env.expect.that_dict(actual_resolved).contains_exactly(expected_resolved) + def _test_conflict_merging(name): analysis_test( name = name, @@ -113,6 +190,26 @@ def _test_conflict_merging_impl(env, _): _entry("ff/fmod.py", "+ff_b/site-packages/ff/fmod.py", [ _file("../+ff_b/site-packages/ff/fmod.py"), ]), + # Case: two TreeArtifact distributions provide overlapping directories via link_to_subpaths + _entry( + "tree_a", + "+tree_a/srcs/site-packages/tree_a", + [_file("../+tree_a/srcs", is_directory = True)], + link_to_subpaths = depset(["a.txt"]), + ), + _entry( + "tree_a/b", + "+tree_a_b/srcs/site-packages/tree_a/b", + [_file("../+tree_a_b/srcs", is_directory = True)], + link_to_subpaths = depset(["b.txt"]), + ), + # Case: two TreeArtifact distributions provide the same pkgutil __init__.py + _entry("tree_ns/__init__.py", "+tree_a/srcs/site-packages/tree_ns/__init__.py", [ + _file("../+tree_a/srcs", is_directory = True), + ]), + _entry("tree_ns/__init__.py", "+tree_b/srcs/site-packages/tree_ns/__init__.py", [ + _file("../+tree_b/srcs", is_directory = True), + ]), ] actual, conflicts = build_link_map(_ctx(), entries, return_conflicts = True) @@ -122,6 +219,9 @@ def _test_conflict_merging_impl(env, _): "a/b/b.txt": _file("../+pypi_a_b/site-packages/a/b/b.txt"), "duplicate/d.py": _file("../+dupe_a/site-packages/duplicate/d.py"), "ff/fmod.py": _file("../+ff_a/site-packages/ff/fmod.py"), + "tree_a/a.txt": "+tree_a/srcs/site-packages/tree_a/a.txt", + "tree_a/b/b.txt": "+tree_a_b/srcs/site-packages/tree_a/b/b.txt", + "tree_ns/__init__.py": "+tree_a/srcs/site-packages/tree_ns/__init__.py", "x/p/p.txt": _file("src-dev/x/p/p.txt"), "x/x.txt": _file("src/x/x.txt"), } @@ -169,6 +269,14 @@ def _test_optimized_grouping_complex_impl(env, target): version_str = "1.0", site_packages_root = env.ctx.label.package + "/site-packages", ) + _assert_tree_artifact_equivalence( + env, + test_ctx, + target.files.to_list(), + entries, + package = "pkg1", + version_str = "1.0", + ) actual = _venv_symlinks_from_entries(entries) rr = "{}/{}/site-packages/".format(test_ctx.workspace_name, env.ctx.label.package) @@ -259,6 +367,14 @@ def _test_optimized_grouping_single_toplevel_impl(env, target): version_str = "1.0", site_packages_root = env.ctx.label.package + "/site-packages", ) + _assert_tree_artifact_equivalence( + env, + test_ctx, + target.files.to_list(), + entries, + package = "pkg2", + version_str = "1.0", + ) actual = _venv_symlinks_from_entries(entries) rr = "{}/{}/site-packages/".format(test_ctx.workspace_name, env.ctx.label.package) @@ -312,6 +428,14 @@ def _test_optimized_grouping_implicit_namespace_packages_impl(env, target): version_str = "1.0", site_packages_root = env.ctx.label.package + "/site-packages", ) + _assert_tree_artifact_equivalence( + env, + test_ctx, + target.files.to_list(), + entries, + package = "pkg3", + version_str = "1.0", + ) actual = _venv_symlinks_from_entries(entries) rr = "{}/{}/site-packages/".format(test_ctx.workspace_name, env.ctx.label.package) @@ -380,6 +504,15 @@ def _test_optimized_grouping_pkgutil_namespace_packages_impl(env, target): site_packages_root = env.ctx.label.package + "/site-packages", namespace_package_files = ns_inits, ) + _assert_tree_artifact_equivalence( + env, + test_ctx, + files, + entries, + package = "pkgutilns", + version_str = "1.0", + namespace_package_files = ns_inits, + ) actual = _venv_symlinks_from_entries(entries) rr = "{}/{}/site-packages/".format(test_ctx.workspace_name, env.ctx.label.package) @@ -727,6 +860,14 @@ def _test_shared_library_symlinking_impl(env, target): version_str = "1.0", site_packages_root = env.ctx.label.package + "/site-packages", ) + _assert_tree_artifact_equivalence( + env, + _ctx(), + srcs, + actual_entries, + package = "foo", + version_str = "1.0", + ) actual = _venv_symlinks_from_entries(actual_entries) @@ -756,6 +897,185 @@ def _test_shared_library_symlinking_impl(env, target): } env.expect.that_dict(actual[VenvSymlinkKind.LIB]).contains_at_least(expected_libs) +def _test_default_mode_directory_merge(name): + analysis_test( + name = name, + impl = _test_default_mode_directory_merge_impl, + target = "//python:none", + ) + +_tests.append(_test_default_mode_directory_merge) + +def _test_default_mode_directory_merge_impl(env, _): + test_ctx = _ctx() + srcs = [ + _file("pkg/site-packages/foo/__init__.py"), + _file("pkg/site-packages/foo/gen_dir", is_directory = True), + _file("pkg/site-packages/foo/bar.py"), + ] + entries = get_venv_symlinks( + test_ctx, + srcs, + package = "foo", + version_str = "1.0", + site_packages_root = "pkg/site-packages", + ) + env.expect.that_int(len(entries)).equals(1) + env.expect.that_collection(entries[0].link_to_subpaths.to_list()).contains_exactly([]) + + conflicting = _entry( + "foo", + "+other/site-packages/foo", + [_file("../+other/site-packages/foo/other.py")], + ) + merged = build_link_map(test_ctx, list(entries) + [conflicting]) + env.expect.that_dict(merged[VenvSymlinkKind.LIB]).contains_exactly({ + "foo/__init__.py": srcs[0], + "foo/bar.py": srcs[2], + "foo/gen_dir": srcs[1], + "foo/other.py": _file("../+other/site-packages/foo/other.py"), + }) + + # Direct-link case: a directory File that is itself the top-level venv entry + # must remain the File object (not a path string) after conflict merging so + # default mode materializes a runfiles root_symlink rather than UnresolvedSymlink. + direct_dir = _file("pkg/site-packages/foo", is_directory = True) + direct_entries = get_venv_symlinks( + test_ctx, + [direct_dir], + package = "foo", + version_str = "1.0", + site_packages_root = "pkg/site-packages", + ) + env.expect.that_int(len(direct_entries)).equals(1) + direct_merged = build_link_map(test_ctx, list(direct_entries) + [conflicting]) + env.expect.that_bool(types.is_string(direct_merged[VenvSymlinkKind.LIB]["foo"])).equals(False) + env.expect.that_dict(direct_merged[VenvSymlinkKind.LIB]).contains_exactly({ + "foo": direct_dir, + "foo/other.py": _file("../+other/site-packages/foo/other.py"), + }) + +def _tree_venv_lib_impl(ctx): + out = ctx.actions.declare_directory(ctx.label.name + "_srcs") + args = ctx.actions.args() + args.add_all("--output-dir", [out], expand_directories = False) + for p in ctx.attr.paths: + args.add("--path", p) + for rel_path, content in ctx.attr.contents.items(): + args.add("--entry", "{}={}".format(rel_path, content)) + ctx.actions.run( + executable = ctx.executable._populate_tree, + arguments = [args], + outputs = [out], + ) + all_paths = sorted({p: True for p in ctx.attr.paths + ctx.attr.contents.keys()}.keys()) + entries = get_venv_symlinks( + ctx, + all_paths, + package = ctx.attr.package, + version_str = ctx.attr.version, + site_packages_root = "site-packages", + namespace_package_files = ctx.attr.namespace_package_files, + tree_artifact = out, + ) + builder = PyInfoBuilder.new() + builder.transitive_sources.add([out]) + builder.venv_symlinks.add(entries) + return [ + DefaultInfo( + files = depset([out]), + runfiles = ctx.runfiles(files = [out]), + ), + builder.build(), + ] + +tree_venv_lib = rule( + implementation = _tree_venv_lib_impl, + attrs = { + "contents": attr.string_dict(), + "namespace_package_files": attr.string_list(), + "package": attr.string(mandatory = True), + "paths": attr.string_list(), + "version": attr.string(default = "1.0"), + "_populate_tree": attr.label( + default = ":populate_tree", + executable = True, + cfg = "exec", + ), + }, +) + +_tree_venv_lib = tree_venv_lib + +def _test_tree_artifact_rule_merging(name): + rt_util.helper_target( + _tree_venv_lib, + name = name + "_tree_a", + package = "tree_a", + paths = [ + "bin/tool_a", + "site-packages/foo/__init__.py", + "site-packages/foo/a.py", + "site-packages/foo/plugins/shared.py", + "site-packages/libnative.so", + "site-packages/pkgutil_ns/__init__.py", + "site-packages/pkgutil_ns/one.py", + ], + namespace_package_files = ["site-packages/pkgutil_ns/__init__.py"], + ) + rt_util.helper_target( + _tree_venv_lib, + name = name + "_tree_b", + package = "tree_b", + paths = [ + "site-packages/foo/plugins/b.py", + "site-packages/foo/plugins/shared.py", + "site-packages/pkgutil_ns/__init__.py", + "site-packages/pkgutil_ns/two.py", + ], + namespace_package_files = ["site-packages/pkgutil_ns/__init__.py"], + ) + rt_util.helper_target( + py_library, + name = name + "_combined", + deps = [ + ":" + name + "_tree_a", + ":" + name + "_tree_b", + ], + ) + analysis_test( + name = name, + impl = _test_tree_artifact_rule_merging_impl, + target = name + "_combined", + ) + +_tests.append(_test_tree_artifact_rule_merging) + +def _test_tree_artifact_rule_merging_impl(env, target): + entries = target[PyInfo].venv_symlinks.to_list() + all_dirs = [f.is_directory for e in entries for f in e.files.to_list()] + env.expect.that_bool(all(all_dirs) and len(all_dirs) > 0).equals(True) + + test_ctx = _ctx(workspace_name = env.ctx.workspace_name) + pkg = env.ctx.label.package + tree_a_prefix = "{}/{}/{}_tree_a_srcs/".format(env.ctx.workspace_name, pkg, env.ctx.label.name) + tree_b_prefix = "{}/{}/{}_tree_b_srcs/".format(env.ctx.workspace_name, pkg, env.ctx.label.name) + + merged = build_link_map(test_ctx, entries) + env.expect.that_dict(merged[VenvSymlinkKind.LIB]).contains_exactly({ + "foo/__init__.py": tree_a_prefix + "site-packages/foo/__init__.py", + "foo/a.py": tree_a_prefix + "site-packages/foo/a.py", + "foo/plugins/b.py": tree_b_prefix + "site-packages/foo/plugins/b.py", + "foo/plugins/shared.py": tree_a_prefix + "site-packages/foo/plugins/shared.py", + "libnative.so": tree_a_prefix + "site-packages/libnative.so", + "pkgutil_ns/__init__.py": tree_a_prefix + "site-packages/pkgutil_ns/__init__.py", + "pkgutil_ns/one.py": tree_a_prefix + "site-packages/pkgutil_ns/one.py", + "pkgutil_ns/two.py": tree_b_prefix + "site-packages/pkgutil_ns/two.py", + }) + env.expect.that_dict(merged[VenvSymlinkKind.BIN]).contains_exactly({ + "tool_a": tree_a_prefix + "bin/tool_a", + }) + def app_files_building_test_suite(name): test_suite( name = name, diff --git a/tests/venv_site_packages_libs/app_files_building/populate_tree.py b/tests/venv_site_packages_libs/app_files_building/populate_tree.py new file mode 100644 index 0000000000..de9482135c --- /dev/null +++ b/tests/venv_site_packages_libs/app_files_building/populate_tree.py @@ -0,0 +1,38 @@ +"""Populates a declared directory artifact with relative file paths and contents. + +Usage: + populate_tree.py --output-dir [--path ...] [--entry = ...] + +Input format: + `--path` creates an empty file at `/`; `--entry` writes UTF-8 text. +""" + +from __future__ import annotations + +import argparse +from collections.abc import Sequence +from pathlib import Path + + +def main(argv: Sequence[str] | None = None) -> None: + parser = argparse.ArgumentParser(description="Populate a test TreeArtifact.") + parser.add_argument("--output-dir", type=Path, required=True) + parser.add_argument("--path", action="append", dest="paths", default=[]) + parser.add_argument("--entry", action="append", dest="entries", default=[]) + args = parser.parse_args(argv) + + args.output_dir.mkdir(parents=True, exist_ok=True) + contents: dict[str, str] = {p: "" for p in args.paths} + for item in args.entries: + rel_path, _, text = item.partition("=") + contents[rel_path] = text + + for rel_path, text in sorted(contents.items()): + dest = args.output_dir / rel_path + dest.parent.mkdir(parents=True, exist_ok=True) + with open(dest, "w", encoding="utf-8", newline="\n") as out_file: + out_file.write(text) + + +if __name__ == "__main__": + main() diff --git a/tests/venv_site_packages_libs/app_files_building/tree_venv_merge_test.py b/tests/venv_site_packages_libs/app_files_building/tree_venv_merge_test.py new file mode 100644 index 0000000000..bfee978aad --- /dev/null +++ b/tests/venv_site_packages_libs/app_files_building/tree_venv_merge_test.py @@ -0,0 +1,46 @@ +"""Runtime test verifying merged TreeArtifact-backed venv site-packages.""" + +from __future__ import annotations + +import importlib +import os +import sys +import unittest +from pathlib import Path + + +class TreeVenvMergeTest(unittest.TestCase): + def _assert_in_venv(self, mod_file: str | None, mod_name: str) -> None: + self.assertIsNotNone(mod_file, f"Expected __file__ on {mod_name}") + assert mod_file is not None # type assert + actual = str(Path(mod_file)).lower() if os.name == "nt" else mod_file + prefix = str(Path(sys.prefix)).lower() if os.name == "nt" else sys.prefix + self.assertTrue( + actual.startswith(prefix), + f"{mod_name} ({mod_file}) was not loaded from venv ({sys.prefix})", + ) + + def test_imports_from_merged_tree_artifacts(self) -> None: + self.assertNotEqual( + sys.prefix, sys.base_prefix, "Expected to run inside a venv" + ) + foo = importlib.import_module("foo") + foo_a = importlib.import_module("foo.a") + foo_b = importlib.import_module("foo.plugins.b") + foo_shared = importlib.import_module("foo.plugins.shared") + ns_one = importlib.import_module("pkgutil_ns.one") + ns_two = importlib.import_module("pkgutil_ns.two") + + self.assertEqual(foo.ORIGIN, "tree_a") + self.assertEqual(foo_a.VALUE, "from_a") + self.assertEqual(foo_b.VALUE, "from_b") + self.assertEqual(foo_shared.WINNER, "tree_a") + self.assertEqual(ns_one.VALUE, 1) + self.assertEqual(ns_two.VALUE, 2) + + for mod in (foo, foo_a, foo_b, foo_shared, ns_one, ns_two): + self._assert_in_venv(mod.__file__, mod.__name__) + + +if __name__ == "__main__": + unittest.main() From 6be669841d98d5a00278f4138b36dca13dc43596 Mon Sep 17 00:00:00 2001 From: Jeremy Volkman Date: Mon, 2 Nov 2026 03:27:58 +0000 Subject: [PATCH 2/7] refactor(py_info): address review feedback Drop the py_interpreter_program staging changes so it stays a thin bootstrap shim, normalize get_venv_symlinks inputs into typed candidate structs before grouping, clarify VenvSymlinkEntry and merge comments, fix MyST directive nesting, and rename the changelog fragment to 4396. --- ...ink-to-subpaths.added.md => 4396.added.md} | 0 python/private/py_info.bzl | 30 ++- python/private/py_interpreter_program.bzl | 72 ++---- python/private/venv_runfiles.bzl | 215 +++++++++++------- .../py_interpreter_program/BUILD.bazel | 18 -- .../py_interpreter_program/helper_mod.py | 1 - .../py_interpreter_program/main_prog.py | 24 -- .../py_interpreter_program_tests.bzl | 130 ----------- .../py_interpreter_program/run_prog_test.py | 19 -- .../py_interpreter_program/sub/main_prog.py | 1 - .../app_files_building_tests.bzl | 14 +- 11 files changed, 169 insertions(+), 355 deletions(-) rename news/{venv-symlink-link-to-subpaths.added.md => 4396.added.md} (100%) delete mode 100644 tests/base_rules/py_interpreter_program/BUILD.bazel delete mode 100644 tests/base_rules/py_interpreter_program/helper_mod.py delete mode 100644 tests/base_rules/py_interpreter_program/main_prog.py delete mode 100644 tests/base_rules/py_interpreter_program/py_interpreter_program_tests.bzl delete mode 100644 tests/base_rules/py_interpreter_program/run_prog_test.py delete mode 100644 tests/base_rules/py_interpreter_program/sub/main_prog.py diff --git a/news/venv-symlink-link-to-subpaths.added.md b/news/4396.added.md similarity index 100% rename from news/venv-symlink-link-to-subpaths.added.md rename to news/4396.added.md diff --git a/python/private/py_info.bzl b/python/private/py_info.bzl index 4ff8fd806f..6c0f3a17dd 100644 --- a/python/private/py_info.bzl +++ b/python/private/py_info.bzl @@ -74,12 +74,18 @@ An entry in `PyInfo.venv_symlinks` "files": """ :type: depset[File] -Files under `link_to_path` that are expected to exist in runfiles. May instead -contain a directory artifact (`TreeArtifact`) that *contains* `link_to_path`; -see {obj}`link_to_subpaths`. - -This is only used when multiple targets have overlapping `venv_path` paths. e.g. -if one adds files to `venv_path=a/` and another adds files to `venv_path=a/b/`. +Files backing this entry in runfiles. Entries may be regular `File` objects or +directory artifacts (`File.is_directory`). + +If any entries are directory artifacts, {obj}`link_to_subpaths` must list every +file under `link_to_path`, relative to it (left empty when `link_to_path` +points to a single file), because a directory artifact's contents aren't known +at analysis time. When {obj}`link_to_subpaths` is non-empty, conflict merging +uses it instead of `files`, and `files` then only supplies runfiles. + +Conflict merging occurs when multiple targets have overlapping `venv_path` +paths, e.g. if one adds files to `venv_path=a/` and another adds files to +`venv_path=a/b/`. """, "kind": """ :type: str @@ -108,8 +114,10 @@ A runfiles-root relative path that `venv_path` will symlink to (if :::{include} /_includes/experimental_api.md ::: -Paths of the files inside the directory artifact in `files`, relative to -`link_to_path`; used instead of `files` when merging conflicting venv entries. +Relative paths of every file under `link_to_path` when `files` contains +directory artifacts (`File.is_directory`) and `link_to_path` is a directory. +When non-empty, conflict merging uses these paths instead of `files` (and +`files` then only supplies runfiles). :::{versionadded} VERSION_NEXT_FEATURE ::: @@ -144,12 +152,12 @@ def _VenvSymlinkEntryBuilder_typedef(): :type: DepsetBuilder[File] ::: - :::{field} link_to_subpaths + ::::{field} link_to_subpaths :type: DepsetBuilder[str] - ::::{versionadded} VERSION_NEXT_FEATURE - :::: + :::{versionadded} VERSION_NEXT_FEATURE ::: + :::: :::{versionadded} 2.2.0 ::: diff --git a/python/private/py_interpreter_program.bzl b/python/private/py_interpreter_program.bzl index feff9433c0..1f1abb1bfe 100644 --- a/python/private/py_interpreter_program.bzl +++ b/python/private/py_interpreter_program.bzl @@ -27,52 +27,15 @@ PyInterpreterProgramInfo = provider( }, ) -def _stage_program_files(ctx): - if not ctx.files.srcs: - executable = ctx.actions.declare_file( - "{}_.{}".format(ctx.label.name, ctx.file.main.extension), - ) - ctx.actions.symlink(output = executable, target_file = ctx.file.main) - return executable, [executable] - - # When additional `srcs` are present, stage `main` and all `srcs` as - # siblings inside a private `_/` directory so `Path(__file__).parent` - # locates sibling modules under remote execution and on Windows without - # symlinks, without resolving into the source tree. - seen_basenames = {ctx.file.main.basename: ctx.file.main.short_path} - executable = ctx.actions.declare_file( - "{}_/{}".format(ctx.label.name, ctx.file.main.basename), - ) - ctx.actions.symlink(output = executable, target_file = ctx.file.main) - staged_srcs = [] - for src in ctx.files.srcs: - if src.basename in seen_basenames: - fail( - "py_interpreter_program duplicate basename '{}' from '{}' and '{}'".format( - src.basename, - seen_basenames[src.basename], - src.short_path, - ), - ) - seen_basenames[src.basename] = src.short_path - staged_src = ctx.actions.declare_file( - "{}_/{}".format(ctx.label.name, src.basename), - ) - ctx.actions.symlink(output = staged_src, target_file = src) - staged_srcs.append(staged_src) - - return executable, [executable] + staged_srcs - def _py_interpreter_program_impl(ctx): # Bazel requires the executable file to be an output created by this target. - executable, all_files = _stage_program_files(ctx) - env = dict(ctx.attr.env) - interpreter_args = list(ctx.attr.interpreter_args) - if ctx.files.srcs: - env = {"PYTHONDONTWRITEBYTECODE": "1"} | env - if "-B" not in interpreter_args: - interpreter_args = ["-B"] + interpreter_args - + # To avoid colliding with the source file (e.g. target=foo, main=foo.py), + # we append an underscore to the name, but keep the extension so that + # the original extension is preserved. + extension = ctx.file.main.extension + executable_name = "{}_.{}".format(ctx.label.name, extension) + executable = ctx.actions.declare_file(executable_name) + ctx.actions.symlink(output = executable, target_file = ctx.file.main) execution_requirements = {} if BuildSettingInfo in ctx.attr.execution_requirements: execution_requirements.update([ @@ -84,12 +47,14 @@ def _py_interpreter_program_impl(ctx): return [ DefaultInfo( executable = executable, - files = depset(all_files), - runfiles = ctx.runfiles(files = all_files), + files = depset([executable]), + runfiles = ctx.runfiles(files = [ + executable, + ]), ), PyInterpreterProgramInfo( - env = env, - interpreter_args = interpreter_args, + env = ctx.attr.env, + interpreter_args = ctx.attr.interpreter_args, main = ctx.file.main, ), testing.ExecutionInfo( @@ -138,17 +103,6 @@ ctx.actions.run( doc = "The entry point Python file.", allow_single_file = True, ), - "srcs": attr.label_list( - doc = """ -Additional Python source files imported by `main`. - -When non-empty, `main` and `srcs` are staged flat as siblings inside a private -`_/` directory (basenames must be unique), and `-B` plus -`PYTHONDONTWRITEBYTECODE=1` are enabled automatically so sibling imports do not -write `__pycache__` into the staging directory. -""", - allow_files = [".py"], - ), }, # This is set to False because this isn't a binary/executable in the usual # Bazel sense (even though it sets DefaultInfo.files_to_run). It just holds diff --git a/python/private/venv_runfiles.bzl b/python/private/venv_runfiles.bzl index 4b07ef3f94..0f42b0e7bb 100644 --- a/python/private/venv_runfiles.bzl +++ b/python/private/venv_runfiles.bzl @@ -138,10 +138,10 @@ def build_link_map(ctx, entries, return_conflicts = False): to be resolved and merged. Returns: - {type}`dict[str, dict[str, str|File]]` Mappings of venv paths to their + {type}`dict[str, dict[str, File | str]]` Mappings of venv paths to their backing files. The first key is a `VenvSymlinkKind` value. The inner dict keys are venv paths relative to the kind's directory. The - inner dict values are strings or Files to link to. + inner dict values are `File`s or `rf_root_path` strings to link to. """ version_by_pkg = {} # dict[str pkg, str version] @@ -164,10 +164,10 @@ def build_link_map(ctx, entries, return_conflicts = False): # else: ignore it; not the selected version # final paths to keep, grouped by kind - keep_link_map = {} # dict[str kind, dict[path, str|File]] + keep_link_map = {} # dict[str kind, dict[str venv_path, File | str rf_root_path]] conflicts = [] if return_conflicts else None for kind, entries in entries_by_kind.items(): - # dict[str kind-relative path, str|File link_to] + # dict[str venv_path, File | str rf_root_path] keep_kind_link_map = {} groups = _group_venv_path_entries(entries) @@ -235,8 +235,8 @@ def _merge_venv_path_group(ctx, group, keep_map): ctx: {type}`ctx` current ctx. group: {type}`list[VenvSymlinkEntry]` a group of entries with overlapping `venv_path` prefixes, ordered from shortest to longest path. - keep_map: {type}`dict[str, str|File]` files kept after merging are - populated into this map. + keep_map: {type}`dict[str, File | str]` (`dict[str venv_path, File | str rf_root_path]`) + files kept after merging are populated into this map. """ # TODO: Compute the minimum number of entries to create. This can't avoid @@ -249,11 +249,12 @@ def _merge_venv_path_group(ctx, group, keep_map): root_venv_path = entry.venv_path link_to_subpaths = entry.link_to_subpaths.to_list() if link_to_subpaths: - # TreeArtifact-backed directory entry: expand to per-file links via link_to_subpaths. - for rel_venv_path in link_to_subpaths: - venv_path = paths.join(root_venv_path, rel_venv_path) + # subpath is relative to both entry.link_to_path (runfiles side) and + # entry.venv_path (venv side); the two trees mirror each other below them. + for subpath in link_to_subpaths: + venv_path = paths.join(root_venv_path, subpath) if venv_path not in keep_map: - keep_map[venv_path] = paths.join(entry.link_to_path, rel_venv_path) + keep_map[venv_path] = paths.join(entry.link_to_path, subpath) continue anchored_link_to_path = entry.link_to_path + "/" @@ -298,38 +299,77 @@ def _merge_venv_path_group(ctx, group, keep_map): if venv_path not in keep_map: keep_map[venv_path] = file -def _get_file_venv_path(ctx, f, site_packages_root, tree_rf_root = None): - """Computes a file's venv_path if it's under the site_packages_root. +def _repo_rel_path_to_venv_path(repo_rel_path, site_packages_root): + """Computes a file's venv_path from its repo-relative (or tree-relative) path. Args: - ctx: The current ctx. - f: The file (or tree-relative path string) to compute the venv_path for. - site_packages_root: The site packages root path; repo-relative - path. - tree_rf_root: Optional runfiles root path of a backing directory artifact. + repo_rel_path: {type}`str` Repo-relative (or tree-relative) path. + site_packages_root: {type}`str` Trailing-slash-terminated site-packages + prefix (e.g. `"site-packages/"`). Returns: - A tuple `(venv_path, rf_root_path)` if the file is under - `site_packages_root` or data/, bin/, include/ otherwise `(None, None)`. + {type}`str | None` The `venv_path` if `repo_rel_path` is under `data/`, + `bin/`, `include/`, or `site_packages_root`; otherwise `None`. """ - if tree_rf_root != None: - repo_rel_path = f - rf_root_path = "{}/{}".format(tree_rf_root, f) - else: - rf_root_path = runfiles_root_path(ctx, f.short_path) - _, _, repo_rel_path = rf_root_path.partition("/") # Check for wheel data/bin/include folders first for prefix in ["data/", "bin/", "include/"]: if repo_rel_path.startswith(prefix): - return (repo_rel_path, rf_root_path) + return repo_rel_path head, found_sp_root, venv_path = repo_rel_path.partition(site_packages_root) if head or not found_sp_root: # If head is set, then the path didn't start with site_packages_root # if found_sp_root is empty, then it means it wasn't found at all. - return (None, None) - return (venv_path, rf_root_path) + return None + return venv_path + +def _venv_candidates(ctx, files, site_packages_root, tree_artifact): + """Normalizes input Files or tree-relative path strings into sorted candidates. + + Args: + ctx: {type}`ctx` The current ctx. + files: {type}`list[File] | list[str]` Regular `File` objects, or + `tree_artifact`-relative path strings (`tree_rel_path`) when + `tree_artifact` is given. + site_packages_root: {type}`str` Trailing-slash-terminated site-packages + prefix (e.g. `"site-packages/"`). + tree_artifact: {type}`File | None` Optional directory artifact backing + the relative path strings in `files`. + + Returns: + {type}`list[struct]` Sorted list of `struct(venv_path, rf_root_path, file)` + for entries mapped into the venv, where `file` is {type}`File | None` + (`None` when backed by `tree_artifact`). + """ + if not files: + return [] + + candidates = [] + if tree_artifact != None: + tree_rf_root = runfiles_root_path(ctx, tree_artifact.short_path) + for tree_rel_path in sorted(files): + venv_path = _repo_rel_path_to_venv_path(tree_rel_path, site_packages_root) + if venv_path == None: + continue + candidates.append(struct( + venv_path = venv_path, + rf_root_path = "{}/{}".format(tree_rf_root, tree_rel_path), + file = None, + )) + else: + for file in sorted(files, key = lambda f: f.short_path): + rf_root_path = runfiles_root_path(ctx, file.short_path) + _, _, repo_rel_path = rf_root_path.partition("/") + venv_path = _repo_rel_path_to_venv_path(repo_rel_path, site_packages_root) + if venv_path == None: + continue + candidates.append(struct( + venv_path = venv_path, + rf_root_path = rf_root_path, + file = file, + )) + return candidates def get_venv_symlinks( ctx, @@ -344,15 +384,16 @@ def get_venv_symlinks( Args: ctx: {type}`ctx` the current ctx. files: {type}`list[File] | list[str]` the underlying files (or - tree-relative path strings when `tree_artifact` is given) that are - under `site_packages_root` and intended to be part of the venv - contents. + `tree_artifact`-relative path strings when `tree_artifact` is given) + that are under `site_packages_root` and intended to be part of the + venv contents. package: {type}`str` the Python distribution name. version_str: {type}`str` the distribution's version. site_packages_root: {type}`str` prefix under which files are considered to be part of the installed files. namespace_package_files: {type}`list[File] | list[str]` a list of files - that are pkgutil-style namespace packages and cannot be + (or `tree_artifact`-relative path strings when `tree_artifact` is + given) that are pkgutil-style namespace packages and cannot be directly linked. tree_artifact: {type}`File | None` optional directory artifact backing the relative path strings in `files`. @@ -371,15 +412,11 @@ def get_venv_symlinks( # Append slash to prevent incorrect prefix-string matches site_packages_root += "/" - is_tree = tree_artifact != None - if is_tree: - tree_rf_root = runfiles_root_path(ctx, tree_artifact.short_path) - tree_files_depset = depset([tree_artifact]) - all_files = sorted(files) - else: - tree_rf_root = None - tree_files_depset = None - all_files = sorted(files, key = lambda f: f.short_path) + candidates = _venv_candidates(ctx, files, site_packages_root, tree_artifact) + tree_rf_root = ( + runfiles_root_path(ctx, tree_artifact.short_path) if tree_artifact != None else None + ) + tree_files_depset = depset([tree_artifact]) if tree_artifact != None else None cannot_be_linked_directly = {} for dirname in [ @@ -411,20 +448,21 @@ def get_venv_symlinks( dirname: True for dirname in namespace_package_dirs.keys() }) - for f in namespace_package_files: - venv_path, _ = _get_file_venv_path(ctx, f, site_packages_root, tree_rf_root) - if venv_path == None: - continue - ns_dir = paths.dirname(venv_path) + for ns_candidate in _venv_candidates( + ctx, + namespace_package_files, + site_packages_root, + tree_artifact, + ): + ns_dir = paths.dirname(ns_candidate.venv_path) namespace_package_dirs[ns_dir] = True cannot_be_linked_directly[ns_dir] = True - # dict[str path, VenvSymlinkEntry] - # Where path is the venv path (i.e. relative to site_packages_prefix) + # dict[str venv_path, VenvSymlinkEntry] venv_symlinks = {} - # List of (File | str, str venv_path) tuples - files_left_to_link = [] + # list[struct(venv_path, rf_root_path, file)] + candidates_left_to_link = [] # We want to minimize the number of files symlinked. Ideally, only the # top-level directories are symlinked. Unfortunately, shared libraries @@ -434,23 +472,20 @@ def get_venv_symlinks( # To fix, we have to directly link shared libraries. This then means that # all the parent directories of the shared library can't be linked # directly. - for src in all_files: - venv_path, rf_root_path = _get_file_venv_path(ctx, src, site_packages_root, tree_rf_root) - if venv_path == None: - continue - + for candidate in candidates: + venv_path = candidate.venv_path filename = paths.basename(venv_path) if _is_linker_loaded_library(filename): # Tree-backed single-file entries set link_to_file = None because # ctx.runfiles(root_symlinks=...) requires individual File objects, - # so consuming binaries materialize them via UnresolvedSymlink. + # so consuming binaries materialize them via ctx.actions.declare_symlink(). venv_symlinks[venv_path] = VenvSymlinkEntry( kind = VenvSymlinkKind.LIB, - link_to_path = rf_root_path, - link_to_file = None if is_tree else src, + link_to_path = candidate.rf_root_path, + link_to_file = candidate.file, package = package, version = version_str, - files = tree_files_depset if is_tree else depset([src]), + files = tree_files_depset if tree_artifact != None else depset([candidate.file]), venv_path = venv_path, ) parent = paths.dirname(venv_path) @@ -463,7 +498,7 @@ def get_venv_symlinks( cannot_be_linked_directly[parent] = True parent = paths.dirname(parent) else: - files_left_to_link.append((src, venv_path)) + candidates_left_to_link.append(candidate) top_level_dirname, _, tail = venv_path.partition("/") if ( @@ -493,68 +528,76 @@ def get_venv_symlinks( # At this point, venv_symlinks has entries for the shared libraries # and cannot_be_linked_directly has the directories that cannot be - # directly linked. Next, we loop over the remaining files and group + # directly linked. Next, we loop over the remaining candidates and group # them into the highest level directory that can be linked. - # dict[str venv_path, list[File]] - optimized_groups = {} + # dict[str group_venv_path, list[struct(venv_path, rf_root_path, file)]] + groups = {} - for src, venv_path in files_left_to_link: - item = venv_path if is_tree else src + for candidate in candidates_left_to_link: + venv_path = candidate.venv_path parent = paths.dirname(venv_path) if not parent or parent in cannot_be_linked_directly: # File in root or in a directory that cannot be directly linked, # so link the file directly. - optimized_groups.setdefault(venv_path, []).append(item) + groups.setdefault(venv_path, []).append(candidate) continue # This path can be grouped. Find the highest-level directory to link. - venv_path = parent + group_venv_path = parent next_parent = paths.dirname(parent) - for _ in range(len(venv_path) + 1): # Iterate enough times + for _ in range(len(group_venv_path) + 1): # Iterate enough times if not next_parent or next_parent in cannot_be_linked_directly: break - venv_path = next_parent + group_venv_path = next_parent next_parent = paths.dirname(next_parent) - optimized_groups.setdefault(venv_path, []).append(item) + groups.setdefault(group_venv_path, []).append(candidate) # Finally, for each group, we create the VenvSymlinkEntry objects - for venv_path, files in optimized_groups.items(): - if venv_path.startswith("data/"): - out_venv_path = venv_path[len("data/"):] + for group_venv_path, members in groups.items(): + if group_venv_path.startswith("data/"): + out_venv_path = group_venv_path[len("data/"):] kind = VenvSymlinkKind.DATA prefix = "" - elif venv_path.startswith("include/"): - out_venv_path = venv_path[len("include/"):] + elif group_venv_path.startswith("include/"): + out_venv_path = group_venv_path[len("include/"):] kind = VenvSymlinkKind.INCLUDE prefix = "" - elif venv_path.startswith("bin/"): - out_venv_path = venv_path[len("bin/"):] + elif group_venv_path.startswith("bin/"): + out_venv_path = group_venv_path[len("bin/"):] kind = VenvSymlinkKind.BIN prefix = "" else: - out_venv_path = venv_path + out_venv_path = group_venv_path kind = VenvSymlinkKind.LIB prefix = site_packages_root - if is_tree: - link_to_path = tree_rf_root + "/" + prefix + venv_path + is_file_link = members[0].venv_path == group_venv_path + if tree_artifact != None: + link_to_path = tree_rf_root + "/" + prefix + group_venv_path link_to_file = None entry_files = tree_files_depset - link_to_subpaths = depset([p[len(venv_path) + 1:] for p in files if p != venv_path]) + link_to_subpaths = depset( + [] if is_file_link else [ + m.venv_path[len(group_venv_path) + 1:] + for m in members + ], + ) else: link_to_path = ( - _get_label_runfiles_repo(ctx, files[0].owner) + + _get_label_runfiles_repo(ctx, members[0].file.owner) + "/" + prefix + - venv_path + group_venv_path + ) + link_to_file = ( + members[0].file if kind == VenvSymlinkKind.BIN and len(members) == 1 else None ) - link_to_file = files[0] if kind == VenvSymlinkKind.BIN and len(files) == 1 else None - entry_files = depset(files) + entry_files = depset([m.file for m in members]) link_to_subpaths = None - venv_symlinks[venv_path] = VenvSymlinkEntry( + venv_symlinks[group_venv_path] = VenvSymlinkEntry( kind = kind, link_to_path = link_to_path, link_to_file = link_to_file, diff --git a/tests/base_rules/py_interpreter_program/BUILD.bazel b/tests/base_rules/py_interpreter_program/BUILD.bazel deleted file mode 100644 index 4a689fd3d0..0000000000 --- a/tests/base_rules/py_interpreter_program/BUILD.bazel +++ /dev/null @@ -1,18 +0,0 @@ -load("//python:py_test.bzl", "py_test") -load(":py_interpreter_program_tests.bzl", "py_interpreter_program_test_suite", "run_interpreter_program") - -py_interpreter_program_test_suite( - name = "py_interpreter_program_tests", -) - -run_interpreter_program( - name = "prog_output", - tags = ["manual"], -) - -py_test( - name = "run_prog_test", - srcs = ["run_prog_test.py"], - data = [":prog_output"], - env = {"PROG_OUTPUT": "$(rootpath :prog_output)"}, -) diff --git a/tests/base_rules/py_interpreter_program/helper_mod.py b/tests/base_rules/py_interpreter_program/helper_mod.py deleted file mode 100644 index b823658192..0000000000 --- a/tests/base_rules/py_interpreter_program/helper_mod.py +++ /dev/null @@ -1 +0,0 @@ -VALUE = "ok-from-sibling" diff --git a/tests/base_rules/py_interpreter_program/main_prog.py b/tests/base_rules/py_interpreter_program/main_prog.py deleted file mode 100644 index 0085a6d05d..0000000000 --- a/tests/base_rules/py_interpreter_program/main_prog.py +++ /dev/null @@ -1,24 +0,0 @@ -"""Entry point used to verify sibling staging in py_interpreter_program.""" - -from __future__ import annotations - -import sys -from pathlib import Path - -# Intentionally avoid `.resolve()` so imports come from the staged `_/` -# directory rather than following symlinks back to the source tree. -_STAGE_DIR = Path(__file__).parent -sys.path.insert(0, str(_STAGE_DIR)) - -import helper_mod # noqa: E402 - - -def main() -> None: - out_path = Path(sys.argv[1]) - assert sys.dont_write_bytecode, "Expected -B / PYTHONDONTWRITEBYTECODE=1" - assert not (_STAGE_DIR / "__pycache__").exists() - out_path.write_text(helper_mod.VALUE + "\n", encoding="utf-8") - - -if __name__ == "__main__": - main() diff --git a/tests/base_rules/py_interpreter_program/py_interpreter_program_tests.bzl b/tests/base_rules/py_interpreter_program/py_interpreter_program_tests.bzl deleted file mode 100644 index 71c4fda5d9..0000000000 --- a/tests/base_rules/py_interpreter_program/py_interpreter_program_tests.bzl +++ /dev/null @@ -1,130 +0,0 @@ -"""Tests for py_interpreter_program.""" - -load("@rules_testing//lib:analysis_test.bzl", "analysis_test") -load("@rules_testing//lib:test_suite.bzl", "test_suite") -load("@rules_testing//lib:truth.bzl", "matching") -load("@rules_testing//lib:util.bzl", rt_util = "util") -load("//python/private:common.bzl", "actions_run") # buildifier: disable=bzl-visibility -load("//python/private:py_interpreter_program.bzl", "PyInterpreterProgramInfo", "py_interpreter_program") # buildifier: disable=bzl-visibility -load("//python/private:toolchain_types.bzl", "EXEC_TOOLS_TOOLCHAIN_TYPE") # buildifier: disable=bzl-visibility - -_tests = [] - -def _run_interpreter_program_impl(ctx): - out = ctx.actions.declare_file(ctx.label.name + ".txt") - args = ctx.actions.args() - args.add(out) - actions_run( - ctx, - executable = ctx.attr._program, - arguments = [args], - outputs = [out], - mnemonic = "TestPyInterpreterProgram", - progress_message = "Testing py_interpreter_program with srcs: %{label}", - ) - return [DefaultInfo(files = depset([out]))] - -_run_interpreter_program_rule = rule( - implementation = _run_interpreter_program_impl, - attrs = { - "_program": attr.label( - default = ":sample_program_with_srcs", - cfg = "exec", - ), - }, - toolchains = [EXEC_TOOLS_TOOLCHAIN_TYPE], -) - -def run_interpreter_program(name, **kwargs): - py_interpreter_program( - name = "sample_program_with_srcs", - main = "main_prog.py", - srcs = ["helper_mod.py"], - tags = ["manual"], - ) - _run_interpreter_program_rule( - name = name, - **kwargs - ) - -def _test_empty_srcs_unchanged(name): - rt_util.helper_target( - py_interpreter_program, - name = name + "_prog", - main = "main_prog.py", - ) - analysis_test( - name = name, - impl = _test_empty_srcs_unchanged_impl, - target = name + "_prog", - ) - -_tests.append(_test_empty_srcs_unchanged) - -def _test_empty_srcs_unchanged_impl(env, target): - files = [f.short_path for f in target[DefaultInfo].files.to_list()] - env.expect.that_collection(files).contains_exactly([ - "{}/{}_prog_.py".format(env.ctx.label.package, env.ctx.label.name), - ]) - info = target[PyInterpreterProgramInfo] - env.expect.that_dict(info.env).contains_exactly({}) - env.expect.that_collection(info.interpreter_args).contains_exactly([]) - -def _test_srcs_staged_as_siblings(name): - rt_util.helper_target( - py_interpreter_program, - name = name + "_prog", - main = "main_prog.py", - srcs = ["helper_mod.py"], - ) - analysis_test( - name = name, - impl = _test_srcs_staged_as_siblings_impl, - target = name + "_prog", - ) - -_tests.append(_test_srcs_staged_as_siblings) - -def _test_srcs_staged_as_siblings_impl(env, target): - pkg = env.ctx.label.package - prefix = "{}/{}_prog_".format(pkg, env.ctx.label.name) - files = [f.short_path for f in target[DefaultInfo].files.to_list()] - env.expect.that_collection(files).contains_exactly([ - prefix + "/helper_mod.py", - prefix + "/main_prog.py", - ]) - env.expect.that_str(target[DefaultInfo].files_to_run.executable.short_path).equals( - prefix + "/main_prog.py", - ) - info = target[PyInterpreterProgramInfo] - env.expect.that_dict(info.env).contains_exactly({ - "PYTHONDONTWRITEBYTECODE": "1", - }) - env.expect.that_collection(info.interpreter_args).contains_exactly(["-B"]) - -def _test_duplicate_basename_fails(name): - rt_util.helper_target( - py_interpreter_program, - name = name + "_prog", - main = "main_prog.py", - srcs = ["sub/main_prog.py"], - ) - analysis_test( - name = name, - impl = _test_duplicate_basename_fails_impl, - target = name + "_prog", - expect_failure = True, - ) - -_tests.append(_test_duplicate_basename_fails) - -def _test_duplicate_basename_fails_impl(env, target): - env.expect.that_target(target).failures().contains_predicate( - matching.contains("duplicate basename 'main_prog.py'"), - ) - -def py_interpreter_program_test_suite(name): - test_suite( - name = name, - tests = _tests, - ) diff --git a/tests/base_rules/py_interpreter_program/run_prog_test.py b/tests/base_rules/py_interpreter_program/run_prog_test.py deleted file mode 100644 index 02e7003c88..0000000000 --- a/tests/base_rules/py_interpreter_program/run_prog_test.py +++ /dev/null @@ -1,19 +0,0 @@ -"""Verifies execution of a py_interpreter_program target with sibling srcs.""" - -from __future__ import annotations - -import os -import unittest -from pathlib import Path - - -class RunInterpreterProgramTest(unittest.TestCase): - def test_sibling_import_and_no_bytecode(self) -> None: - out_path = Path(os.environ["PROG_OUTPUT"]) - self.assertEqual( - out_path.read_text(encoding="utf-8").strip(), "ok-from-sibling" - ) - - -if __name__ == "__main__": - unittest.main() diff --git a/tests/base_rules/py_interpreter_program/sub/main_prog.py b/tests/base_rules/py_interpreter_program/sub/main_prog.py deleted file mode 100644 index 272b3c9185..0000000000 --- a/tests/base_rules/py_interpreter_program/sub/main_prog.py +++ /dev/null @@ -1 +0,0 @@ -"""Duplicate basename fixture for py_interpreter_program negative test.""" diff --git a/tests/venv_site_packages_libs/app_files_building/app_files_building_tests.bzl b/tests/venv_site_packages_libs/app_files_building/app_files_building_tests.bzl index 097381721b..c514fb597d 100644 --- a/tests/venv_site_packages_libs/app_files_building/app_files_building_tests.bzl +++ b/tests/venv_site_packages_libs/app_files_building/app_files_building_tests.bzl @@ -938,7 +938,8 @@ def _test_default_mode_directory_merge_impl(env, _): # Direct-link case: a directory File that is itself the top-level venv entry # must remain the File object (not a path string) after conflict merging so - # default mode materializes a runfiles root_symlink rather than UnresolvedSymlink. + # default mode materializes a runfiles root_symlink rather than + # ctx.actions.declare_symlink(). direct_dir = _file("pkg/site-packages/foo", is_directory = True) direct_entries = get_venv_symlinks( test_ctx, @@ -1005,11 +1006,9 @@ tree_venv_lib = rule( }, ) -_tree_venv_lib = tree_venv_lib - def _test_tree_artifact_rule_merging(name): rt_util.helper_target( - _tree_venv_lib, + tree_venv_lib, name = name + "_tree_a", package = "tree_a", paths = [ @@ -1024,7 +1023,7 @@ def _test_tree_artifact_rule_merging(name): namespace_package_files = ["site-packages/pkgutil_ns/__init__.py"], ) rt_util.helper_target( - _tree_venv_lib, + tree_venv_lib, name = name + "_tree_b", package = "tree_b", paths = [ @@ -1053,7 +1052,10 @@ _tests.append(_test_tree_artifact_rule_merging) def _test_tree_artifact_rule_merging_impl(env, target): entries = target[PyInfo].venv_symlinks.to_list() - all_dirs = [f.is_directory for e in entries for f in e.files.to_list()] + all_dirs = [] + for e in entries: + for f in e.files.to_list(): + all_dirs.append(f.is_directory) env.expect.that_bool(all(all_dirs) and len(all_dirs) > 0).equals(True) test_ctx = _ctx(workspace_name = env.ctx.workspace_name) From 544d5d625abb186244896bd858c595e3470b6389 Mon Sep 17 00:00:00 2001 From: Jeremy Volkman Date: Mon, 2 Nov 2026 06:17:31 +0000 Subject: [PATCH 3/7] docs: clarify venv_runfiles keep_map types --- python/private/venv_runfiles.bzl | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/python/private/venv_runfiles.bzl b/python/private/venv_runfiles.bzl index 0f42b0e7bb..86b3ce3354 100644 --- a/python/private/venv_runfiles.bzl +++ b/python/private/venv_runfiles.bzl @@ -138,10 +138,11 @@ def build_link_map(ctx, entries, return_conflicts = False): to be resolved and merged. Returns: - {type}`dict[str, dict[str, File | str]]` Mappings of venv paths to their - backing files. The first key is a `VenvSymlinkKind` value. - The inner dict keys are venv paths relative to the kind's directory. The - inner dict values are `File`s or `rf_root_path` strings to link to. + {type}`dict[str kind, dict[str venv_path, File | str rf_root_path]]` + Mappings of venv paths to their backing files. The first key is a + `VenvSymlinkKind` value. The inner dict keys are venv paths relative to + the kind's directory. The inner dict values are `File`s or + runfiles-root-relative path strings to link to. """ version_by_pkg = {} # dict[str pkg, str version] @@ -235,8 +236,9 @@ def _merge_venv_path_group(ctx, group, keep_map): ctx: {type}`ctx` current ctx. group: {type}`list[VenvSymlinkEntry]` a group of entries with overlapping `venv_path` prefixes, ordered from shortest to longest path. - keep_map: {type}`dict[str, File | str]` (`dict[str venv_path, File | str rf_root_path]`) - files kept after merging are populated into this map. + keep_map: {type}`dict[str venv_path, File | str rf_root_path]` files + kept after merging are populated into this map. Values are `File`s, + or runfiles-root-relative path strings to link to. """ # TODO: Compute the minimum number of entries to create. This can't avoid From 0956f1a5b4d8e475dee36ede6619c2f4c8ccdc48 Mon Sep 17 00:00:00 2001 From: Jeremy Volkman Date: Mon, 2 Nov 2026 19:49:24 +0000 Subject: [PATCH 4/7] refactor: clarify venv merge cases and doc types --- python/private/py_info.bzl | 5 ++ python/private/venv_runfiles.bzl | 122 +++++++++++++++++-------------- 2 files changed, 72 insertions(+), 55 deletions(-) diff --git a/python/private/py_info.bzl b/python/private/py_info.bzl index 6c0f3a17dd..25293c61ad 100644 --- a/python/private/py_info.bzl +++ b/python/private/py_info.bzl @@ -119,6 +119,11 @@ directory artifacts (`File.is_directory`) and `link_to_path` is a directory. When non-empty, conflict merging uses these paths instead of `files` (and `files` then only supplies runfiles). +This is for entries whose target isn't a single `File`, so it's mutually +exclusive with `link_to_file`: `link_to_file` is only used when an entry +doesn't conflict, whereas `link_to_subpaths` is only used during conflict +merging. + :::{versionadded} VERSION_NEXT_FEATURE ::: """, diff --git a/python/private/venv_runfiles.bzl b/python/private/venv_runfiles.bzl index 86b3ce3354..b374a6544f 100644 --- a/python/private/venv_runfiles.bzl +++ b/python/private/venv_runfiles.bzl @@ -138,10 +138,9 @@ def build_link_map(ctx, entries, return_conflicts = False): to be resolved and merged. Returns: - {type}`dict[str kind, dict[str venv_path, File | str rf_root_path]]` - Mappings of venv paths to their backing files. The first key is a - `VenvSymlinkKind` value. The inner dict keys are venv paths relative to - the kind's directory. The inner dict values are `File`s or + {type}`dict[str, dict[str, File | str]]` Mappings of venv paths to their + backing files, keyed by `VenvSymlinkKind` value, then by venv path + relative to the kind's directory. Values are `File`s or runfiles-root-relative path strings to link to. """ @@ -236,9 +235,9 @@ def _merge_venv_path_group(ctx, group, keep_map): ctx: {type}`ctx` current ctx. group: {type}`list[VenvSymlinkEntry]` a group of entries with overlapping `venv_path` prefixes, ordered from shortest to longest path. - keep_map: {type}`dict[str venv_path, File | str rf_root_path]` files - kept after merging are populated into this map. Values are `File`s, - or runfiles-root-relative path strings to link to. + keep_map: {type}`dict[str, File | str]` files kept after merging are + populated into this map, keyed by venv path. Values are `File`s or + runfiles-root-relative path strings to link to. """ # TODO: Compute the minimum number of entries to create. This can't avoid @@ -262,20 +261,13 @@ def _merge_venv_path_group(ctx, group, keep_map): anchored_link_to_path = entry.link_to_path + "/" for file in entry.files.to_list(): rf_root_path = runfiles_root_path(ctx, file.short_path) - is_dir = file.is_directory # It's a file (or directory) being directly linked and # must be directly linked. - # TreeArtifact-backed single-file entry: files holds the tree root - # (an ancestor of link_to_path). - is_direct_link = rf_root_path == entry.link_to_path - is_tree_backed_file = is_dir and entry.link_to_path.startswith(rf_root_path + "/") - if is_direct_link or is_tree_backed_file: + if rf_root_path == entry.link_to_path: # For lack of a better option, first added wins. if entry.venv_path not in keep_map: - keep_map[entry.venv_path] = ( - file if is_direct_link else entry.link_to_path - ) + keep_map[entry.venv_path] = file # Skip anything remaining: anything left is either # the same path (first set wins), a suffix (violates @@ -283,6 +275,15 @@ def _merge_venv_path_group(ctx, group, keep_map): # the prefix (violates preconditions). break + # A directory artifact that contains link_to_path, e.g. the + # installed wheel tree backing a single-file entry. There's no + # File for the target itself, so link to its runfiles path. + if file.is_directory and entry.link_to_path.startswith(rf_root_path + "/"): + # For lack of a better option, first added wins. + if entry.venv_path not in keep_map: + keep_map[entry.venv_path] = entry.link_to_path + break + # Compute the file-specific venv path. i.e. the relative # path of the file under entry.venv_path, joined with # entry.venv_path @@ -460,10 +461,11 @@ def get_venv_symlinks( namespace_package_dirs[ns_dir] = True cannot_be_linked_directly[ns_dir] = True - # dict[str venv_path, VenvSymlinkEntry] + # dict[str path, VenvSymlinkEntry] + # Where path is the venv path (i.e. relative to site_packages_prefix) venv_symlinks = {} - # list[struct(venv_path, rf_root_path, file)] + # List of candidate structs; see _venv_candidates. candidates_left_to_link = [] # We want to minimize the number of files symlinked. Ideally, only the @@ -530,76 +532,86 @@ def get_venv_symlinks( # At this point, venv_symlinks has entries for the shared libraries # and cannot_be_linked_directly has the directories that cannot be - # directly linked. Next, we loop over the remaining candidates and group + # directly linked. Next, we loop over the remaining files and group # them into the highest level directory that can be linked. - # dict[str group_venv_path, list[struct(venv_path, rf_root_path, file)]] - groups = {} + # dict[str venv_path, list[candidate struct]] + optimized_groups = {} for candidate in candidates_left_to_link: venv_path = candidate.venv_path parent = paths.dirname(venv_path) - if not parent or parent in cannot_be_linked_directly: - # File in root or in a directory that cannot be directly linked, - # so link the file directly. - groups.setdefault(venv_path, []).append(candidate) + if not parent: + # File in root, must be linked directly + optimized_groups.setdefault(venv_path, []) + optimized_groups[venv_path].append(candidate) continue - # This path can be grouped. Find the highest-level directory to link. - group_venv_path = parent - next_parent = paths.dirname(parent) - for _ in range(len(group_venv_path) + 1): # Iterate enough times - if not next_parent or next_parent in cannot_be_linked_directly: - break - group_venv_path = next_parent - next_parent = paths.dirname(next_parent) + if parent in cannot_be_linked_directly: + # File in a directory that cannot be directly linked, + # so link the file directly + optimized_groups.setdefault(venv_path, []) + optimized_groups[venv_path].append(candidate) + else: + # This path can be grouped. Find the highest-level directory to link. + venv_path = parent + next_parent = paths.dirname(parent) + for _ in range(len(venv_path) + 1): # Iterate enough times + if next_parent: + if next_parent not in cannot_be_linked_directly: + venv_path = next_parent + next_parent = paths.dirname(next_parent) + else: + break + else: + break - groups.setdefault(group_venv_path, []).append(candidate) + optimized_groups.setdefault(venv_path, []) + optimized_groups[venv_path].append(candidate) # Finally, for each group, we create the VenvSymlinkEntry objects - for group_venv_path, members in groups.items(): - if group_venv_path.startswith("data/"): - out_venv_path = group_venv_path[len("data/"):] + for venv_path, members in optimized_groups.items(): + if venv_path.startswith("data/"): + out_venv_path = venv_path[len("data/"):] kind = VenvSymlinkKind.DATA prefix = "" - elif group_venv_path.startswith("include/"): - out_venv_path = group_venv_path[len("include/"):] + elif venv_path.startswith("include/"): + out_venv_path = venv_path[len("include/"):] kind = VenvSymlinkKind.INCLUDE prefix = "" - elif group_venv_path.startswith("bin/"): - out_venv_path = group_venv_path[len("bin/"):] + elif venv_path.startswith("bin/"): + out_venv_path = venv_path[len("bin/"):] kind = VenvSymlinkKind.BIN prefix = "" else: - out_venv_path = group_venv_path + out_venv_path = venv_path kind = VenvSymlinkKind.LIB prefix = site_packages_root - is_file_link = members[0].venv_path == group_venv_path if tree_artifact != None: - link_to_path = tree_rf_root + "/" + prefix + group_venv_path + link_to_path = tree_rf_root + "/" + prefix + venv_path link_to_file = None entry_files = tree_files_depset - link_to_subpaths = depset( - [] if is_file_link else [ - m.venv_path[len(group_venv_path) + 1:] - for m in members - ], - ) + + # A group keyed by its member's own path links that single file + # directly, so it has no subpaths. + is_file_link = members[0].venv_path == venv_path + link_to_subpaths = depset([] if is_file_link else [ + m.venv_path[len(venv_path) + 1:] + for m in members + ]) else: link_to_path = ( _get_label_runfiles_repo(ctx, members[0].file.owner) + "/" + prefix + - group_venv_path - ) - link_to_file = ( - members[0].file if kind == VenvSymlinkKind.BIN and len(members) == 1 else None + venv_path ) + link_to_file = members[0].file if kind == VenvSymlinkKind.BIN and len(members) == 1 else None entry_files = depset([m.file for m in members]) link_to_subpaths = None - venv_symlinks[group_venv_path] = VenvSymlinkEntry( + venv_symlinks[venv_path] = VenvSymlinkEntry( kind = kind, link_to_path = link_to_path, link_to_file = link_to_file, From a6d4ef5c17ea2a3d2809c74049f526ab9461bffe Mon Sep 17 00:00:00 2001 From: Jeremy Volkman Date: Wed, 11 Nov 2026 00:35:24 +0000 Subject: [PATCH 5/7] test: exercise shared libraries in tree-backed venv entries Simplify the `files` expression on shared-library `VenvSymlinkEntry` records in `get_venv_symlinks`, document `tree_venv_lib` and `_assert_tree_artifact_equivalence`, and add `tree_shared_lib_loading_test` to verify loading compiled C extensions and transitive shared libraries from a `TreeArtifact`-backed venv at runtime. --- python/private/venv_runfiles.bzl | 2 +- .../app_files_building/BUILD.bazel | 29 +++ .../app_files_building_tests.bzl | 78 ++++++- .../app_files_building/populate_tree.py | 22 +- .../tree_shared_lib_loading_test.py | 216 ++++++++++++++++++ 5 files changed, 337 insertions(+), 10 deletions(-) create mode 100644 tests/venv_site_packages_libs/app_files_building/tree_shared_lib_loading_test.py diff --git a/python/private/venv_runfiles.bzl b/python/private/venv_runfiles.bzl index b374a6544f..d3dfcbe2d5 100644 --- a/python/private/venv_runfiles.bzl +++ b/python/private/venv_runfiles.bzl @@ -489,7 +489,7 @@ def get_venv_symlinks( link_to_file = candidate.file, package = package, version = version_str, - files = tree_files_depset if tree_artifact != None else depset([candidate.file]), + files = tree_files_depset or depset([candidate.file]), venv_path = venv_path, ) parent = paths.dirname(venv_path) diff --git a/tests/venv_site_packages_libs/app_files_building/BUILD.bazel b/tests/venv_site_packages_libs/app_files_building/BUILD.bazel index 1a0b470ff6..0daaae87ed 100644 --- a/tests/venv_site_packages_libs/app_files_building/BUILD.bazel +++ b/tests/venv_site_packages_libs/app_files_building/BUILD.bazel @@ -1,5 +1,6 @@ load("//python:py_binary.bzl", "py_binary") load("//tests/support:py_reconfig.bzl", "py_reconfig_test") +load("//tests/support:support.bzl", "NOT_WINDOWS") load(":app_files_building_tests.bzl", "app_files_building_test_suite", "tree_venv_lib") py_binary( @@ -55,3 +56,31 @@ py_reconfig_test( ":tree_venv_runtime_b", ], ) + +tree_venv_lib( + name = "tree_ext_with_libs", + contents = { + "site-packages/tree_ext_with_libs/__init__.py": "", + }, + files = { + "//tests/venv_site_packages_libs/ext_with_libs:adder": "site-packages/tree_ext_with_libs", + "//tests/venv_site_packages_libs/ext_with_libs:increment": "site-packages/tree_ext_with_libs/libs", + }, + package = "tree-ext-with-libs", + tags = ["manual"], + target_compatible_with = NOT_WINDOWS, +) + +py_reconfig_test( + name = "tree_shared_lib_loading_test", + srcs = ["tree_shared_lib_loading_test.py"], + bootstrap_impl = "script", + main = "tree_shared_lib_loading_test.py", + target_compatible_with = NOT_WINDOWS, + venvs_site_packages = "yes", + deps = [ + ":tree_ext_with_libs", + "@dev_pip//macholib", + "@dev_pip//pyelftools", + ], +) diff --git a/tests/venv_site_packages_libs/app_files_building/app_files_building_tests.bzl b/tests/venv_site_packages_libs/app_files_building/app_files_building_tests.bzl index c514fb597d..f3200c4f2d 100644 --- a/tests/venv_site_packages_libs/app_files_building/app_files_building_tests.bzl +++ b/tests/venv_site_packages_libs/app_files_building/app_files_building_tests.bzl @@ -87,6 +87,25 @@ def _assert_tree_artifact_equivalence( package, version_str, namespace_package_files = []): + """Verifies at analysis time that tree-mode matches file-mode venv entries. + + Checks using fake `File` structs that `get_venv_symlinks(tree_artifact = ...)` + and `build_link_map` produce entries and conflict-merged link maps equivalent + to file mode for the same package layout. This is an analysis-only check and + does not load modules or shared libraries at runtime; see + `//tests/venv_site_packages_libs/app_files_building:tree_shared_lib_loading_test` + (and `:tree_venv_merge_test`) for runtime verification. + + Args: + env: `rules_testing` analysis test environment. + test_ctx: Fake rule context struct from `_ctx()`. + files: Fake `File` structs passed to file-mode `get_venv_symlinks`. + file_entries: Expected `VenvSymlinkEntry` list produced in file mode. + package: Distribution name. + version_str: Distribution version string. + namespace_package_files: Optional fake `File` structs for pkgutil-style + namespace package `__init__.py` files. + """ pkg_prefix = env.ctx.label.package + "/" tree_artifact = _file(pkg_prefix + "srcs", is_directory = True) tree_files = [f.short_path[len(pkg_prefix):] for f in files] @@ -957,6 +976,7 @@ def _test_default_mode_directory_merge_impl(env, _): }) def _tree_venv_lib_impl(ctx): + """Populates a directory artifact and returns tree-backed `PyInfo.venv_symlinks`.""" out = ctx.actions.declare_directory(ctx.label.name + "_srcs") args = ctx.actions.args() args.add_all("--output-dir", [out], expand_directories = False) @@ -964,12 +984,30 @@ def _tree_venv_lib_impl(ctx): args.add("--path", p) for rel_path, content in ctx.attr.contents.items(): args.add("--entry", "{}={}".format(rel_path, content)) + + inputs = [] + copied_paths = [] + for src_target, dest_dir in ctx.attr.files.items(): + clean_dir = dest_dir.strip("/") + for src_file in src_target[DefaultInfo].files.to_list(): + if clean_dir: + rel_path = "{}/{}".format(clean_dir, src_file.basename) + else: + rel_path = src_file.basename + args.add("--copy", "{}={}".format(rel_path, src_file.path)) + inputs.append(src_file) + copied_paths.append(rel_path) + ctx.actions.run( executable = ctx.executable._populate_tree, + inputs = inputs, arguments = [args], outputs = [out], ) - all_paths = sorted({p: True for p in ctx.attr.paths + ctx.attr.contents.keys()}.keys()) + all_paths = sorted({ + p: True + for p in ctx.attr.paths + ctx.attr.contents.keys() + copied_paths + }.keys()) entries = get_venv_symlinks( ctx, all_paths, @@ -992,12 +1030,40 @@ def _tree_venv_lib_impl(ctx): tree_venv_lib = rule( implementation = _tree_venv_lib_impl, + doc = """ +Test-only rule that writes the given paths, contents, and files into a +directory artifact (`TreeArtifact`) and returns a `PyInfo` whose +`venv_symlinks` are tree-backed via `get_venv_symlinks(tree_artifact = ...)`, +standing in for a target that installs a package into a directory artifact at +build time. +""", attrs = { - "contents": attr.string_dict(), - "namespace_package_files": attr.string_list(), - "package": attr.string(mandatory = True), - "paths": attr.string_list(), - "version": attr.string(default = "1.0"), + "contents": attr.string_dict( + doc = "Mapping of tree-relative file paths to UTF-8 text contents.", + ), + "files": attr.label_keyed_string_dict( + allow_files = True, + doc = """ +Mapping of source file targets to destination directories inside the tree +artifact; each file's basename is preserved. +""", + ), + "namespace_package_files": attr.string_list( + doc = """ +Tree-relative paths to pkgutil-style namespace `__init__.py` files. +""", + ), + "package": attr.string( + doc = "Python distribution name passed to `get_venv_symlinks`.", + mandatory = True, + ), + "paths": attr.string_list( + doc = "Tree-relative paths to create as empty files.", + ), + "version": attr.string( + default = "1.0", + doc = "Python distribution version passed to `get_venv_symlinks`.", + ), "_populate_tree": attr.label( default = ":populate_tree", executable = True, diff --git a/tests/venv_site_packages_libs/app_files_building/populate_tree.py b/tests/venv_site_packages_libs/app_files_building/populate_tree.py index de9482135c..8879f61539 100644 --- a/tests/venv_site_packages_libs/app_files_building/populate_tree.py +++ b/tests/venv_site_packages_libs/app_files_building/populate_tree.py @@ -1,15 +1,19 @@ -"""Populates a declared directory artifact with relative file paths and contents. +"""Populates a declared directory artifact with paths, text entries, and files. Usage: - populate_tree.py --output-dir [--path ...] [--entry = ...] + populate_tree.py --output-dir [--path ...] + [--entry = ...] [--copy = ...] Input format: - `--path` creates an empty file at `/`; `--entry` writes UTF-8 text. + `--path` creates an empty file at `/`; `--entry` writes UTF-8 + text; `--copy` copies `` to `/`. """ from __future__ import annotations import argparse +import os +import shutil from collections.abc import Sequence from pathlib import Path @@ -19,6 +23,7 @@ def main(argv: Sequence[str] | None = None) -> None: parser.add_argument("--output-dir", type=Path, required=True) parser.add_argument("--path", action="append", dest="paths", default=[]) parser.add_argument("--entry", action="append", dest="entries", default=[]) + parser.add_argument("--copy", action="append", dest="copies", default=[]) args = parser.parse_args(argv) args.output_dir.mkdir(parents=True, exist_ok=True) @@ -33,6 +38,17 @@ def main(argv: Sequence[str] | None = None) -> None: with open(dest, "w", encoding="utf-8", newline="\n") as out_file: out_file.write(text) + for item in args.copies: + rel_path, _, src_str = item.partition("=") + src_path = Path(src_str) + dest = args.output_dir / rel_path + dest.parent.mkdir(parents=True, exist_ok=True) + if dest.exists(): + dest.unlink() + shutil.copyfile(src_path, dest) + if os.access(src_path, os.X_OK): + dest.chmod(0o755) + if __name__ == "__main__": main() diff --git a/tests/venv_site_packages_libs/app_files_building/tree_shared_lib_loading_test.py b/tests/venv_site_packages_libs/app_files_building/tree_shared_lib_loading_test.py new file mode 100644 index 0000000000..bc5a37df01 --- /dev/null +++ b/tests/venv_site_packages_libs/app_files_building/tree_shared_lib_loading_test.py @@ -0,0 +1,216 @@ +"""Runtime test loading C extensions and shared libraries from a tree-backed venv.""" + +from __future__ import annotations + +import ctypes +import importlib +import importlib.util +import os +import sys +import unittest +from pathlib import Path + +# Platform-conditional binary header parsers (pyelftools on Linux, macholib on macOS). +if os.name == "posix" and sys.platform != "darwin": + from elftools.elf.elffile import ELFFile # pyrefly: ignore[missing-import] +else: + ELFFile = None + +if sys.platform == "darwin": + from macholib import mach_o # pyrefly: ignore[missing-import] + from macholib.MachO import MachO # pyrefly: ignore[missing-import] +else: + mach_o = None + MachO = None + +_ELF_MAGIC = b"\x7fELF" +_MACHO_MAGICS = ( + b"\xce\xfa\xed\xfe", # 32-bit big-endian + b"\xcf\xfa\xed\xfe", # 64-bit big-endian + b"\xfe\xed\xfa\xce", # 32-bit little-endian + b"\xfe\xed\xfa\xcf", # 64-bit little-endian +) + + +class TreeSharedLibLoadingTest(unittest.TestCase): + def setUp(self) -> None: + super().setUp() + if sys.prefix == sys.base_prefix: + raise AssertionError("Not running under a venv") + self.venv = Path(sys.prefix) + + def test_tree_backed_shared_library_linking(self) -> None: + try: + adder = importlib.import_module("tree_ext_with_libs.adder") + except ImportError as err: + spec = importlib.util.find_spec("tree_ext_with_libs.adder") + if not spec or not spec.origin: + self.fail(f"Import failed and could not find module spec: {err}") + + info = self._get_linking_info(spec.origin) + self.fail( + f"Failed to import tree_ext_with_libs.adder.\n" + f"Original error: {err}\n" + f"Linking info for {spec.origin}:\n" + f" RPATHs: {info.get('rpaths', 'N/A')}\n" + f" Needed libs: {info.get('needed', 'N/A')}" + ) + + self.assertIsNotNone(adder.__file__) + assert adder.__file__ is not None # type assert + self.assertIn(".venv/", adder.__file__) + + adder_venv_path = Path(adder.__file__) + self.assertTrue( + str(adder_venv_path).startswith(str(self.venv)), + f"Extension {adder_venv_path} was not loaded from venv {self.venv}", + ) + + # Shared libraries must be linked directly as file symlinks rather than + # via a directory symlink on the enclosing package or `libs/` directory. + pkg_dir = adder_venv_path.parent + libs_dir = pkg_dir / "libs" + libincrement_name = ( + "libincrement.dylib" if sys.platform == "darwin" else "libincrement.so" + ) + increment_venv_path = libs_dir / libincrement_name + + self.assertTrue(pkg_dir.is_dir()) + self.assertFalse( + pkg_dir.is_symlink(), + f"Expected {pkg_dir} to be a real directory, not a symlink", + ) + self.assertTrue(libs_dir.is_dir()) + self.assertFalse( + libs_dir.is_symlink(), + f"Expected {libs_dir} to be a real directory, not a symlink", + ) + self.assertTrue( + adder_venv_path.is_symlink(), + f"Expected {adder_venv_path} to be a direct file symlink", + ) + self.assertTrue( + increment_venv_path.is_symlink(), + f"Expected {increment_venv_path} to be a direct file symlink", + ) + + adder_real = adder_venv_path.resolve() + increment_real = increment_venv_path.resolve() + self.assertEqual(adder_real.parent / "libs" / libincrement_name, increment_real) + self.assertIn("tree_ext_with_libs_srcs/", adder_real.as_posix()) + self.assertIn("tree_ext_with_libs_srcs/", increment_real.as_posix()) + + with open(adder_real, "rb") as f: + magic_bytes = f.read(4) + + if magic_bytes == _ELF_MAGIC: + self._assert_elf_linking(str(adder_real)) + elif magic_bytes in _MACHO_MAGICS: + self._assert_macho_linking(str(adder_real)) + else: + self.fail(f"Unsupported file format for adder: magic bytes {magic_bytes!r}") + + self.assertEqual(adder.do_add(), 2) + + loaded_libs = self._get_loaded_shared_libs() + self.assertIn(adder_real, loaded_libs) + self.assertIn(increment_real, loaded_libs) + + def _get_loaded_shared_libs(self) -> set[Path]: + if sys.platform == "darwin": + libsystem = ctypes.CDLL(None) + dyld_image_count = libsystem._dyld_image_count + dyld_image_count.argtypes = [] + dyld_image_count.restype = ctypes.c_uint32 + dyld_get_image_name = libsystem._dyld_get_image_name + dyld_get_image_name.argtypes = [ctypes.c_uint32] + dyld_get_image_name.restype = ctypes.c_char_p + loaded: set[Path] = set() + for idx in range(dyld_image_count()): + raw_name = dyld_get_image_name(idx) + if raw_name is not None: + loaded.add(Path(os.fsdecode(raw_name)).resolve()) + return loaded + + loaded = set() + with open("/proc/self/maps", encoding="utf-8") as maps_file: + for line in maps_file: + parts = line.split(maxsplit=5) + if len(parts) == 6 and parts[5].startswith("/"): + mapped = parts[5].strip() + if not mapped.endswith(" (deleted)"): + loaded.add(Path(mapped).resolve()) + return loaded + + def _get_linking_info(self, path: str) -> dict[str, list[str]]: + real_path = os.path.realpath(path) + with open(real_path, "rb") as f: + magic_bytes = f.read(4) + + if magic_bytes == _ELF_MAGIC: + return self._get_elf_info(real_path) + if magic_bytes in _MACHO_MAGICS: + return self._get_macho_info(real_path) + return {} + + def _get_elf_info(self, path: str) -> dict[str, list[str]]: + assert ELFFile is not None # type assert + info: dict[str, list[str]] = { + "rpaths": [], + "needed": [], + "undefined_symbols": [], + } + with open(path, "rb") as f: + elf = ELFFile(f) + dynamic = elf.get_section_by_name(".dynamic") + if dynamic: + for tag in dynamic.iter_tags(): + # pyelftools generates tag attributes dynamically by d_tag kind. + if tag.entry.d_tag == "DT_NEEDED": # pyrefly: ignore[missing-attribute] + info["needed"].append(tag.needed) # pyrefly: ignore[missing-attribute] + elif tag.entry.d_tag == "DT_RPATH": # pyrefly: ignore[missing-attribute] + info["rpaths"].append(tag.rpath) # pyrefly: ignore[missing-attribute] + elif tag.entry.d_tag == "DT_RUNPATH": # pyrefly: ignore[missing-attribute] + info["rpaths"].append(tag.runpath) # pyrefly: ignore[missing-attribute] + + dynsym = elf.get_section_by_name(".dynsym") + if dynsym: + info["undefined_symbols"] = [ + s.name + for s in dynsym.iter_symbols() + if s.entry["st_shndx"] == "SHN_UNDEF" # pyrefly: ignore[missing-attribute] + ] + return info + + def _get_macho_info(self, path: str) -> dict[str, list[str]]: + assert MachO is not None and mach_o is not None # type assert + info: dict[str, list[str]] = {"rpaths": [], "needed": []} + macho = MachO(path) + for header in macho.headers: + for cmd_load, _, data in header.commands: + if cmd_load.cmd == mach_o.LC_LOAD_DYLIB: + info["needed"].append(data.decode().strip("\x00")) + elif cmd_load.cmd == mach_o.LC_RPATH: + info["rpaths"].append(data.decode().strip("\x00")) + return info + + def _assert_elf_linking(self, path: str) -> None: + info = self._get_elf_info(path) + self.assertIn("libincrement.so", info["needed"]) + self.assertIn("increment", info["undefined_symbols"]) + self.assertTrue( + any("$ORIGIN/libs" in rpath for rpath in info["rpaths"]), + f"Expected $ORIGIN/libs in ELF rpaths, got {info['rpaths']}", + ) + + def _assert_macho_linking(self, path: str) -> None: + info = self._get_macho_info(path) + self.assertIn("@rpath/libincrement.dylib", info["needed"]) + self.assertTrue( + any("@loader_path/libs" in rpath for rpath in info["rpaths"]), + f"Expected @loader_path/libs in Mach-O rpaths, got {info['rpaths']}", + ) + + +if __name__ == "__main__": + unittest.main() From 9a5fe5a18a5ab091270f7117667dcc97ae606fcf Mon Sep 17 00:00:00 2001 From: Jeremy Volkman Date: Wed, 11 Nov 2026 06:19:48 +0000 Subject: [PATCH 6/7] test: migrate tree-backed venv runtime tests to pytest_test Switch `tree_venv_merge_test` and `tree_shared_lib_loading_test` from `unittest.TestCase` + `py_reconfig_test` to `pytest_test` with plain `pytest` test functions and assertions. --- .../app_files_building/BUILD.bazel | 27 +- .../tree_shared_lib_loading_test.py | 349 +++++++++--------- .../tree_venv_merge_test.py | 64 ++-- 3 files changed, 215 insertions(+), 225 deletions(-) diff --git a/tests/venv_site_packages_libs/app_files_building/BUILD.bazel b/tests/venv_site_packages_libs/app_files_building/BUILD.bazel index 0daaae87ed..60a5d37b09 100644 --- a/tests/venv_site_packages_libs/app_files_building/BUILD.bazel +++ b/tests/venv_site_packages_libs/app_files_building/BUILD.bazel @@ -1,5 +1,5 @@ load("//python:py_binary.bzl", "py_binary") -load("//tests/support:py_reconfig.bzl", "py_reconfig_test") +load("//python:pytest_test.bzl", "pytest_test") load("//tests/support:support.bzl", "NOT_WINDOWS") load(":app_files_building_tests.bzl", "app_files_building_test_suite", "tree_venv_lib") @@ -42,15 +42,19 @@ tree_venv_lib( package = "tree-b", ) -py_reconfig_test( +pytest_test( name = "tree_venv_merge_test", srcs = ["tree_venv_merge_test.py"], - bootstrap_impl = select({ - "@platforms//os:windows": "system_python", - "//conditions:default": "script", + config_settings = select({ + "@platforms//os:windows": { + "//python/config_settings:bootstrap_impl": "system_python", + "//python/config_settings:venvs_site_packages": "yes", + }, + "//conditions:default": { + "//python/config_settings:bootstrap_impl": "script", + "//python/config_settings:venvs_site_packages": "yes", + }, }), - main = "tree_venv_merge_test.py", - venvs_site_packages = "yes", deps = [ ":tree_venv_runtime_a", ":tree_venv_runtime_b", @@ -71,13 +75,14 @@ tree_venv_lib( target_compatible_with = NOT_WINDOWS, ) -py_reconfig_test( +pytest_test( name = "tree_shared_lib_loading_test", srcs = ["tree_shared_lib_loading_test.py"], - bootstrap_impl = "script", - main = "tree_shared_lib_loading_test.py", + config_settings = { + "//python/config_settings:bootstrap_impl": "script", + "//python/config_settings:venvs_site_packages": "yes", + }, target_compatible_with = NOT_WINDOWS, - venvs_site_packages = "yes", deps = [ ":tree_ext_with_libs", "@dev_pip//macholib", diff --git a/tests/venv_site_packages_libs/app_files_building/tree_shared_lib_loading_test.py b/tests/venv_site_packages_libs/app_files_building/tree_shared_lib_loading_test.py index bc5a37df01..d45bee8a92 100644 --- a/tests/venv_site_packages_libs/app_files_building/tree_shared_lib_loading_test.py +++ b/tests/venv_site_packages_libs/app_files_building/tree_shared_lib_loading_test.py @@ -7,7 +7,6 @@ import importlib.util import os import sys -import unittest from pathlib import Path # Platform-conditional binary header parsers (pyelftools on Linux, macholib on macOS). @@ -32,185 +31,179 @@ ) -class TreeSharedLibLoadingTest(unittest.TestCase): - def setUp(self) -> None: - super().setUp() - if sys.prefix == sys.base_prefix: - raise AssertionError("Not running under a venv") - self.venv = Path(sys.prefix) - - def test_tree_backed_shared_library_linking(self) -> None: - try: - adder = importlib.import_module("tree_ext_with_libs.adder") - except ImportError as err: - spec = importlib.util.find_spec("tree_ext_with_libs.adder") - if not spec or not spec.origin: - self.fail(f"Import failed and could not find module spec: {err}") - - info = self._get_linking_info(spec.origin) - self.fail( - f"Failed to import tree_ext_with_libs.adder.\n" - f"Original error: {err}\n" - f"Linking info for {spec.origin}:\n" - f" RPATHs: {info.get('rpaths', 'N/A')}\n" - f" Needed libs: {info.get('needed', 'N/A')}" - ) - - self.assertIsNotNone(adder.__file__) - assert adder.__file__ is not None # type assert - self.assertIn(".venv/", adder.__file__) - - adder_venv_path = Path(adder.__file__) - self.assertTrue( - str(adder_venv_path).startswith(str(self.venv)), - f"Extension {adder_venv_path} was not loaded from venv {self.venv}", - ) - - # Shared libraries must be linked directly as file symlinks rather than - # via a directory symlink on the enclosing package or `libs/` directory. - pkg_dir = adder_venv_path.parent - libs_dir = pkg_dir / "libs" - libincrement_name = ( - "libincrement.dylib" if sys.platform == "darwin" else "libincrement.so" - ) - increment_venv_path = libs_dir / libincrement_name - - self.assertTrue(pkg_dir.is_dir()) - self.assertFalse( - pkg_dir.is_symlink(), - f"Expected {pkg_dir} to be a real directory, not a symlink", - ) - self.assertTrue(libs_dir.is_dir()) - self.assertFalse( - libs_dir.is_symlink(), - f"Expected {libs_dir} to be a real directory, not a symlink", - ) - self.assertTrue( - adder_venv_path.is_symlink(), - f"Expected {adder_venv_path} to be a direct file symlink", - ) - self.assertTrue( - increment_venv_path.is_symlink(), - f"Expected {increment_venv_path} to be a direct file symlink", - ) - - adder_real = adder_venv_path.resolve() - increment_real = increment_venv_path.resolve() - self.assertEqual(adder_real.parent / "libs" / libincrement_name, increment_real) - self.assertIn("tree_ext_with_libs_srcs/", adder_real.as_posix()) - self.assertIn("tree_ext_with_libs_srcs/", increment_real.as_posix()) - - with open(adder_real, "rb") as f: - magic_bytes = f.read(4) - - if magic_bytes == _ELF_MAGIC: - self._assert_elf_linking(str(adder_real)) - elif magic_bytes in _MACHO_MAGICS: - self._assert_macho_linking(str(adder_real)) - else: - self.fail(f"Unsupported file format for adder: magic bytes {magic_bytes!r}") - - self.assertEqual(adder.do_add(), 2) - - loaded_libs = self._get_loaded_shared_libs() - self.assertIn(adder_real, loaded_libs) - self.assertIn(increment_real, loaded_libs) - - def _get_loaded_shared_libs(self) -> set[Path]: - if sys.platform == "darwin": - libsystem = ctypes.CDLL(None) - dyld_image_count = libsystem._dyld_image_count - dyld_image_count.argtypes = [] - dyld_image_count.restype = ctypes.c_uint32 - dyld_get_image_name = libsystem._dyld_get_image_name - dyld_get_image_name.argtypes = [ctypes.c_uint32] - dyld_get_image_name.restype = ctypes.c_char_p - loaded: set[Path] = set() - for idx in range(dyld_image_count()): - raw_name = dyld_get_image_name(idx) - if raw_name is not None: - loaded.add(Path(os.fsdecode(raw_name)).resolve()) - return loaded - - loaded = set() - with open("/proc/self/maps", encoding="utf-8") as maps_file: - for line in maps_file: - parts = line.split(maxsplit=5) - if len(parts) == 6 and parts[5].startswith("/"): - mapped = parts[5].strip() - if not mapped.endswith(" (deleted)"): - loaded.add(Path(mapped).resolve()) +def _get_loaded_shared_libs() -> set[Path]: + if sys.platform == "darwin": + libsystem = ctypes.CDLL(None) + dyld_image_count = libsystem._dyld_image_count + dyld_image_count.argtypes = [] + dyld_image_count.restype = ctypes.c_uint32 + dyld_get_image_name = libsystem._dyld_get_image_name + dyld_get_image_name.argtypes = [ctypes.c_uint32] + dyld_get_image_name.restype = ctypes.c_char_p + loaded: set[Path] = set() + for idx in range(dyld_image_count()): + raw_name = dyld_get_image_name(idx) + if raw_name is not None: + loaded.add(Path(os.fsdecode(raw_name)).resolve()) return loaded - def _get_linking_info(self, path: str) -> dict[str, list[str]]: - real_path = os.path.realpath(path) - with open(real_path, "rb") as f: - magic_bytes = f.read(4) - - if magic_bytes == _ELF_MAGIC: - return self._get_elf_info(real_path) - if magic_bytes in _MACHO_MAGICS: - return self._get_macho_info(real_path) - return {} - - def _get_elf_info(self, path: str) -> dict[str, list[str]]: - assert ELFFile is not None # type assert - info: dict[str, list[str]] = { - "rpaths": [], - "needed": [], - "undefined_symbols": [], - } - with open(path, "rb") as f: - elf = ELFFile(f) - dynamic = elf.get_section_by_name(".dynamic") - if dynamic: - for tag in dynamic.iter_tags(): - # pyelftools generates tag attributes dynamically by d_tag kind. - if tag.entry.d_tag == "DT_NEEDED": # pyrefly: ignore[missing-attribute] - info["needed"].append(tag.needed) # pyrefly: ignore[missing-attribute] - elif tag.entry.d_tag == "DT_RPATH": # pyrefly: ignore[missing-attribute] - info["rpaths"].append(tag.rpath) # pyrefly: ignore[missing-attribute] - elif tag.entry.d_tag == "DT_RUNPATH": # pyrefly: ignore[missing-attribute] - info["rpaths"].append(tag.runpath) # pyrefly: ignore[missing-attribute] - - dynsym = elf.get_section_by_name(".dynsym") - if dynsym: - info["undefined_symbols"] = [ - s.name - for s in dynsym.iter_symbols() - if s.entry["st_shndx"] == "SHN_UNDEF" # pyrefly: ignore[missing-attribute] - ] - return info - - def _get_macho_info(self, path: str) -> dict[str, list[str]]: - assert MachO is not None and mach_o is not None # type assert - info: dict[str, list[str]] = {"rpaths": [], "needed": []} - macho = MachO(path) - for header in macho.headers: - for cmd_load, _, data in header.commands: - if cmd_load.cmd == mach_o.LC_LOAD_DYLIB: - info["needed"].append(data.decode().strip("\x00")) - elif cmd_load.cmd == mach_o.LC_RPATH: - info["rpaths"].append(data.decode().strip("\x00")) - return info - - def _assert_elf_linking(self, path: str) -> None: - info = self._get_elf_info(path) - self.assertIn("libincrement.so", info["needed"]) - self.assertIn("increment", info["undefined_symbols"]) - self.assertTrue( - any("$ORIGIN/libs" in rpath for rpath in info["rpaths"]), - f"Expected $ORIGIN/libs in ELF rpaths, got {info['rpaths']}", - ) - - def _assert_macho_linking(self, path: str) -> None: - info = self._get_macho_info(path) - self.assertIn("@rpath/libincrement.dylib", info["needed"]) - self.assertTrue( - any("@loader_path/libs" in rpath for rpath in info["rpaths"]), - f"Expected @loader_path/libs in Mach-O rpaths, got {info['rpaths']}", + loaded = set() + with open("/proc/self/maps", encoding="utf-8") as maps_file: + for line in maps_file: + parts = line.split(maxsplit=5) + if len(parts) == 6 and parts[5].startswith("/"): + mapped = parts[5].strip() + if not mapped.endswith(" (deleted)"): + loaded.add(Path(mapped).resolve()) + return loaded + + +def _get_elf_info(path: str) -> dict[str, list[str]]: + assert ELFFile is not None # type assert + info: dict[str, list[str]] = { + "rpaths": [], + "needed": [], + "undefined_symbols": [], + } + with open(path, "rb") as f: + elf = ELFFile(f) + dynamic = elf.get_section_by_name(".dynamic") + if dynamic: + for tag in dynamic.iter_tags(): + # pyelftools generates tag attributes dynamically by d_tag kind. + if tag.entry.d_tag == "DT_NEEDED": # pyrefly: ignore[missing-attribute] + info["needed"].append(tag.needed) # pyrefly: ignore[missing-attribute] + elif tag.entry.d_tag == "DT_RPATH": # pyrefly: ignore[missing-attribute] + info["rpaths"].append(tag.rpath) # pyrefly: ignore[missing-attribute] + elif tag.entry.d_tag == "DT_RUNPATH": # pyrefly: ignore[missing-attribute] + info["rpaths"].append(tag.runpath) # pyrefly: ignore[missing-attribute] + + dynsym = elf.get_section_by_name(".dynsym") + if dynsym: + info["undefined_symbols"] = [ + s.name + for s in dynsym.iter_symbols() + if s.entry["st_shndx"] == "SHN_UNDEF" # pyrefly: ignore[missing-attribute] + ] + return info + + +def _get_macho_info(path: str) -> dict[str, list[str]]: + assert MachO is not None and mach_o is not None # type assert + info: dict[str, list[str]] = {"rpaths": [], "needed": []} + macho = MachO(path) + for header in macho.headers: + for cmd_load, _, data in header.commands: + if cmd_load.cmd == mach_o.LC_LOAD_DYLIB: + info["needed"].append(data.decode().strip("\x00")) + elif cmd_load.cmd == mach_o.LC_RPATH: + info["rpaths"].append(data.decode().strip("\x00")) + return info + + +def _get_linking_info(path: str) -> dict[str, list[str]]: + real_path = os.path.realpath(path) + with open(real_path, "rb") as f: + magic_bytes = f.read(4) + + if magic_bytes == _ELF_MAGIC: + return _get_elf_info(real_path) + if magic_bytes in _MACHO_MAGICS: + return _get_macho_info(real_path) + return {} + + +def _assert_elf_linking(path: str) -> None: + info = _get_elf_info(path) + assert "libincrement.so" in info["needed"] + assert "increment" in info["undefined_symbols"] + assert any("$ORIGIN/libs" in rpath for rpath in info["rpaths"]), ( + f"Expected $ORIGIN/libs in ELF rpaths, got {info['rpaths']}" + ) + + +def _assert_macho_linking(path: str) -> None: + info = _get_macho_info(path) + assert "@rpath/libincrement.dylib" in info["needed"] + assert any("@loader_path/libs" in rpath for rpath in info["rpaths"]), ( + f"Expected @loader_path/libs in Mach-O rpaths, got {info['rpaths']}" + ) + + +def test_tree_backed_shared_library_linking() -> None: + assert sys.prefix != sys.base_prefix, "Not running under a venv" + venv = Path(sys.prefix) + + try: + adder = importlib.import_module("tree_ext_with_libs.adder") + except ImportError as err: + spec = importlib.util.find_spec("tree_ext_with_libs.adder") + if not spec or not spec.origin: + raise AssertionError( + f"Import failed and could not find module spec: {err}" + ) from err + + info = _get_linking_info(spec.origin) + raise AssertionError( + f"Failed to import tree_ext_with_libs.adder.\n" + f"Original error: {err}\n" + f"Linking info for {spec.origin}:\n" + f" RPATHs: {info.get('rpaths', 'N/A')}\n" + f" Needed libs: {info.get('needed', 'N/A')}" + ) from err + + assert adder.__file__ is not None + assert ".venv/" in adder.__file__ + + adder_venv_path = Path(adder.__file__) + assert str(adder_venv_path).startswith(str(venv)), ( + f"Extension {adder_venv_path} was not loaded from venv {venv}" + ) + + # Shared libraries must be linked directly as file symlinks rather than + # via a directory symlink on the enclosing package or `libs/` directory. + pkg_dir = adder_venv_path.parent + libs_dir = pkg_dir / "libs" + libincrement_name = ( + "libincrement.dylib" if sys.platform == "darwin" else "libincrement.so" + ) + increment_venv_path = libs_dir / libincrement_name + + assert pkg_dir.is_dir() + assert not pkg_dir.is_symlink(), ( + f"Expected {pkg_dir} to be a real directory, not a symlink" + ) + assert libs_dir.is_dir() + assert not libs_dir.is_symlink(), ( + f"Expected {libs_dir} to be a real directory, not a symlink" + ) + assert adder_venv_path.is_symlink(), ( + f"Expected {adder_venv_path} to be a direct file symlink" + ) + assert increment_venv_path.is_symlink(), ( + f"Expected {increment_venv_path} to be a direct file symlink" + ) + + adder_real = adder_venv_path.resolve() + increment_real = increment_venv_path.resolve() + assert adder_real.parent / "libs" / libincrement_name == increment_real + assert "tree_ext_with_libs_srcs/" in adder_real.as_posix() + assert "tree_ext_with_libs_srcs/" in increment_real.as_posix() + + with open(adder_real, "rb") as f: + magic_bytes = f.read(4) + + if magic_bytes == _ELF_MAGIC: + _assert_elf_linking(str(adder_real)) + elif magic_bytes in _MACHO_MAGICS: + _assert_macho_linking(str(adder_real)) + else: + raise AssertionError( + f"Unsupported file format for adder: magic bytes {magic_bytes!r}" ) + assert adder.do_add() == 2 -if __name__ == "__main__": - unittest.main() + loaded_libs = _get_loaded_shared_libs() + assert adder_real in loaded_libs + assert increment_real in loaded_libs diff --git a/tests/venv_site_packages_libs/app_files_building/tree_venv_merge_test.py b/tests/venv_site_packages_libs/app_files_building/tree_venv_merge_test.py index bfee978aad..d633e76701 100644 --- a/tests/venv_site_packages_libs/app_files_building/tree_venv_merge_test.py +++ b/tests/venv_site_packages_libs/app_files_building/tree_venv_merge_test.py @@ -5,42 +5,34 @@ import importlib import os import sys -import unittest from pathlib import Path -class TreeVenvMergeTest(unittest.TestCase): - def _assert_in_venv(self, mod_file: str | None, mod_name: str) -> None: - self.assertIsNotNone(mod_file, f"Expected __file__ on {mod_name}") - assert mod_file is not None # type assert - actual = str(Path(mod_file)).lower() if os.name == "nt" else mod_file - prefix = str(Path(sys.prefix)).lower() if os.name == "nt" else sys.prefix - self.assertTrue( - actual.startswith(prefix), - f"{mod_name} ({mod_file}) was not loaded from venv ({sys.prefix})", - ) - - def test_imports_from_merged_tree_artifacts(self) -> None: - self.assertNotEqual( - sys.prefix, sys.base_prefix, "Expected to run inside a venv" - ) - foo = importlib.import_module("foo") - foo_a = importlib.import_module("foo.a") - foo_b = importlib.import_module("foo.plugins.b") - foo_shared = importlib.import_module("foo.plugins.shared") - ns_one = importlib.import_module("pkgutil_ns.one") - ns_two = importlib.import_module("pkgutil_ns.two") - - self.assertEqual(foo.ORIGIN, "tree_a") - self.assertEqual(foo_a.VALUE, "from_a") - self.assertEqual(foo_b.VALUE, "from_b") - self.assertEqual(foo_shared.WINNER, "tree_a") - self.assertEqual(ns_one.VALUE, 1) - self.assertEqual(ns_two.VALUE, 2) - - for mod in (foo, foo_a, foo_b, foo_shared, ns_one, ns_two): - self._assert_in_venv(mod.__file__, mod.__name__) - - -if __name__ == "__main__": - unittest.main() +def _assert_in_venv(mod_file: str | None, mod_name: str) -> None: + assert mod_file is not None, f"Expected __file__ on {mod_name}" + actual = str(Path(mod_file)).lower() if os.name == "nt" else mod_file + prefix = str(Path(sys.prefix)).lower() if os.name == "nt" else sys.prefix + assert actual.startswith(prefix), ( + f"{mod_name} ({mod_file}) was not loaded from venv ({sys.prefix})" + ) + + +def test_imports_from_merged_tree_artifacts() -> None: + assert sys.prefix != sys.base_prefix, "Expected to run inside a venv" + + foo = importlib.import_module("foo") + foo_a = importlib.import_module("foo.a") + foo_b = importlib.import_module("foo.plugins.b") + foo_shared = importlib.import_module("foo.plugins.shared") + ns_one = importlib.import_module("pkgutil_ns.one") + ns_two = importlib.import_module("pkgutil_ns.two") + + assert foo.ORIGIN == "tree_a" + assert foo_a.VALUE == "from_a" + assert foo_b.VALUE == "from_b" + assert foo_shared.WINNER == "tree_a" + assert ns_one.VALUE == 1 + assert ns_two.VALUE == 2 + + for mod in (foo, foo_a, foo_b, foo_shared, ns_one, ns_two): + _assert_in_venv(mod.__file__, mod.__name__) From 31bc40b512d07923c0ad11c7f0dbf73e69be851c Mon Sep 17 00:00:00 2001 From: Jeremy Volkman Date: Thu, 12 Nov 2026 05:36:21 +0000 Subject: [PATCH 7/7] fix: fail at analysis when venv entries can't be merged An entry that is linked as a single unit (a directory File, or a path inside a directory artifact without link_to_subpaths) can't share its venv path with files from other entries. Such overlaps used to surface only when creating the runfiles tree, or produced an incorrect venv. Detect the overlap while merging and fail with the entries involved. Clarify in the VenvSymlinkEntry docs that link_to_subpaths is optional and what leaving it empty means. --- python/private/py_info.bzl | 22 ++-- python/private/venv_runfiles.bzl | 67 +++++++++++- .../app_files_building_tests.bzl | 100 ++++++++++++++++-- 3 files changed, 169 insertions(+), 20 deletions(-) diff --git a/python/private/py_info.bzl b/python/private/py_info.bzl index 25293c61ad..ead24ec978 100644 --- a/python/private/py_info.bzl +++ b/python/private/py_info.bzl @@ -77,11 +77,13 @@ An entry in `PyInfo.venv_symlinks` Files backing this entry in runfiles. Entries may be regular `File` objects or directory artifacts (`File.is_directory`). -If any entries are directory artifacts, {obj}`link_to_subpaths` must list every -file under `link_to_path`, relative to it (left empty when `link_to_path` -points to a single file), because a directory artifact's contents aren't known -at analysis time. When {obj}`link_to_subpaths` is non-empty, conflict merging -uses it instead of `files`, and `files` then only supplies runfiles. +A directory artifact's contents aren't known at analysis time, so conflict +merging can't look inside one. To make an entry backed by a directory artifact +mergeable, list the files under `link_to_path` in {obj}`link_to_subpaths`; +conflict merging then uses those paths instead of `files`, and `files` only +supplies runfiles. If {obj}`link_to_subpaths` is empty, the entry is linked as +a single unit (`venv_path` to `link_to_path`) and can't be merged: analysis +fails if another entry's files have to be created under its `venv_path`. Conflict merging occurs when multiple targets have overlapping `venv_path` paths, e.g. if one adds files to `venv_path=a/` and another adds files to @@ -114,10 +116,12 @@ A runfiles-root relative path that `venv_path` will symlink to (if :::{include} /_includes/experimental_api.md ::: -Relative paths of every file under `link_to_path` when `files` contains -directory artifacts (`File.is_directory`) and `link_to_path` is a directory. -When non-empty, conflict merging uses these paths instead of `files` (and -`files` then only supplies runfiles). +Relative paths of the files under `link_to_path`, for entries whose `files` +are directory artifacts (`File.is_directory`) and whose `link_to_path` is a +directory. Optional. When non-empty, it must list every file under +`link_to_path`, and conflict merging uses these paths instead of `files` +(`files` then only supplies runfiles). When empty, the entry is linked as a +single unit and can't be merged with entries under its `venv_path`. This is for entries whose target isn't a single `File`, so it's mutually exclusive with `link_to_file`: `link_to_file` is only used when an entry diff --git a/python/private/venv_runfiles.bzl b/python/private/venv_runfiles.bzl index d3dfcbe2d5..670a914cd7 100644 --- a/python/private/venv_runfiles.bzl +++ b/python/private/venv_runfiles.bzl @@ -126,7 +126,7 @@ def create_venv_app_files(ctx, deps, venv_dir_map): ) # Visible for testing -def build_link_map(ctx, entries, return_conflicts = False): +def build_link_map(ctx, entries, return_conflicts = False, _fail = fail): """Compute the mapping of venv paths to their backing objects. Args: @@ -136,6 +136,8 @@ def build_link_map(ctx, entries, return_conflicts = False): return_conflicts: {type}`bool`. Only present for testing. If True, also return a list of the groups that had overlapping paths and had to be resolved and merged. + _fail: {type}`callable`. Only present for testing. Called with the + error message when overlapping entries can't be merged. Returns: {type}`dict[str, dict[str, File | str]]` Mappings of venv paths to their @@ -184,8 +186,13 @@ def build_link_map(ctx, entries, return_conflicts = False): if return_conflicts: conflicts.append(group) - # Merge a group of overlapping prefixes - _merge_venv_path_group(ctx, group, keep_kind_link_map) + # Merge a group of overlapping prefixes. Groups never share + # venv paths, so merging into a separate map gives the same + # result and limits the nesting check to this group's paths. + group_link_map = {} + _merge_venv_path_group(ctx, group, group_link_map) + _check_no_nested_links(kind, group, group_link_map, _fail) + keep_kind_link_map.update(group_link_map) keep_link_map[kind] = keep_kind_link_map if return_conflicts: @@ -193,6 +200,60 @@ def build_link_map(ctx, entries, return_conflicts = False): else: return keep_link_map +def _check_no_nested_links(kind, group, group_link_map, _fail): + """Fails if a merged venv path is also the parent of another one. + + An entry linked as a single unit (a directory `File`, or a path inside a + directory artifact that has no `link_to_subpaths`) can't have other + entries' files created under it: its venv path would have to be both a + symlink and a directory. + + Args: + kind: {type}`str` the `VenvSymlinkKind` value of the group. + group: {type}`list[VenvSymlinkEntry]` the entries that were merged. + group_link_map: {type}`dict[str, File | str]` the merged venv paths of + the group. + _fail: {type}`callable` called with the error message. + """ + for venv_path in group_link_map: + parent = paths.dirname(venv_path) + for _ in range(venv_path.count("/")): + if parent in group_link_map: + _fail(( + "venv {kind} path '{parent}' is linked as a single " + + "unit, but '{child}' must also be created under it. " + + "Overlapping entries can only be merged when their " + + "files are known at analysis time. Overlapping entries:\n" + + "{overlapping}" + ).format( + kind = kind, + parent = parent, + child = venv_path, + overlapping = _describe_entries_above(group, venv_path), + )) + return + parent = paths.dirname(parent) + +def _describe_entries_above(group, venv_path): + """Describes the entries in `group` that are at or above `venv_path`. + + Args: + group: {type}`list[VenvSymlinkEntry]` the entries that were merged. + venv_path: {type}`str` the venv path that couldn't be created. + + Returns: + {type}`str` one line per entry, for use in an error message. + """ + lines = [] + for entry in group: + if not (venv_path + "/").startswith(entry.venv_path + "/"): + continue + line = " '{}' -> {}".format(entry.venv_path, entry.link_to_path) + if entry.package: + line += " (package: {})".format(entry.package) + lines.append(line) + return "\n".join(lines) + def _group_venv_path_entries(entries): """Group entries by VenvSymlinkEntry.venv_path overlap. diff --git a/tests/venv_site_packages_libs/app_files_building/app_files_building_tests.bzl b/tests/venv_site_packages_libs/app_files_building/app_files_building_tests.bzl index f3200c4f2d..2c7ce620f6 100644 --- a/tests/venv_site_packages_libs/app_files_building/app_files_building_tests.bzl +++ b/tests/venv_site_packages_libs/app_files_building/app_files_building_tests.bzl @@ -143,15 +143,27 @@ def _assert_tree_artifact_equivalence( ]) env.expect.that_collection(tree_summary).contains_exactly(file_summary) - other_entries = [ - _entry( + # Make another package conflict with every entry: it adds a file under each + # directory entry and claims the same path as each single-file entry (a + # file can't have anything created under it). + dir_entries = { + (e.kind, e.venv_path): True + for e in tree_entries + if e.link_to_subpaths.to_list() + } + other_entries = [] + for e in file_entries: + other_path = "+other/site-packages/" + e.venv_path + if (e.kind, e.venv_path) in dir_entries: + other_file = other_path + "/__other__.py" + else: + other_file = other_path + other_entries.append(_entry( e.venv_path, - "+other/site-packages/" + e.venv_path, - [_file("../+other/site-packages/" + e.venv_path + "/__other__.py")], + other_path, + [_file("../" + other_file)], kind = e.kind, - ) - for e in file_entries - ] + )) file_merged = build_link_map(test_ctx, list(file_entries) + other_entries) tree_merged = build_link_map(test_ctx, list(tree_entries) + other_entries) for kind, kind_map in file_merged.items(): @@ -968,10 +980,82 @@ def _test_default_mode_directory_merge_impl(env, _): site_packages_root = "pkg/site-packages", ) env.expect.that_int(len(direct_entries)).equals(1) - direct_merged = build_link_map(test_ctx, list(direct_entries) + [conflicting]) + other_direct_dir = _entry( + "foo", + "+other/site-packages/foo", + [_file("../+other/site-packages/foo", is_directory = True)], + ) + direct_merged = build_link_map(test_ctx, list(direct_entries) + [other_direct_dir]) env.expect.that_bool(types.is_string(direct_merged[VenvSymlinkKind.LIB]["foo"])).equals(False) env.expect.that_dict(direct_merged[VenvSymlinkKind.LIB]).contains_exactly({ "foo": direct_dir, + }) + +def _test_unmergeable_overlap_fails(name): + analysis_test( + name = name, + impl = _test_unmergeable_overlap_fails_impl, + target = "//python:none", + ) + +_tests.append(_test_unmergeable_overlap_fails) + +def _test_unmergeable_overlap_fails_impl(env, _): + test_ctx = _ctx() + other = _entry( + "foo", + "+other/site-packages/foo", + [_file("../+other/site-packages/foo/other.py")], + package = "other", + ) + + # A directly linked directory File: `foo` would have to be both a symlink + # and a directory containing `other.py`. + direct_dir = _entry( + "foo", + "_main/pkg/site-packages/foo", + [_file("pkg/site-packages/foo", is_directory = True)], + package = "direct", + ) + failures = [] + build_link_map(test_ctx, [direct_dir, other], _fail = failures.append) + env.expect.that_collection(failures).has_size(1) + env.expect.that_str(failures[0]).contains("'foo/other.py'") + env.expect.that_str(failures[0]).contains("(package: direct)") + env.expect.that_str(failures[0]).contains("(package: other)") + + # A directory inside a directory artifact without link_to_subpaths: its + # files aren't known, so it can't be merged file by file either. The + # result doesn't depend on the order of the entries. + tree_dir = _entry( + "foo", + "_main/pkg/srcs/site-packages/foo", + [_file("pkg/srcs", is_directory = True)], + package = "tree", + ) + failures = [] + build_link_map(test_ctx, [other, tree_dir], _fail = failures.append) + env.expect.that_collection(failures).has_size(1) + env.expect.that_str(failures[0]).contains("'foo/other.py'") + env.expect.that_str(failures[0]).contains("(package: tree)") + + # With link_to_subpaths, the same layout merges. + tree_dir_with_subpaths = _entry( + "foo", + "_main/pkg/srcs/site-packages/foo", + [_file("pkg/srcs", is_directory = True)], + package = "tree", + link_to_subpaths = depset(["__init__.py"]), + ) + failures = [] + merged = build_link_map( + test_ctx, + [tree_dir_with_subpaths, other], + _fail = failures.append, + ) + env.expect.that_collection(failures).has_size(0) + env.expect.that_dict(merged[VenvSymlinkKind.LIB]).contains_exactly({ + "foo/__init__.py": "_main/pkg/srcs/site-packages/foo/__init__.py", "foo/other.py": _file("../+other/site-packages/foo/other.py"), })