From 01be3e25793af7d78cfe3d588d68b20d3d7cf96a Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Fri, 7 Feb 2025 22:40:14 -0800 Subject: [PATCH 01/12] wip: libs in venv site packages --- .bazelrc | 4 +- CHANGELOG.md | 9 +- MODULE.bazel | 6 ++ python/features.bzl | 1 + python/private/attributes.bzl | 6 ++ python/private/builders.bzl | 13 ++- python/private/common.bzl | 16 ++- python/private/py_executable.bzl | 65 +++++++++++- python/private/py_info.bzl | 32 +++++- python/private/py_library.bzl | 99 +++++++++++++++++++ tests/modules/other/BUILD.bazel | 0 tests/modules/other/MODULE.bazel | 3 + tests/modules/other/nspkg_delta/BUILD.bazel | 9 ++ .../nspkg/subnspkg/delta/__init__.py | 1 + tests/modules/other/nspkg_gamma/BUILD.bazel | 9 ++ .../nspkg/subnspkg/gamma/__init__.py | 1 + tests/venv_site_packages_libs/BUILD.bazel | 16 +++ tests/venv_site_packages_libs/bin.py | 32 ++++++ .../nspkg_alpha/BUILD.bazel | 9 ++ .../nspkg/subnspkg/alpha/__init__.py | 1 + .../nspkg_beta/BUILD.bazel | 9 ++ .../nspkg/subnspkg/beta/__init__.py | 1 + .../venv_site_packages_pypi_test.py | 36 +++++++ 23 files changed, 369 insertions(+), 9 deletions(-) create mode 100644 tests/modules/other/BUILD.bazel create mode 100644 tests/modules/other/MODULE.bazel create mode 100644 tests/modules/other/nspkg_delta/BUILD.bazel create mode 100644 tests/modules/other/nspkg_delta/site-packages/nspkg/subnspkg/delta/__init__.py create mode 100644 tests/modules/other/nspkg_gamma/BUILD.bazel create mode 100644 tests/modules/other/nspkg_gamma/site-packages/nspkg/subnspkg/gamma/__init__.py create mode 100644 tests/venv_site_packages_libs/BUILD.bazel create mode 100644 tests/venv_site_packages_libs/bin.py create mode 100644 tests/venv_site_packages_libs/nspkg_alpha/BUILD.bazel create mode 100644 tests/venv_site_packages_libs/nspkg_alpha/site-packages/nspkg/subnspkg/alpha/__init__.py create mode 100644 tests/venv_site_packages_libs/nspkg_beta/BUILD.bazel create mode 100644 tests/venv_site_packages_libs/nspkg_beta/site-packages/nspkg/subnspkg/beta/__init__.py create mode 100644 tests/venv_site_packages_libs/venv_site_packages_pypi_test.py diff --git a/.bazelrc b/.bazelrc index ada5c5a0a7..4e6f2fa187 100644 --- a/.bazelrc +++ b/.bazelrc @@ -4,8 +4,8 @@ # (Note, we cannot use `common --deleted_packages` because the bazel version command doesn't support it) # To update these lines, execute # `bazel run @rules_bazel_integration_test//tools:update_deleted_packages` -build --deleted_packages=examples/build_file_generation,examples/build_file_generation/random_number_generator,examples/bzlmod,examples/bzlmod_build_file_generation,examples/bzlmod_build_file_generation/other_module/other_module/pkg,examples/bzlmod_build_file_generation/runfiles,examples/bzlmod/entry_points,examples/bzlmod/entry_points/tests,examples/bzlmod/libs/my_lib,examples/bzlmod/other_module,examples/bzlmod/other_module/other_module/pkg,examples/bzlmod/patches,examples/bzlmod/py_proto_library,examples/bzlmod/py_proto_library/example.com/another_proto,examples/bzlmod/py_proto_library/example.com/proto,examples/bzlmod/runfiles,examples/bzlmod/tests,examples/bzlmod/tests/other_module,examples/bzlmod/whl_mods,examples/multi_python_versions/libs/my_lib,examples/multi_python_versions/requirements,examples/multi_python_versions/tests,examples/pip_parse,examples/pip_parse_vendored,examples/pip_repository_annotations,examples/py_proto_library,examples/py_proto_library/example.com/another_proto,examples/py_proto_library/example.com/proto,gazelle,gazelle/manifest,gazelle/manifest/generate,gazelle/manifest/hasher,gazelle/manifest/test,gazelle/modules_mapping,gazelle/python,gazelle/pythonconfig,gazelle/python/private,tests/integration/compile_pip_requirements,tests/integration/compile_pip_requirements_test_from_external_repo,tests/integration/custom_commands,tests/integration/ignore_root_user_error,tests/integration/ignore_root_user_error/submodule,tests/integration/local_toolchains,tests/integration/pip_parse,tests/integration/pip_parse/empty,tests/integration/py_cc_toolchain_registered -query --deleted_packages=examples/build_file_generation,examples/build_file_generation/random_number_generator,examples/bzlmod,examples/bzlmod_build_file_generation,examples/bzlmod_build_file_generation/other_module/other_module/pkg,examples/bzlmod_build_file_generation/runfiles,examples/bzlmod/entry_points,examples/bzlmod/entry_points/tests,examples/bzlmod/libs/my_lib,examples/bzlmod/other_module,examples/bzlmod/other_module/other_module/pkg,examples/bzlmod/patches,examples/bzlmod/py_proto_library,examples/bzlmod/py_proto_library/example.com/another_proto,examples/bzlmod/py_proto_library/example.com/proto,examples/bzlmod/runfiles,examples/bzlmod/tests,examples/bzlmod/tests/other_module,examples/bzlmod/whl_mods,examples/multi_python_versions/libs/my_lib,examples/multi_python_versions/requirements,examples/multi_python_versions/tests,examples/pip_parse,examples/pip_parse_vendored,examples/pip_repository_annotations,examples/py_proto_library,examples/py_proto_library/example.com/another_proto,examples/py_proto_library/example.com/proto,gazelle,gazelle/manifest,gazelle/manifest/generate,gazelle/manifest/hasher,gazelle/manifest/test,gazelle/modules_mapping,gazelle/python,gazelle/pythonconfig,gazelle/python/private,tests/integration/compile_pip_requirements,tests/integration/compile_pip_requirements_test_from_external_repo,tests/integration/custom_commands,tests/integration/ignore_root_user_error,tests/integration/ignore_root_user_error/submodule,tests/integration/local_toolchains,tests/integration/pip_parse,tests/integration/pip_parse/empty,tests/integration/py_cc_toolchain_registered +build --deleted_packages=examples/build_file_generation,examples/build_file_generation/random_number_generator,examples/bzlmod,examples/bzlmod_build_file_generation,examples/bzlmod_build_file_generation/other_module/other_module/pkg,examples/bzlmod_build_file_generation/runfiles,examples/bzlmod/entry_points,examples/bzlmod/entry_points/tests,examples/bzlmod/libs/my_lib,examples/bzlmod/other_module,examples/bzlmod/other_module/other_module/pkg,examples/bzlmod/patches,examples/bzlmod/py_proto_library,examples/bzlmod/py_proto_library/example.com/another_proto,examples/bzlmod/py_proto_library/example.com/proto,examples/bzlmod/runfiles,examples/bzlmod/tests,examples/bzlmod/tests/other_module,examples/bzlmod/whl_mods,examples/multi_python_versions/libs/my_lib,examples/multi_python_versions/requirements,examples/multi_python_versions/tests,examples/pip_parse,examples/pip_parse_vendored,examples/pip_repository_annotations,examples/py_proto_library,examples/py_proto_library/example.com/another_proto,examples/py_proto_library/example.com/proto,gazelle,gazelle/manifest,gazelle/manifest/generate,gazelle/manifest/hasher,gazelle/manifest/test,gazelle/modules_mapping,gazelle/python,gazelle/pythonconfig,gazelle/python/private,tests/integration/compile_pip_requirements,tests/integration/compile_pip_requirements_test_from_external_repo,tests/integration/custom_commands,tests/integration/ignore_root_user_error,tests/integration/ignore_root_user_error/submodule,tests/integration/local_toolchains,tests/integration/pip_parse,tests/integration/pip_parse/empty,tests/integration/py_cc_toolchain_registered,tests/modules/other,tests/modules/other/nspkg_delta,tests/modules/other/nspkg_gamma +query --deleted_packages=examples/build_file_generation,examples/build_file_generation/random_number_generator,examples/bzlmod,examples/bzlmod_build_file_generation,examples/bzlmod_build_file_generation/other_module/other_module/pkg,examples/bzlmod_build_file_generation/runfiles,examples/bzlmod/entry_points,examples/bzlmod/entry_points/tests,examples/bzlmod/libs/my_lib,examples/bzlmod/other_module,examples/bzlmod/other_module/other_module/pkg,examples/bzlmod/patches,examples/bzlmod/py_proto_library,examples/bzlmod/py_proto_library/example.com/another_proto,examples/bzlmod/py_proto_library/example.com/proto,examples/bzlmod/runfiles,examples/bzlmod/tests,examples/bzlmod/tests/other_module,examples/bzlmod/whl_mods,examples/multi_python_versions/libs/my_lib,examples/multi_python_versions/requirements,examples/multi_python_versions/tests,examples/pip_parse,examples/pip_parse_vendored,examples/pip_repository_annotations,examples/py_proto_library,examples/py_proto_library/example.com/another_proto,examples/py_proto_library/example.com/proto,gazelle,gazelle/manifest,gazelle/manifest/generate,gazelle/manifest/hasher,gazelle/manifest/test,gazelle/modules_mapping,gazelle/python,gazelle/pythonconfig,gazelle/python/private,tests/integration/compile_pip_requirements,tests/integration/compile_pip_requirements_test_from_external_repo,tests/integration/custom_commands,tests/integration/ignore_root_user_error,tests/integration/ignore_root_user_error/submodule,tests/integration/local_toolchains,tests/integration/pip_parse,tests/integration/pip_parse/empty,tests/integration/py_cc_toolchain_registered,tests/modules/other,tests/modules/other/nspkg_delta,tests/modules/other/nspkg_gamma test --test_output=errors diff --git a/CHANGELOG.md b/CHANGELOG.md index e93cdc5327..2d97acca4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,7 +60,14 @@ Unreleased changes template. {#v0-0-0-added} ### Added -* Nothing added. +* (providers) {obj}`PyInfo.site_packages_symlinks` field added to allow + specifying links to create within the venv site packages + (only applicable with {obj}`--bootstrap_impl=script`) + ([#2156](https://github.com/bazelbuild/rules_python/issues/2156)). +* (rules) {obj}`py_library.site_packages_root` attribute added to allow + specifying a library's sources follow a site-packages file layout. + (only applicable with {obj}`--bootstrap_impl=script`) + ([#2156](https://github.com/bazelbuild/rules_python/issues/2156)). {#v0-0-0-removed} ### Removed diff --git a/MODULE.bazel b/MODULE.bazel index 76710e4ac4..6985ec03ac 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -85,6 +85,7 @@ bazel_dep(name = "rules_shell", version = "0.3.0", dev_dependency = True) bazel_dep(name = "rules_multirun", version = "0.9.0", dev_dependency = True) bazel_dep(name = "bazel_ci_rules", version = "1.0.0", dev_dependency = True) bazel_dep(name = "rules_pkg", version = "1.0.1", dev_dependency = True) +bazel_dep(name = "other", version = "0", dev_dependency = True) # Extra gazelle plugin deps so that WORKSPACE.bzlmod can continue including it for e2e tests. # We use `WORKSPACE.bzlmod` because it is impossible to have dev-only local overrides. @@ -106,6 +107,11 @@ local_path_override( path = "gazelle", ) +local_path_override( + module_name = "other", + path = "tests/modules/other", +) + dev_python = use_extension( "//python/extensions:python.bzl", "python", diff --git a/python/features.bzl b/python/features.bzl index a7098f4710..b1bd9cd0b5 100644 --- a/python/features.bzl +++ b/python/features.bzl @@ -23,4 +23,5 @@ features = struct( version = _VERSION_PRIVATE if "$Format" not in _VERSION_PRIVATE else "", precompile = True, uses_builtin_rules = not config.enable_pystar, + site_packages_root_attr = True, ) diff --git a/python/private/attributes.bzl b/python/private/attributes.bzl index e167482eb1..bf2ccbaaca 100644 --- a/python/private/attributes.bzl +++ b/python/private/attributes.bzl @@ -282,6 +282,12 @@ that depend on this rule. The strings are repo-runfiles-root relative, Absolute paths (paths that start with `/`) and paths that references a path above the execution root are not allowed and will result in an error. + +:::{attention} +Setting both this and the {attr}`site_packages_root` attribute may result in +undefined behavior. Both will result in the code being importable, but from +different sys.path (and thus `__file__`) entries. +::: """, ), } diff --git a/python/private/builders.bzl b/python/private/builders.bzl index bf5dbb8667..93cfe82952 100644 --- a/python/private/builders.bzl +++ b/python/private/builders.bzl @@ -15,12 +15,19 @@ load("@bazel_skylib//lib:types.bzl", "types") -def _DepsetBuilder(): - """Create a builder for a depset.""" +def _DepsetBuilder(order = None): + """Create a builder for a depset. + + Args: + order: {type}`str | None` The order to initialize the depset to, if any. + + Returns: + {type}`DepsetBuilder` + """ # buildifier: disable=uninitialized self = struct( - _order = [None], + _order = [order], add = lambda *a, **k: _DepsetBuilder_add(self, *a, **k), build = lambda *a, **k: _DepsetBuilder_build(self, *a, **k), direct = [], diff --git a/python/private/common.bzl b/python/private/common.bzl index 137f0d23f3..12b120d16b 100644 --- a/python/private/common.bzl +++ b/python/private/common.bzl @@ -30,6 +30,15 @@ PackageSpecificationInfo = getattr(py_internal, "PackageSpecificationInfo", None # Extensions without the dot _PYTHON_SOURCE_EXTENSIONS = ["py"] +# Extensions that make a file considered importable +PYTHON_FILE_EXTENSIONS = [ + "py", + "so", # Python C modules, usually Linux + "dylib", # Python C modules, Mac specific + "pyc", + "dll", # Python C modules, Windows specific +] + def create_binary_semantics_struct( *, create_executable, @@ -413,7 +422,8 @@ def create_py_info( required_pyc_files, implicit_pyc_files, implicit_pyc_source_files, - imports): + imports, + site_packages_symlinks = []): """Create PyInfo provider. Args: @@ -431,6 +441,9 @@ def create_py_info( implicit_pyc_files: {type}`depset[File]` Implicitly generated pyc files that a binary can choose to include. imports: depset of strings; the import path values to propagate. + site_packages_symlinks: {type}`list[tuple[str, str]]` tuples of + `(runfiles_path, site_packages_path)` for symlinks to create + in the consuming binary's venv site packages. Returns: A tuple of the PyInfo instance and a depset of the @@ -438,6 +451,7 @@ def create_py_info( necessary for deprecated extra actions support). """ py_info = PyInfoBuilder() + py_info.site_packages_symlinks.add(site_packages_symlinks) py_info.direct_original_sources.add(original_sources) py_info.direct_pyc_files.add(required_pyc_files) py_info.direct_pyi_files.add(ctx.files.pyi_srcs) diff --git a/python/private/py_executable.bzl b/python/private/py_executable.bzl index a2ccdc65f3..bfd7655b19 100644 --- a/python/private/py_executable.bzl +++ b/python/private/py_executable.bzl @@ -591,15 +591,78 @@ def _create_venv(ctx, output_prefix, imports, runtime_details): }, computed_substitutions = computed_subs, ) + site_packages_symlinks = _create_site_packages_symlinks(ctx, site_packages) return struct( interpreter = interpreter, recreate_venv_at_runtime = not venvs_use_declare_symlink_enabled, # Runfiles root relative path or absolute path interpreter_actual_path = interpreter_actual_path, - files_without_interpreter = [pyvenv_cfg, pth, site_init], + files_without_interpreter = [pyvenv_cfg, pth, site_init] + site_packages_symlinks, ) +def _create_site_packages_symlinks(ctx, site_packages): + """Creates symlinks within site-packages. + + Args: + ctx: current rule ctx + site_packages: runfiles-root-relative path to the site-packages directory + + Returns: + {type}`list[File]` list of the File symlink objects created. + """ + + # maps site-package symlink to the runfiles path it should point to + entries = depset( + # NOTE: Topological ordering is used so that dependencies closer to the + # binary have precedence in creating their symlinks. This allows the + # binary a modicum of control over the result. + order = "topological", + transitive = [ + dep[PyInfo].site_packages_symlinks + for dep in ctx.attr.deps + if PyInfo in dep + ], + ).to_list() + link_map = {} + for link_to_runfiles_path, site_packages_path in entries: + if site_packages_path in link_map: + # We ignore duplicates by design. The dependency closer to the + # binary gets precedence due to the topological ordering. + continue + else: + link_map[site_packages_path] = link_to_runfiles_path + + # An empty link_to value means to not create the site package symlink. + # Because of the topological ordering, this allows binaries to remove + # entries by having an earlier dependency produce empty link_to values + for sp_dir_path, link_to in link_map.items(): + if not link_to: + link_map.pop(sp_dir_path) + + # This is N^2; we can certainly do better by sorting and exploiting the + # order. + # A trailing slash is appended / to prevent /X matching /XY + sp_dirs = [x + "/" for x in link_map.keys()] + for search_for in sp_dirs: + for prefix in sp_dirs: + if search_for != prefix and search_for.startswith(prefix): + fail("sub-link: {} under {}", search_for, prefix) + + sp_files = [] + for sp_dir_path, link_to in link_map.items(): + sp_link = ctx.actions.declare_symlink(paths.join(site_packages, sp_dir_path)) + sp_link_rf_path = runfiles_root_path(ctx, sp_link.short_path) + rel_path = relative_path( + # dirname is necessary because a relative symlink is relative to + # the directory the symlink resides within. + from_ = paths.dirname(sp_link_rf_path), + to = link_to, + ) + ctx.actions.symlink(output = sp_link, target_path = rel_path) + sp_files.append(sp_link) + return sp_files + def _map_each_identity(v): return v diff --git a/python/private/py_info.bzl b/python/private/py_info.bzl index ef654c303e..ae326278a9 100644 --- a/python/private/py_info.bzl +++ b/python/private/py_info.bzl @@ -42,7 +42,8 @@ def _PyInfo_init( direct_original_sources = depset(), transitive_original_sources = depset(), direct_pyi_files = depset(), - transitive_pyi_files = depset()): + transitive_pyi_files = depset(), + site_packages_symlinks = depset()): _check_arg_type("transitive_sources", "depset", transitive_sources) # Verify it's postorder compatible, but retain is original ordering. @@ -70,6 +71,7 @@ def _PyInfo_init( "has_py2_only_sources": has_py2_only_sources, "has_py3_only_sources": has_py2_only_sources, "imports": imports, + "site_packages_symlinks": site_packages_symlinks, "transitive_implicit_pyc_files": transitive_implicit_pyc_files, "transitive_implicit_pyc_source_files": transitive_implicit_pyc_source_files, "transitive_original_sources": transitive_original_sources, @@ -140,6 +142,31 @@ A depset of import path strings to be added to the `PYTHONPATH` of executable Python targets. These are accumulated from the transitive `deps`. The order of the depset is not guaranteed and may be changed in the future. It is recommended to use `default` order (the default). +""", + "site_packages_symlinks": """ +:type: depset[tuple[str | None, str]] + +A depset with `topological` ordering. + +Tuples of `(runfiles_path, site_packages_path)`. Where +* `runfiles_path` is a runfiles-root relative path. It is the path that + has the code to make importable. If `None` or empty string, then it means + to not create a site packages directory with the `site_packages_path` + name. +* `site_packages_path` is a path relative to the site-packages directory of + the venv for whatever creates the venv (typically py_binary). It makes + the code in `runfiles_path` available for import. Note that this + is created as a "raw" symlink (via `declare_symlink`). + +:::{tip} +The topological ordering means dependencies earlier and closer to the consumer +have precedence. This allows e.g. a binary to add dependencies that override +values from further way dependencies, such as forcing symlinks to point to +specific paths or preventing symlinks from being created. +::: + +:::{versionadded} VERSION_NEXT_FEATURE +::: """, "transitive_implicit_pyc_files": """ :type: depset[File] @@ -266,6 +293,7 @@ def PyInfoBuilder(): transitive_pyc_files = builders.DepsetBuilder(), transitive_pyi_files = builders.DepsetBuilder(), transitive_sources = builders.DepsetBuilder(), + site_packages_symlinks = builders.DepsetBuilder(order = "topological"), ) return self @@ -351,6 +379,7 @@ def _PyInfoBuilder_merge_all(self, transitive, *, direct = []): self.transitive_original_sources.add(info.transitive_original_sources) self.transitive_pyc_files.add(info.transitive_pyc_files) self.transitive_pyi_files.add(info.transitive_pyi_files) + self.site_packages_symlinks.add(info.site_packages_symlinks) return self @@ -400,6 +429,7 @@ def _PyInfoBuilder_build(self): transitive_original_sources = self.transitive_original_sources.build(), transitive_pyc_files = self.transitive_pyc_files.build(), transitive_pyi_files = self.transitive_pyi_files.build(), + site_packages_symlinks = self.site_packages_symlinks.build(), ) else: kwargs = {} diff --git a/python/private/py_library.bzl b/python/private/py_library.bzl index 350ea35aa6..70c68a9b78 100644 --- a/python/private/py_library.bzl +++ b/python/private/py_library.bzl @@ -14,6 +14,7 @@ """Common code for implementing py_library rules.""" load("@bazel_skylib//lib:dicts.bzl", "dicts") +load("@bazel_skylib//lib:paths.bzl", "paths") load("@bazel_skylib//rules:common_settings.bzl", "BuildSettingInfo") load( ":attributes.bzl", @@ -29,12 +30,14 @@ load( load(":builders.bzl", "builders") load( ":common.bzl", + "PYTHON_FILE_EXTENSIONS", "collect_imports", "collect_runfiles", "create_instrumented_files_info", "create_output_group_info", "create_py_info", "filter_to_py_srcs", + "runfiles_root_path", "union_attrs", ) load(":flags.bzl", "AddSrcsToRunfilesFlag", "PrecompileFlag") @@ -55,6 +58,32 @@ LIBRARY_ATTRS = union_attrs( create_srcs_version_attr(values = SRCS_VERSION_ALL_VALUES), create_srcs_attr(mandatory = False), { + "site_packages_root": attr.string( + doc = """ +Package relative prefix to remove from `srcs` for site-packages layouts. + +When set, `srcs` are interpreted to have a file layout as if they were installed +in site-packages. This attribute specifies the directory within `srcs` to treat +as the site-packages root so the correct site-packages relative paths for +the files can be computed. + +For example, given `srcs=["site-packages/foo/bar.py"]`, specifying +`site_packages_root="site-packages/" means `foo/bar.py` is the file path +under the binary's venv site-packages directory that should be made availble. + +:::{note} +This string is relative to the target's *Bazel package*. e.g. Relative to the +directory with the BUILD file that defines the target (the same as how e.g. +`srcs`). +::: + +:::{attention} +Setting both this an the {attr}`imports` attribute may result in undefined +behavior. Both will result in the code being importable, but from different +sys.path (and thus `__file__`) entries. +::: +""", + ), "_add_srcs_to_runfiles_flag": attr.label( default = "//python/config_settings:add_srcs_to_runfiles", ), @@ -99,6 +128,8 @@ def py_library_impl(ctx, *, semantics): runfiles.add(collect_runfiles(ctx)) runfiles = runfiles.build(ctx) + site_packages_symlinks = _get_site_packages_symlinks(ctx) + cc_info = semantics.get_cc_info_for_library(ctx) py_info, deps_transitive_sources, builtins_py_info = create_py_info( ctx, @@ -108,6 +139,7 @@ def py_library_impl(ctx, *, semantics): implicit_pyc_files = implicit_pyc_files, implicit_pyc_source_files = implicit_pyc_source_files, imports = collect_imports(ctx, semantics), + site_packages_symlinks = site_packages_symlinks, ) # TODO(b/253059598): Remove support for extra actions; https://github.com/bazelbuild/bazel/issues/16455 @@ -171,3 +203,70 @@ def create_py_library_rule(*, attrs = {}, **kwargs): fragments = fragments + ["py"], **kwargs ) + +def _get_site_packages_symlinks(ctx): + if not ctx.attr.site_packages_root: + return [] + + # We have to build a list of (runfiles path, site-packages path) pairs of + # the files to create in the consuming binary's venv site-packages directory. + # To minimize the number of files to create, we just return the paths + # to the directories containing the code of interest. + # + # However, namespace packages complicate matters: multiple + # distributions install in the same directory in site-packages. This + # works out because they don't overlap in their files. Typically, they + # install to different directories within the namespace package + # directory. Namespace package directories are simply directories + # within site-packages that *don't* have an `__init__.py` file, which + # can be arbitrarily deep. Thus, we simply have to look for the + # directories that _do_ have an `__init__.py` file and treat those as + # the path to symlink to. + + site_packages_root = paths.join(ctx.label.package, ctx.attr.site_packages_root) + repo_runfiles_dirname = None + dirs_with_init = {} # dirname -> runfile path + for src in ctx.files.srcs: + if src.extension not in PYTHON_FILE_EXTENSIONS: + continue + path = _repo_relative_short_path(src.short_path) + if not path.startswith(site_packages_root): + continue + path = path.removeprefix(site_packages_root) + dir_name, _, filename = path.rpartition("/") + if not dir_name: + # This would be e.g. `site-packages/__init__.py`, which isn't valid. + # Apparently, the pypi integration adds such a file? + continue + + if filename.startswith("__init__."): + dirs_with_init[dir_name] = None + repo_runfiles_dirname = runfiles_root_path(ctx, src.short_path).partition("/")[0] + + # Sort so that we encounter `foo` before `foo/bar`. This ensures we + # see the top-most explicit package first. + dirnames = sorted(dirs_with_init.keys()) + first_level_explicit_packages = [] + for d in dirnames: + is_sub_package = False + for existing in first_level_explicit_packages: + # Suffix with / to prevent foo matching foobar + if d.startswith(existing + "/"): + is_sub_package = True + break + if not is_sub_package: + first_level_explicit_packages.append(d) + + site_packages_symlinks = [] + for dirname in first_level_explicit_packages: + site_packages_symlinks.append(( + paths.join(repo_runfiles_dirname, site_packages_root, dirname), + dirname, + )) + return site_packages_symlinks + +def _repo_relative_short_path(short_path): + if short_path.startswith("../"): + return short_path[3:].partition("/")[2] + else: + return short_path diff --git a/tests/modules/other/BUILD.bazel b/tests/modules/other/BUILD.bazel new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/modules/other/MODULE.bazel b/tests/modules/other/MODULE.bazel new file mode 100644 index 0000000000..7cd3118b81 --- /dev/null +++ b/tests/modules/other/MODULE.bazel @@ -0,0 +1,3 @@ +module(name = "other") + +bazel_dep(name = "rules_python", version = "0") diff --git a/tests/modules/other/nspkg_delta/BUILD.bazel b/tests/modules/other/nspkg_delta/BUILD.bazel new file mode 100644 index 0000000000..e3b005e4b2 --- /dev/null +++ b/tests/modules/other/nspkg_delta/BUILD.bazel @@ -0,0 +1,9 @@ +load("@rules_python//python:py_library.bzl", "py_library") + +package(default_visibility = ["//visibility:public"]) + +py_library( + name = "nspkg_delta", + srcs = glob(["site-packages/**/*.py"]), + site_packages_root = "site-packages/", +) diff --git a/tests/modules/other/nspkg_delta/site-packages/nspkg/subnspkg/delta/__init__.py b/tests/modules/other/nspkg_delta/site-packages/nspkg/subnspkg/delta/__init__.py new file mode 100644 index 0000000000..bb7b160deb --- /dev/null +++ b/tests/modules/other/nspkg_delta/site-packages/nspkg/subnspkg/delta/__init__.py @@ -0,0 +1 @@ +# Intentionally empty diff --git a/tests/modules/other/nspkg_gamma/BUILD.bazel b/tests/modules/other/nspkg_gamma/BUILD.bazel new file mode 100644 index 0000000000..dbbd7a7c04 --- /dev/null +++ b/tests/modules/other/nspkg_gamma/BUILD.bazel @@ -0,0 +1,9 @@ +load("@rules_python//python:py_library.bzl", "py_library") + +package(default_visibility = ["//visibility:public"]) + +py_library( + name = "nspkg_gamma", + srcs = glob(["site-packages/**/*.py"]), + site_packages_root = "site-packages/", +) diff --git a/tests/modules/other/nspkg_gamma/site-packages/nspkg/subnspkg/gamma/__init__.py b/tests/modules/other/nspkg_gamma/site-packages/nspkg/subnspkg/gamma/__init__.py new file mode 100644 index 0000000000..bb7b160deb --- /dev/null +++ b/tests/modules/other/nspkg_gamma/site-packages/nspkg/subnspkg/gamma/__init__.py @@ -0,0 +1 @@ +# Intentionally empty diff --git a/tests/venv_site_packages_libs/BUILD.bazel b/tests/venv_site_packages_libs/BUILD.bazel new file mode 100644 index 0000000000..22b7d9fa97 --- /dev/null +++ b/tests/venv_site_packages_libs/BUILD.bazel @@ -0,0 +1,16 @@ +load("//tests/support:sh_py_run_test.bzl", "py_reconfig_test") +load("//tests/support:support.bzl", "SUPPORTS_BOOTSTRAP_SCRIPT") + +py_reconfig_test( + name = "venv_site_packages_libs_test", + srcs = ["bin.py"], + bootstrap_impl = "script", + main = "bin.py", + target_compatible_with = SUPPORTS_BOOTSTRAP_SCRIPT, + deps = [ + "//tests/venv_site_packages_libs/nspkg_alpha", + "//tests/venv_site_packages_libs/nspkg_beta", + "@other//nspkg_delta", + "@other//nspkg_gamma", + ], +) diff --git a/tests/venv_site_packages_libs/bin.py b/tests/venv_site_packages_libs/bin.py new file mode 100644 index 0000000000..b944be69e3 --- /dev/null +++ b/tests/venv_site_packages_libs/bin.py @@ -0,0 +1,32 @@ +import importlib +import os +import sys +import unittest + + +class VenvSitePackagesLibraryTest(unittest.TestCase): + def setUp(self): + super().setUp() + if sys.prefix == sys.base_prefix: + raise AssertionError("Not running under a venv") + self.venv = sys.prefix + + def assert_imported_from_venv(self, module_name): + module = importlib.import_module(module_name) + self.assertEqual(module.__name__, module_name) + self.assertTrue( + module.__file__.startswith(self.venv), + f"\n{module_name} was imported, but not from the venv.\n" + + f"venv : {self.venv}\n" + + f"actual: {module.__file__}", + ) + + def test_imported_from_venv(self): + self.assert_imported_from_venv("nspkg.subnspkg.alpha") + self.assert_imported_from_venv("nspkg.subnspkg.beta") + self.assert_imported_from_venv("nspkg.subnspkg.gamma") + self.assert_imported_from_venv("nspkg.subnspkg.delta") + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/venv_site_packages_libs/nspkg_alpha/BUILD.bazel b/tests/venv_site_packages_libs/nspkg_alpha/BUILD.bazel new file mode 100644 index 0000000000..2a53839048 --- /dev/null +++ b/tests/venv_site_packages_libs/nspkg_alpha/BUILD.bazel @@ -0,0 +1,9 @@ +load("@rules_python//python:py_library.bzl", "py_library") + +package(default_visibility = ["//visibility:public"]) + +py_library( + name = "nspkg_alpha", + srcs = glob(["site-packages/**/*.py"]), + site_packages_root = "site-packages/", +) diff --git a/tests/venv_site_packages_libs/nspkg_alpha/site-packages/nspkg/subnspkg/alpha/__init__.py b/tests/venv_site_packages_libs/nspkg_alpha/site-packages/nspkg/subnspkg/alpha/__init__.py new file mode 100644 index 0000000000..b5ee093672 --- /dev/null +++ b/tests/venv_site_packages_libs/nspkg_alpha/site-packages/nspkg/subnspkg/alpha/__init__.py @@ -0,0 +1 @@ +whoami = "alpha" diff --git a/tests/venv_site_packages_libs/nspkg_beta/BUILD.bazel b/tests/venv_site_packages_libs/nspkg_beta/BUILD.bazel new file mode 100644 index 0000000000..b0bfdfceba --- /dev/null +++ b/tests/venv_site_packages_libs/nspkg_beta/BUILD.bazel @@ -0,0 +1,9 @@ +load("@rules_python//python:py_library.bzl", "py_library") + +package(default_visibility = ["//visibility:public"]) + +py_library( + name = "nspkg_beta", + srcs = glob(["site-packages/**/*.py"]), + site_packages_root = "site-packages/", +) diff --git a/tests/venv_site_packages_libs/nspkg_beta/site-packages/nspkg/subnspkg/beta/__init__.py b/tests/venv_site_packages_libs/nspkg_beta/site-packages/nspkg/subnspkg/beta/__init__.py new file mode 100644 index 0000000000..a2a65910c7 --- /dev/null +++ b/tests/venv_site_packages_libs/nspkg_beta/site-packages/nspkg/subnspkg/beta/__init__.py @@ -0,0 +1 @@ +whoami = "beta" diff --git a/tests/venv_site_packages_libs/venv_site_packages_pypi_test.py b/tests/venv_site_packages_libs/venv_site_packages_pypi_test.py new file mode 100644 index 0000000000..519b258044 --- /dev/null +++ b/tests/venv_site_packages_libs/venv_site_packages_pypi_test.py @@ -0,0 +1,36 @@ +import os +import sys +import unittest + + +class VenvSitePackagesLibraryTest(unittest.TestCase): + def test_imported_from_venv(self): + self.assertNotEqual(sys.prefix, sys.base_prefix, "Not running under a venv") + venv = sys.prefix + + from nspkg.subnspkg import alpha + + self.assertEqual(alpha.whoami, "alpha") + self.assertEqual(alpha.__name__, "nspkg.subnspkg.alpha") + + self.assertTrue( + alpha.__file__.startswith(sys.prefix), + f"\nalpha was imported, not from within the venv.\n" + + f"venv : {venv}\n" + + f"actual: {alpha.__file__}", + ) + + from nspkg.subnspkg import beta + + self.assertEqual(beta.whoami, "beta") + self.assertEqual(beta.__name__, "nspkg.subnspkg.beta") + self.assertTrue( + beta.__file__.startswith(sys.prefix), + f"\nbeta was imported, not from within the venv.\n" + + f"venv : {venv}\n" + + f"actual: {beta.__file__}", + ) + + +if __name__ == "__main__": + unittest.main() From 0ed66f833bd58f4b9c968b941e5477e6d595a581 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Sun, 16 Feb 2025 21:58:36 -0800 Subject: [PATCH 02/12] add docs for features.bzl --- docs/BUILD.bazel | 1 + python/BUILD.bazel | 3 +++ python/features.bzl | 43 +++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/docs/BUILD.bazel b/docs/BUILD.bazel index ea386f114a..71e2df8fc4 100644 --- a/docs/BUILD.bazel +++ b/docs/BUILD.bazel @@ -81,6 +81,7 @@ sphinx_stardocs( name = "bzl_api_docs", srcs = [ "//python:defs_bzl", + "//python:features_bzl", "//python:packaging_bzl", "//python:pip_bzl", "//python:py_binary_bzl", diff --git a/python/BUILD.bazel b/python/BUILD.bazel index c52e772666..a699c81cc4 100644 --- a/python/BUILD.bazel +++ b/python/BUILD.bazel @@ -79,6 +79,9 @@ bzl_library( bzl_library( name = "features_bzl", srcs = ["features.bzl"], + deps = [ + "@rules_python_internal//:rules_python_config_bzl", + ], ) bzl_library( diff --git a/python/features.bzl b/python/features.bzl index b1bd9cd0b5..f0c18bc40c 100644 --- a/python/features.bzl +++ b/python/features.bzl @@ -19,9 +19,48 @@ load("@rules_python_internal//:rules_python_config.bzl", "config") # See https://git-scm.com/docs/git-archive/2.29.0#Documentation/git-archive.txt-export-subst _VERSION_PRIVATE = "$Format:%(describe:tags=true)$" +def _features_typedef(): + """Information about features rules_python has implemented. + + ::::{field} precompile + :type: bool + + True if the precompile attributes are available. + :::{versionadded} TODO + ::: + :::: + + ::::{field} site_packages_root_attr + :type: bool + + True if the {obj}`site_packages_root` attribute is available. + + :::{versionadded} VERSION_NEXT_FEATURE + ::: + :::: + + ::::{field} uses_builtin_rules + :type: bool + + True if the rules are using the Bazel-builtin implementation. + + :::{versionadded} TODO + ::: + :::: + + ::::{field} version + :type: str + + The rules_python version. This is a semver format, e.g. `X.Y.Z` with + optional trailing `-rcN`. For unreleased versions, it is an empty string. + :::{versionadded} TODO + :::: + """ + features = struct( - version = _VERSION_PRIVATE if "$Format" not in _VERSION_PRIVATE else "", + TYPEDEF = _features_typedef, precompile = True, - uses_builtin_rules = not config.enable_pystar, site_packages_root_attr = True, + uses_builtin_rules = not config.enable_pystar, + version = _VERSION_PRIVATE if "$Format" not in _VERSION_PRIVATE else "", ) From 62207c37a1cd7ebfc03e1547a444b6d8c284b01b Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Mon, 17 Feb 2025 15:02:00 -0800 Subject: [PATCH 03/12] add versionadded info to features docs --- python/features.bzl | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/python/features.bzl b/python/features.bzl index f0c18bc40c..f5b1a0b2bb 100644 --- a/python/features.bzl +++ b/python/features.bzl @@ -26,7 +26,8 @@ def _features_typedef(): :type: bool True if the precompile attributes are available. - :::{versionadded} TODO + + :::{versionadded} 0.33.0 ::: :::: @@ -44,7 +45,7 @@ def _features_typedef(): True if the rules are using the Bazel-builtin implementation. - :::{versionadded} TODO + :::{versionadded} 1.1.0 ::: :::: @@ -53,7 +54,7 @@ def _features_typedef(): The rules_python version. This is a semver format, e.g. `X.Y.Z` with optional trailing `-rcN`. For unreleased versions, it is an empty string. - :::{versionadded} TODO + :::{versionadded} 0.38.0 :::: """ From 67872b789205ed00c842d12428eac09a9a2be0fa Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Sat, 22 Feb 2025 12:17:04 -0800 Subject: [PATCH 04/12] address review comments and finish some todos * fail if both imports and site_packages_root set * doc the topological ordering more * skip, not fail, for child-path symlinks * Also treat .pyi files as namespace anchors --- python/private/attributes.bzl | 17 +++++++--- python/private/common.bzl | 9 +++--- python/private/py_executable.bzl | 55 +++++++++++++++++++------------- python/private/py_library.bzl | 46 ++++++++++++++++++++------ 4 files changed, 86 insertions(+), 41 deletions(-) diff --git a/python/private/attributes.bzl b/python/private/attributes.bzl index bf2ccbaaca..d3ecfb6a4a 100644 --- a/python/private/attributes.bzl +++ b/python/private/attributes.bzl @@ -283,11 +283,7 @@ that depend on this rule. The strings are repo-runfiles-root relative, Absolute paths (paths that start with `/`) and paths that references a path above the execution root are not allowed and will result in an error. -:::{attention} -Setting both this and the {attr}`site_packages_root` attribute may result in -undefined behavior. Both will result in the code being importable, but from -different sys.path (and thus `__file__`) entries. -::: +This attribute is mutually exclusive with the {attr}`site_packages_root` attribute. """, ), } @@ -314,6 +310,17 @@ These are typically `py_library` rules. Targets that only provide data files used at runtime belong in the `data` attribute. + +:::{note} +The order of this list can matter because it affects the order that information +from dependencies is merged in, which can be relevant depending on the ordering +mode of depsets that are merged. + +* {obj}`PyInfo.site_packages_symlinks` uses topological ordering. + +See {obj}`PyInfo` for more information about the ordering of its depsets and +how its fields are merged. +::: """, ), "precompile": attr.string( diff --git a/python/private/common.bzl b/python/private/common.bzl index 12b120d16b..863d38d8ef 100644 --- a/python/private/common.bzl +++ b/python/private/common.bzl @@ -30,13 +30,14 @@ PackageSpecificationInfo = getattr(py_internal, "PackageSpecificationInfo", None # Extensions without the dot _PYTHON_SOURCE_EXTENSIONS = ["py"] -# Extensions that make a file considered importable +# Extensions that mean a file is relevant to Python PYTHON_FILE_EXTENSIONS = [ - "py", - "so", # Python C modules, usually Linux + "dll", # Python C modules, Windows specific "dylib", # Python C modules, Mac specific + "py", "pyc", - "dll", # Python C modules, Windows specific + "pyi", + "so", # Python C modules, usually Linux ] def create_binary_semantics_struct( diff --git a/python/private/py_executable.bzl b/python/private/py_executable.bzl index bfd7655b19..4733ce6b5f 100644 --- a/python/private/py_executable.bzl +++ b/python/private/py_executable.bzl @@ -624,6 +624,23 @@ def _create_site_packages_symlinks(ctx, site_packages): if PyInfo in dep ], ).to_list() + link_map = _build_link_map(entries) + + sp_files = [] + for sp_dir_path, link_to in link_map.items(): + sp_link = ctx.actions.declare_symlink(paths.join(site_packages, sp_dir_path)) + sp_link_rf_path = runfiles_root_path(ctx, sp_link.short_path) + rel_path = relative_path( + # dirname is necessary because a relative symlink is relative to + # the directory the symlink resides within. + from_ = paths.dirname(sp_link_rf_path), + to = link_to, + ) + ctx.actions.symlink(output = sp_link, target_path = rel_path) + sp_files.append(sp_link) + return sp_files + +def _build_link_map(entries): link_map = {} for link_to_runfiles_path, site_packages_path in entries: if site_packages_path in link_map: @@ -635,33 +652,27 @@ def _create_site_packages_symlinks(ctx, site_packages): # An empty link_to value means to not create the site package symlink. # Because of the topological ordering, this allows binaries to remove - # entries by having an earlier dependency produce empty link_to values + # entries by having an earlier dependency produce empty link_to values. for sp_dir_path, link_to in link_map.items(): if not link_to: link_map.pop(sp_dir_path) - # This is N^2; we can certainly do better by sorting and exploiting the - # order. - # A trailing slash is appended / to prevent /X matching /XY - sp_dirs = [x + "/" for x in link_map.keys()] - for search_for in sp_dirs: - for prefix in sp_dirs: - if search_for != prefix and search_for.startswith(prefix): - fail("sub-link: {} under {}", search_for, prefix) + # Remove entries that would be a child path of a created symlink. + # Earlier entries have precedence to match how exact matches are handled. + keep_link_map = {} + for _ in range(len(link_map)): + if not link_map: + break + dirname, value = link_map.popitem() + keep_link_map[dirname] = value - sp_files = [] - for sp_dir_path, link_to in link_map.items(): - sp_link = ctx.actions.declare_symlink(paths.join(site_packages, sp_dir_path)) - sp_link_rf_path = runfiles_root_path(ctx, sp_link.short_path) - rel_path = relative_path( - # dirname is necessary because a relative symlink is relative to - # the directory the symlink resides within. - from_ = paths.dirname(sp_link_rf_path), - to = link_to, - ) - ctx.actions.symlink(output = sp_link, target_path = rel_path) - sp_files.append(sp_link) - return sp_files + prefix = dirname + "/" # Add slash to prevent /X matching /XY + for maybe_suffix in link_map.keys(): + maybe_suffix += "/" # Add slash to prevent /X matching /XY + if maybe_suffix.startswith(prefix) or prefix.startswith(maybe_suffix): + link_map.pop(maybe_suffix) + + return keep_link_map def _map_each_identity(v): return v diff --git a/python/private/py_library.bzl b/python/private/py_library.bzl index 70c68a9b78..2eb3ecd24e 100644 --- a/python/private/py_library.bzl +++ b/python/private/py_library.bzl @@ -62,25 +62,40 @@ LIBRARY_ATTRS = union_attrs( doc = """ Package relative prefix to remove from `srcs` for site-packages layouts. +This attribute is mutually exclusive with the {attr}`imports` attribute. + When set, `srcs` are interpreted to have a file layout as if they were installed in site-packages. This attribute specifies the directory within `srcs` to treat as the site-packages root so the correct site-packages relative paths for the files can be computed. -For example, given `srcs=["site-packages/foo/bar.py"]`, specifying -`site_packages_root="site-packages/" means `foo/bar.py` is the file path -under the binary's venv site-packages directory that should be made availble. - :::{note} This string is relative to the target's *Bazel package*. e.g. Relative to the directory with the BUILD file that defines the target (the same as how e.g. `srcs`). ::: -:::{attention} -Setting both this an the {attr}`imports` attribute may result in undefined -behavior. Both will result in the code being importable, but from different -sys.path (and thus `__file__`) entries. +For example, given `srcs=["site-packages/foo/bar.py"]`, specifying +`site_packages_root="site-packages/" means `foo/bar.py` is the file path +under the binary's venv site-packages directory that should be made available. + +`__init__.py` files are treated specially to provide basic support for [implicit +namespace packages]( +https://packaging.python.org/en/latest/guides/packaging-namespace-packages/#native-namespace-packages). +However, the *content* of the files cannot be taken into account, merely their +presence or absense. Stated another way: [pkgutil-style namespace packages]( +https://packaging.python.org/en/latest/guides/packaging-namespace-packages/#pkgutil-style-namespace-packages) +won't be understood as namespace packages; they'll be seen as regular packages. This will +likely lead to conflicts with other targets that contribute to the namespace. + +:::{tip} +This attributes populates {obj}`PyInfo.site_packages_symlinks`, which is +a topologically ordered depset. This means dependencies closer and earlier +to a consumer have precedence. See {obj}`PyInfo.site_packages_symlinks` for +more information. +::: + +:::{versionadded} VERSION_NEXT_FEATURE ::: """, ), @@ -128,7 +143,18 @@ def py_library_impl(ctx, *, semantics): runfiles.add(collect_runfiles(ctx)) runfiles = runfiles.build(ctx) - site_packages_symlinks = _get_site_packages_symlinks(ctx) + imports = [] + site_packages_symlinks = [] + if ctx.attr.imports and ctx.attr.site_packages_root: + fail(("Only one of the `imports` or `site_packages_root` attributes " + + "can be set: site_packages_root={}, imports={}").format( + ctx.attr.site_packages_root, + ctx.attr.imports, + )) + elif ctx.attr.site_packages_root: + site_packages_symlinks = _get_site_packages_symlinks(ctx) + elif ctx.attr.imports: + imports = collect_imports(ctx, semantics) cc_info = semantics.get_cc_info_for_library(ctx) py_info, deps_transitive_sources, builtins_py_info = create_py_info( @@ -138,7 +164,7 @@ def py_library_impl(ctx, *, semantics): required_pyc_files = required_pyc_files, implicit_pyc_files = implicit_pyc_files, implicit_pyc_source_files = implicit_pyc_source_files, - imports = collect_imports(ctx, semantics), + imports = imports, site_packages_symlinks = site_packages_symlinks, ) From d4603a4d7ba6dc79229bfe82277ee72810cb459d Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Sat, 22 Feb 2025 14:15:06 -0800 Subject: [PATCH 05/12] add other repo for testing with workspace --- internal_dev_deps.bzl | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal_dev_deps.bzl b/internal_dev_deps.bzl index cd33475f43..87690be1ad 100644 --- a/internal_dev_deps.bzl +++ b/internal_dev_deps.bzl @@ -15,6 +15,7 @@ """Dependencies that are needed for development and testing of rules_python itself.""" load("@bazel_tools//tools/build_defs/repo:http.bzl", _http_archive = "http_archive", _http_file = "http_file") +load("@bazel_tools//tools/build_defs/repo:local.bzl", "local_repository") load("@bazel_tools//tools/build_defs/repo:utils.bzl", "maybe") load("//python/private:internal_config_repo.bzl", "internal_config_repo") # buildifier: disable=bzl-visibility @@ -42,6 +43,11 @@ def rules_python_internal_deps(): """ internal_config_repo(name = "rules_python_internal") + local_repository( + name = "other", + path = "tests/modules/other", + ) + http_archive( name = "bazel_skylib", sha256 = "bc283cdfcd526a52c3201279cda4bc298652efa898b10b4db0837dc51652756f", From d80fd7a3f9cdba1ded18b7d6038f8899c116713d Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Thu, 27 Mar 2025 21:45:58 -0700 Subject: [PATCH 06/12] add pypi flag --- python/config_settings/BUILD.bazel | 7 +++++++ python/private/enum.bzl | 20 ++++++++++++++++++++ python/private/flags.bzl | 24 +++--------------------- python/private/pypi/flags.bzl | 5 +++++ 4 files changed, 35 insertions(+), 21 deletions(-) diff --git a/python/config_settings/BUILD.bazel b/python/config_settings/BUILD.bazel index 796cf0c9c4..f54620157d 100644 --- a/python/config_settings/BUILD.bazel +++ b/python/config_settings/BUILD.bazel @@ -195,6 +195,13 @@ string_flag( visibility = ["//visibility:public"], ) +string_flag( + name = "pip_venv_site_packages", + build_setting_default = PipVenvSitePackages.NO, + # NOTE: Only public because it is used in pip hub repos. + visibility = ["//visibility:public"], +) + define_pypi_internal_flags( name = "define_pypi_internal_flags", ) diff --git a/python/private/enum.bzl b/python/private/enum.bzl index d71442e3b5..4d0fb10699 100644 --- a/python/private/enum.bzl +++ b/python/private/enum.bzl @@ -43,3 +43,23 @@ def enum(methods = {}, **kwargs): self = struct(__members__ = members, **kwargs) return self + +def _FlagEnum_flag_values(self): + return sorted(self.__members__.values()) + +def FlagEnum(**kwargs): + """Define an enum specialized for flags. + + Args: + **kwargs: members of the enum. + + Returns: + {type}`FlagEnum` struct. This is an enum with the following extras: + * `flag_values`: A function that returns a sorted list of the + flag values (enum `__members__`). Useful for passing to the + `values` attribute for string flags. + """ + return enum( + methods = dict(flag_values = _FlagEnum_flag_values), + **kwargs + ) diff --git a/python/private/flags.bzl b/python/private/flags.bzl index 1019faa8d6..4ddf5a44e2 100644 --- a/python/private/flags.bzl +++ b/python/private/flags.bzl @@ -19,27 +19,7 @@ unnecessary files when all that are needed are flag definitions. """ load("@bazel_skylib//rules:common_settings.bzl", "BuildSettingInfo") -load(":enum.bzl", "enum") - -def _FlagEnum_flag_values(self): - return sorted(self.__members__.values()) - -def FlagEnum(**kwargs): - """Define an enum specialized for flags. - - Args: - **kwargs: members of the enum. - - Returns: - {type}`FlagEnum` struct. This is an enum with the following extras: - * `flag_values`: A function that returns a sorted list of the - flag values (enum `__members__`). Useful for passing to the - `values` attribute for string flags. - """ - return enum( - methods = dict(flag_values = _FlagEnum_flag_values), - **kwargs - ) +load(":enum.bzl", "FlagEnum", "enum") def _AddSrcsToRunfilesFlag_is_enabled(ctx): value = ctx.attr._add_srcs_to_runfiles_flag[BuildSettingInfo].value @@ -138,6 +118,8 @@ VenvsUseDeclareSymlinkFlag = FlagEnum( get_value = _venvs_use_declare_symlink_flag_get_value, ) +VenvsCreateSitePackages + # Used for matching freethreaded toolchains and would have to be used in wheels # as well. # buildifier: disable=name-conventions diff --git a/python/private/pypi/flags.bzl b/python/private/pypi/flags.bzl index a25579a2b8..b2d3822638 100644 --- a/python/private/pypi/flags.bzl +++ b/python/private/pypi/flags.bzl @@ -44,6 +44,11 @@ UniversalWhlFlag = enum( UNIVERSAL = "universal", ) +PipVenvSitePackages = FlagEnum( + YES = "yes" + NO = "no", +) + _STRING_FLAGS = [ "dist", "whl_plat", From 546279347c61c05c18648d386d8f69bee3ee0498 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Mon, 31 Mar 2025 18:40:42 -0700 Subject: [PATCH 07/12] partially make logic hidden --- .../python/config_settings/index.md | 11 ++++ python/config_settings/BUILD.bazel | 5 +- python/private/flags.bzl | 6 +- python/private/py_library.bzl | 57 ++++++++++++++----- python/private/pypi/flags.bzl | 5 -- 5 files changed, 62 insertions(+), 22 deletions(-) diff --git a/docs/api/rules_python/python/config_settings/index.md b/docs/api/rules_python/python/config_settings/index.md index 79c7d0c109..1efb359673 100644 --- a/docs/api/rules_python/python/config_settings/index.md +++ b/docs/api/rules_python/python/config_settings/index.md @@ -213,6 +213,17 @@ Values: :::: +:::: + +:::{flag} venv_site_packages + +Determines if + +Values: +* `no` (default) +* `yes` +:::: + ::::{bzl:flag} bootstrap_impl Determine how programs implement their startup process. diff --git a/python/config_settings/BUILD.bazel b/python/config_settings/BUILD.bazel index f54620157d..57c6148675 100644 --- a/python/config_settings/BUILD.bazel +++ b/python/config_settings/BUILD.bazel @@ -9,6 +9,7 @@ load( "LibcFlag", "PrecompileFlag", "PrecompileSourceRetentionFlag", + "VenvSitePackages", "VenvsUseDeclareSymlinkFlag", ) load( @@ -196,8 +197,8 @@ string_flag( ) string_flag( - name = "pip_venv_site_packages", - build_setting_default = PipVenvSitePackages.NO, + name = "venv_site_packages", + build_setting_default = VenvSitePackages.NO, # NOTE: Only public because it is used in pip hub repos. visibility = ["//visibility:public"], ) diff --git a/python/private/flags.bzl b/python/private/flags.bzl index 4ddf5a44e2..7bc3c48c18 100644 --- a/python/private/flags.bzl +++ b/python/private/flags.bzl @@ -118,7 +118,11 @@ VenvsUseDeclareSymlinkFlag = FlagEnum( get_value = _venvs_use_declare_symlink_flag_get_value, ) -VenvsCreateSitePackages +# Decides if libraries +VenvSitePackages = FlagEnum( + YES = "yes", + NO = "no", +) # Used for matching freethreaded toolchains and would have to be used in wheels # as well. diff --git a/python/private/py_library.bzl b/python/private/py_library.bzl index 2fa235fb11..99e956c468 100644 --- a/python/private/py_library.bzl +++ b/python/private/py_library.bzl @@ -59,7 +59,19 @@ LIBRARY_ATTRS = dicts.add( PY_SRCS_ATTRS, IMPORTS_ATTRS, { - "site_packages_root": lambda: attrb.String( + "experimental_venv_site_packages": lambda: attrb.Bool( + doc = """ +Internal attribute. Should only be set by rules_python-internal code. + +:::{include} /_includes/experimental_api.md +::: + +If true, the library consults {flag}`//python/config_settings:venv_site_packages` +to decide if `srcs` is a site-packages relative layout. +""", + default = False, + ), + "XXsite_packages_root": lambda: attrb.String( doc = """ Package relative prefix to remove from `srcs` for site-packages layouts. @@ -156,16 +168,18 @@ def py_library_impl(ctx, *, semantics): imports = [] site_packages_symlinks = [] - if ctx.attr.imports and ctx.attr.site_packages_root: - fail(("Only one of the `imports` or `site_packages_root` attributes " + - "can be set: site_packages_root={}, imports={}").format( - ctx.attr.site_packages_root, - ctx.attr.imports, - )) - elif ctx.attr.site_packages_root: - site_packages_symlinks = _get_site_packages_symlinks(ctx) - elif ctx.attr.imports: - imports = collect_imports(ctx, semantics) + + ##if ctx.attr.imports and ctx.attr.site_packages_root: + ## fail(("Only one of the `imports` or `site_packages_root` attributes " + + ## "can be set: site_packages_root={}, imports={}").format( + ## ctx.attr.site_packages_root, + ## ctx.attr.imports, + ## )) + ##elif ctx.attr.site_packages_root: + ## site_packages_symlinks = _get_site_packages_symlinks(ctx) + ##elif ctx.attr.imports: + ## imports = collect_imports(ctx, semantics) + imports, site_packages_symlinks = _get_imports_and_site_packages_symlinks(ctx) cc_info = semantics.get_cc_info_for_library(ctx) py_info, deps_transitive_sources, builtins_py_info = create_py_info( @@ -214,9 +228,24 @@ Source files are no longer added to the runfiles directly. ::: """ +def _get_imports_and_site_packages_symlinks(ctx, semantics): + imports = depset() + site_packages_symlinks = depset() + if (ctx.attr.experimental_venv_site_packages and + ctx.attr._venv_site_packages_flag[BuildSettingInfo].value): + site_packages_symlinks = _get_site_packages_symlinks(ctx) + else: + imports = collect_imports(ctx, semantics) + return imports, site_packages_symlinks + def _get_site_packages_symlinks(ctx): - if not ctx.attr.site_packages_root: - return [] + imports = ctx.attr.imports + if len(imports) == 0: + fail("Must specify imports attr") + elif len(imports) > 1: + fail("Too many imports paths") + else: + site_packages_root = imports[0] # We have to build a list of (runfiles path, site-packages path) pairs of # the files to create in the consuming binary's venv site-packages directory. @@ -233,7 +262,7 @@ def _get_site_packages_symlinks(ctx): # directories that _do_ have an `__init__.py` file and treat those as # the path to symlink to. - site_packages_root = paths.join(ctx.label.package, ctx.attr.site_packages_root) + site_packages_root = paths.join(ctx.label.package, site_packages_root) repo_runfiles_dirname = None dirs_with_init = {} # dirname -> runfile path for src in ctx.files.srcs: diff --git a/python/private/pypi/flags.bzl b/python/private/pypi/flags.bzl index b2d3822638..a25579a2b8 100644 --- a/python/private/pypi/flags.bzl +++ b/python/private/pypi/flags.bzl @@ -44,11 +44,6 @@ UniversalWhlFlag = enum( UNIVERSAL = "universal", ) -PipVenvSitePackages = FlagEnum( - YES = "yes" - NO = "no", -) - _STRING_FLAGS = [ "dist", "whl_plat", From 81af833028a847e8d56523befaf1d0b51cb5f072 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Mon, 31 Mar 2025 19:03:29 -0700 Subject: [PATCH 08/12] switch to internal attr to trigger behavior --- CHANGELOG.md | 8 +-- docs/_includes/experimental_api.md | 5 ++ .../python/config_settings/index.md | 14 ++-- python/config_settings/BUILD.bazel | 6 +- python/features.bzl | 10 --- python/private/attributes.bzl | 2 - python/private/flags.bzl | 14 +++- python/private/py_info.bzl | 3 + python/private/py_library.bzl | 67 +++++++------------ python/private/pypi/whl_library_targets.bzl | 1 + tests/modules/other/nspkg_delta/BUILD.bazel | 3 +- tests/modules/other/nspkg_gamma/BUILD.bazel | 3 +- tests/support/sh_py_run_test.bzl | 4 ++ tests/venv_site_packages_libs/BUILD.bazel | 3 +- .../nspkg_alpha/BUILD.bazel | 3 +- .../nspkg_beta/BUILD.bazel | 3 +- 16 files changed, 78 insertions(+), 71 deletions(-) create mode 100644 docs/_includes/experimental_api.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 98c150df7b..4a498f0c8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -91,6 +91,10 @@ Unreleased changes template. please check the {obj}`uv.configure` tag class. * Add support for riscv64 linux platform. * (toolchains) Add python 3.13.2 and 3.12.9 toolchains +* (providers) (experimental) {obj}`PyInfo.site_packages_symlinks` field added to + allow specifying links to create within the venv site packages (only + applicable with {obj}`--bootstrap_impl=script`) + ([#2156](https://github.com/bazelbuild/rules_python/issues/2156)). {#v0-0-0-removed} ### Removed @@ -156,10 +160,6 @@ Unreleased changes template. which allows pass arguments to the interpreter before the regular args. * (rules) Added {obj}`main_module` attribute to `py_binary` and `py_test`, which allows specifying a module name to run (i.e. `python -m `). -* (providers) (experimental) {obj}`PyInfo.site_packages_symlinks` field added to - allow specifying links to create within the venv site packages (only - applicable with {obj}`--bootstrap_impl=script`) - ([#2156](https://github.com/bazelbuild/rules_python/issues/2156)). {#v1-3-0-removed} ### Removed diff --git a/docs/_includes/experimental_api.md b/docs/_includes/experimental_api.md new file mode 100644 index 0000000000..45473a7cbf --- /dev/null +++ b/docs/_includes/experimental_api.md @@ -0,0 +1,5 @@ +:::{warning} + +**Experimental API.** This API is still under development and may change or be +removed without notice. +::: diff --git a/docs/api/rules_python/python/config_settings/index.md b/docs/api/rules_python/python/config_settings/index.md index 1efb359673..340335d9b1 100644 --- a/docs/api/rules_python/python/config_settings/index.md +++ b/docs/api/rules_python/python/config_settings/index.md @@ -215,13 +215,19 @@ Values: :::: -:::{flag} venv_site_packages +:::{flag} venvs_site_packages + +Determines if libraries use a site-packages layout for their files. + +Note this flag only affects PyPI dependencies of `--bootstrap_impl=script` binaries + +:::{include} /_includes/experimental_api.md +::: -Determines if Values: -* `no` (default) -* `yes` +* `no` (default): Make libraries importable by adding to `sys.path` +* `yes`: Make libraries importable by creating paths in a binary's site-packages directory. :::: ::::{bzl:flag} bootstrap_impl diff --git a/python/config_settings/BUILD.bazel b/python/config_settings/BUILD.bazel index 57c6148675..45354e24d9 100644 --- a/python/config_settings/BUILD.bazel +++ b/python/config_settings/BUILD.bazel @@ -9,7 +9,7 @@ load( "LibcFlag", "PrecompileFlag", "PrecompileSourceRetentionFlag", - "VenvSitePackages", + "VenvsSitePackages", "VenvsUseDeclareSymlinkFlag", ) load( @@ -197,8 +197,8 @@ string_flag( ) string_flag( - name = "venv_site_packages", - build_setting_default = VenvSitePackages.NO, + name = "venvs_site_packages", + build_setting_default = VenvsSitePackages.NO, # NOTE: Only public because it is used in pip hub repos. visibility = ["//visibility:public"], ) diff --git a/python/features.bzl b/python/features.bzl index f5b1a0b2bb..59bf0b0693 100644 --- a/python/features.bzl +++ b/python/features.bzl @@ -31,15 +31,6 @@ def _features_typedef(): ::: :::: - ::::{field} site_packages_root_attr - :type: bool - - True if the {obj}`site_packages_root` attribute is available. - - :::{versionadded} VERSION_NEXT_FEATURE - ::: - :::: - ::::{field} uses_builtin_rules :type: bool @@ -61,7 +52,6 @@ def _features_typedef(): features = struct( TYPEDEF = _features_typedef, precompile = True, - site_packages_root_attr = True, uses_builtin_rules = not config.enable_pystar, version = _VERSION_PRIVATE if "$Format" not in _VERSION_PRIVATE else "", ) diff --git a/python/private/attributes.bzl b/python/private/attributes.bzl index bd2d596585..e8d69823e1 100644 --- a/python/private/attributes.bzl +++ b/python/private/attributes.bzl @@ -236,8 +236,6 @@ that depend on this rule. The strings are repo-runfiles-root relative, Absolute paths (paths that start with `/`) and paths that references a path above the execution root are not allowed and will result in an error. - -This attribute is mutually exclusive with the {attr}`site_packages_root` attribute. """, ), } diff --git a/python/private/flags.bzl b/python/private/flags.bzl index 7bc3c48c18..c53e4610ff 100644 --- a/python/private/flags.bzl +++ b/python/private/flags.bzl @@ -118,10 +118,20 @@ VenvsUseDeclareSymlinkFlag = FlagEnum( get_value = _venvs_use_declare_symlink_flag_get_value, ) -# Decides if libraries -VenvSitePackages = FlagEnum( +def _venvs_site_packages_is_enabled(ctx): + if not ctx.attr.experimental_venvs_site_packages: + return False + flag_value = ctx.attr.experimental_venvs_site_packages[BuildSettingInfo].value + return flag_value == VenvsSitePackages.YES + +# Decides if libraries try to use a site-packages layout using site_packages_symlinks +# buildifier: disable=name-conventions +VenvsSitePackages = FlagEnum( + # Use site_packages_symlinks YES = "yes", + # Don't use site_packages_symlinks NO = "no", + is_enabled = _venvs_site_packages_is_enabled, ) # Used for matching freethreaded toolchains and would have to be used in wheels diff --git a/python/private/py_info.bzl b/python/private/py_info.bzl index ae326278a9..4ecd02a438 100644 --- a/python/private/py_info.bzl +++ b/python/private/py_info.bzl @@ -158,6 +158,9 @@ Tuples of `(runfiles_path, site_packages_path)`. Where the code in `runfiles_path` available for import. Note that this is created as a "raw" symlink (via `declare_symlink`). +:::{include} /_includes/experimental_api.md +::: + :::{tip} The topological ordering means dependencies earlier and closer to the consumer have precedence. This allows e.g. a binary to add dependencies that override diff --git a/python/private/py_library.bzl b/python/private/py_library.bzl index 99e956c468..336bc1fb51 100644 --- a/python/private/py_library.bzl +++ b/python/private/py_library.bzl @@ -39,9 +39,8 @@ load( "filter_to_py_srcs", "get_imports", "runfiles_root_path", - "union_attrs", ) -load(":flags.bzl", "AddSrcsToRunfilesFlag", "PrecompileFlag") +load(":flags.bzl", "AddSrcsToRunfilesFlag", "PrecompileFlag", "VenvsSitePackages") load(":precompile.bzl", "maybe_precompile") load(":py_cc_link_params_info.bzl", "PyCcLinkParamsInfo") load(":py_internal.bzl", "py_internal") @@ -59,38 +58,25 @@ LIBRARY_ATTRS = dicts.add( PY_SRCS_ATTRS, IMPORTS_ATTRS, { - "experimental_venv_site_packages": lambda: attrb.Bool( + "experimental_venvs_site_packages": lambda: attrb.Label( doc = """ -Internal attribute. Should only be set by rules_python-internal code. +**INTERNAL ATTRIBUTE. SHOULD ONLY BE SET BY rules_python-INTERNAL CODE.** :::{include} /_includes/experimental_api.md ::: -If true, the library consults {flag}`//python/config_settings:venv_site_packages` -to decide if `srcs` is a site-packages relative layout. -""", - default = False, - ), - "XXsite_packages_root": lambda: attrb.String( - doc = """ -Package relative prefix to remove from `srcs` for site-packages layouts. - -This attribute is mutually exclusive with the {attr}`imports` attribute. +A flag that decides whether the library should treat its sources as a +site-packages layout. -When set, `srcs` are interpreted to have a file layout as if they were installed -in site-packages. This attribute specifies the directory within `srcs` to treat -as the site-packages root so the correct site-packages relative paths for -the files can be computed. +When the flag is `yes`, then the `srcs` files are treated as a site-packages +layout that is relative to the `imports` attribute. The `imports` attribute +can have only a single element. It is a repo-relative runfiles path. -:::{note} -This string is relative to the target's *Bazel package*. e.g. Relative to the -directory with the BUILD file that defines the target (the same as how e.g. -`srcs`). -::: - -For example, given `srcs=["site-packages/foo/bar.py"]`, specifying -`site_packages_root="site-packages/" means `foo/bar.py` is the file path -under the binary's venv site-packages directory that should be made available. +For example, in the `my/pkg/BUILD.bazel` file, given +`srcs=["site-packages/foo/bar.py"]`, specifying +`imports=["my/pkg/site-packages"]` means `foo/bar.py` is the file path +under the binary's venv site-packages directory that should be made available (i.e. +`import foo.bar` will work). `__init__.py` files are treated specially to provide basic support for [implicit namespace packages]( @@ -169,17 +155,7 @@ def py_library_impl(ctx, *, semantics): imports = [] site_packages_symlinks = [] - ##if ctx.attr.imports and ctx.attr.site_packages_root: - ## fail(("Only one of the `imports` or `site_packages_root` attributes " + - ## "can be set: site_packages_root={}, imports={}").format( - ## ctx.attr.site_packages_root, - ## ctx.attr.imports, - ## )) - ##elif ctx.attr.site_packages_root: - ## site_packages_symlinks = _get_site_packages_symlinks(ctx) - ##elif ctx.attr.imports: - ## imports = collect_imports(ctx, semantics) - imports, site_packages_symlinks = _get_imports_and_site_packages_symlinks(ctx) + imports, site_packages_symlinks = _get_imports_and_site_packages_symlinks(ctx, semantics) cc_info = semantics.get_cc_info_for_library(ctx) py_info, deps_transitive_sources, builtins_py_info = create_py_info( @@ -231,8 +207,7 @@ Source files are no longer added to the runfiles directly. def _get_imports_and_site_packages_symlinks(ctx, semantics): imports = depset() site_packages_symlinks = depset() - if (ctx.attr.experimental_venv_site_packages and - ctx.attr._venv_site_packages_flag[BuildSettingInfo].value): + if VenvsSitePackages.is_enabled(ctx): site_packages_symlinks = _get_site_packages_symlinks(ctx) else: imports = collect_imports(ctx, semantics) @@ -247,6 +222,14 @@ def _get_site_packages_symlinks(ctx): else: site_packages_root = imports[0] + if site_packages_root.endswith("/"): + fail("should not end in slash") + if site_packages_root.startswith("/"): + fail("cannot start with slash") + + # Append slash to prevent incorrectly prefix-string matches + site_packages_root += "/" + # We have to build a list of (runfiles path, site-packages path) pairs of # the files to create in the consuming binary's venv site-packages directory. # To minimize the number of files to create, we just return the paths @@ -262,7 +245,7 @@ def _get_site_packages_symlinks(ctx): # directories that _do_ have an `__init__.py` file and treat those as # the path to symlink to. - site_packages_root = paths.join(ctx.label.package, site_packages_root) + ##site_packages_root = paths.join(ctx.label.package, site_packages_root) repo_runfiles_dirname = None dirs_with_init = {} # dirname -> runfile path for src in ctx.files.srcs: @@ -302,6 +285,8 @@ def _get_site_packages_symlinks(ctx): paths.join(repo_runfiles_dirname, site_packages_root, dirname), dirname, )) + if not site_packages_symlinks: + fail("empty?", ctx.label, site_packages_root, ctx.files.srcs[0]) return site_packages_symlinks def _repo_relative_short_path(short_path): diff --git a/python/private/pypi/whl_library_targets.bzl b/python/private/pypi/whl_library_targets.bzl index c390da2613..95031e6181 100644 --- a/python/private/pypi/whl_library_targets.bzl +++ b/python/private/pypi/whl_library_targets.bzl @@ -266,6 +266,7 @@ def whl_library_targets( ), tags = tags, visibility = impl_vis, + experimental_venvs_site_packages = Label("@rules_python//python/config_settings:venvs_site_packages"), ) def _config_settings(dependencies_by_platform, native = native, **kwargs): diff --git a/tests/modules/other/nspkg_delta/BUILD.bazel b/tests/modules/other/nspkg_delta/BUILD.bazel index e3b005e4b2..457033aacf 100644 --- a/tests/modules/other/nspkg_delta/BUILD.bazel +++ b/tests/modules/other/nspkg_delta/BUILD.bazel @@ -5,5 +5,6 @@ package(default_visibility = ["//visibility:public"]) py_library( name = "nspkg_delta", srcs = glob(["site-packages/**/*.py"]), - site_packages_root = "site-packages/", + experimental_venvs_site_packages = "@rules_python//python/config_settings:venvs_site_packages", + imports = [package_name() + "/site-packages"], ) diff --git a/tests/modules/other/nspkg_gamma/BUILD.bazel b/tests/modules/other/nspkg_gamma/BUILD.bazel index dbbd7a7c04..89038e80d2 100644 --- a/tests/modules/other/nspkg_gamma/BUILD.bazel +++ b/tests/modules/other/nspkg_gamma/BUILD.bazel @@ -5,5 +5,6 @@ package(default_visibility = ["//visibility:public"]) py_library( name = "nspkg_gamma", srcs = glob(["site-packages/**/*.py"]), - site_packages_root = "site-packages/", + experimental_venvs_site_packages = "@rules_python//python/config_settings:venvs_site_packages", + imports = [package_name() + "/site-packages"], ) diff --git a/tests/support/sh_py_run_test.bzl b/tests/support/sh_py_run_test.bzl index 7b3b617da1..9c8134ff40 100644 --- a/tests/support/sh_py_run_test.bzl +++ b/tests/support/sh_py_run_test.bzl @@ -40,6 +40,8 @@ def _perform_transition_impl(input_settings, attr, base_impl): settings["//python/bin:python_src"] = attr.python_src if attr.venvs_use_declare_symlink: settings["//python/config_settings:venvs_use_declare_symlink"] = attr.venvs_use_declare_symlink + if attr.venvs_site_packages: + settings["//python/config_settings:venvs_site_packages"] = attr.venvs_site_packages return settings _RECONFIG_INPUTS = [ @@ -47,6 +49,7 @@ _RECONFIG_INPUTS = [ "//python/bin:python_src", "//command_line_option:extra_toolchains", "//python/config_settings:venvs_use_declare_symlink", + "//python/config_settings:venvs_site_packages", ] _RECONFIG_OUTPUTS = _RECONFIG_INPUTS + [ "//command_line_option:build_python_zip", @@ -67,6 +70,7 @@ toolchain. """, ), "python_src": attrb.Label(), + "venvs_site_packages": attrb.String(), "venvs_use_declare_symlink": attrb.String(), } diff --git a/tests/venv_site_packages_libs/BUILD.bazel b/tests/venv_site_packages_libs/BUILD.bazel index 22b7d9fa97..5d02708800 100644 --- a/tests/venv_site_packages_libs/BUILD.bazel +++ b/tests/venv_site_packages_libs/BUILD.bazel @@ -2,11 +2,12 @@ load("//tests/support:sh_py_run_test.bzl", "py_reconfig_test") load("//tests/support:support.bzl", "SUPPORTS_BOOTSTRAP_SCRIPT") py_reconfig_test( - name = "venv_site_packages_libs_test", + name = "venvs_site_packages_libs_test", srcs = ["bin.py"], bootstrap_impl = "script", main = "bin.py", target_compatible_with = SUPPORTS_BOOTSTRAP_SCRIPT, + venvs_site_packages = "yes", deps = [ "//tests/venv_site_packages_libs/nspkg_alpha", "//tests/venv_site_packages_libs/nspkg_beta", diff --git a/tests/venv_site_packages_libs/nspkg_alpha/BUILD.bazel b/tests/venv_site_packages_libs/nspkg_alpha/BUILD.bazel index 2a53839048..c40c3b4080 100644 --- a/tests/venv_site_packages_libs/nspkg_alpha/BUILD.bazel +++ b/tests/venv_site_packages_libs/nspkg_alpha/BUILD.bazel @@ -5,5 +5,6 @@ package(default_visibility = ["//visibility:public"]) py_library( name = "nspkg_alpha", srcs = glob(["site-packages/**/*.py"]), - site_packages_root = "site-packages/", + experimental_venvs_site_packages = "//python/config_settings:venvs_site_packages", + imports = [package_name() + "/site-packages"], ) diff --git a/tests/venv_site_packages_libs/nspkg_beta/BUILD.bazel b/tests/venv_site_packages_libs/nspkg_beta/BUILD.bazel index b0bfdfceba..5d402183bd 100644 --- a/tests/venv_site_packages_libs/nspkg_beta/BUILD.bazel +++ b/tests/venv_site_packages_libs/nspkg_beta/BUILD.bazel @@ -5,5 +5,6 @@ package(default_visibility = ["//visibility:public"]) py_library( name = "nspkg_beta", srcs = glob(["site-packages/**/*.py"]), - site_packages_root = "site-packages/", + experimental_venvs_site_packages = "//python/config_settings:venvs_site_packages", + imports = [package_name() + "/site-packages"], ) From 619af6ec761de217f77f1404b673b613cd7f0007 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Mon, 31 Mar 2025 23:58:29 -0700 Subject: [PATCH 09/12] cleanup --- python/private/py_library.bzl | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/python/private/py_library.bzl b/python/private/py_library.bzl index 336bc1fb51..418fb06bbf 100644 --- a/python/private/py_library.bzl +++ b/python/private/py_library.bzl @@ -216,16 +216,20 @@ def _get_imports_and_site_packages_symlinks(ctx, semantics): def _get_site_packages_symlinks(ctx): imports = ctx.attr.imports if len(imports) == 0: - fail("Must specify imports attr") + fail("When venvs_site_packages is enabled, exactly one `imports` " + + "value must be specified, got 0") elif len(imports) > 1: - fail("Too many imports paths") + fail("When venvs_site_packages is enabled, exactly one `imports` " + + "value must be specified, got {}".format(imports)) else: site_packages_root = imports[0] if site_packages_root.endswith("/"): - fail("should not end in slash") + fail("The site packages root value from `imports` cannot end in " + + "slash, got {}".format(site_packages_root)) if site_packages_root.startswith("/"): - fail("cannot start with slash") + fail("The site packages root value from `imports` cannot start with " + + "slash, got {}".format(site_packages_root)) # Append slash to prevent incorrectly prefix-string matches site_packages_root += "/" @@ -245,7 +249,6 @@ def _get_site_packages_symlinks(ctx): # directories that _do_ have an `__init__.py` file and treat those as # the path to symlink to. - ##site_packages_root = paths.join(ctx.label.package, site_packages_root) repo_runfiles_dirname = None dirs_with_init = {} # dirname -> runfile path for src in ctx.files.srcs: @@ -257,8 +260,10 @@ def _get_site_packages_symlinks(ctx): path = path.removeprefix(site_packages_root) dir_name, _, filename = path.rpartition("/") if not dir_name: - # This would be e.g. `site-packages/__init__.py`, which isn't valid. - # Apparently, the pypi integration adds such a file? + # This would be e.g. `site-packages/__init__.py`, which isn't valid + # because it's not within a directory for an importable Python package. + # However, the pypi integration over-eagerly adds a pkgutil-style + # __init__.py file during the repo phase. Just ignore them for now. continue if filename.startswith("__init__."): @@ -285,12 +290,12 @@ def _get_site_packages_symlinks(ctx): paths.join(repo_runfiles_dirname, site_packages_root, dirname), dirname, )) - if not site_packages_symlinks: - fail("empty?", ctx.label, site_packages_root, ctx.files.srcs[0]) return site_packages_symlinks def _repo_relative_short_path(short_path): + # Convert `../+pypi+foo/some/file.py` to `some/file.py` if short_path.startswith("../"): + fail(short_path) return short_path[3:].partition("/")[2] else: return short_path From e2d749cece7cd1db5d0804e314419724804b8a76 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Tue, 1 Apr 2025 00:05:34 -0700 Subject: [PATCH 10/12] add features flag --- python/features.bzl | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/python/features.bzl b/python/features.bzl index 59bf0b0693..8edfb698fc 100644 --- a/python/features.bzl +++ b/python/features.bzl @@ -31,6 +31,14 @@ def _features_typedef(): ::: :::: + ::::{field} py_info_site_packages_symlinks + + True if the `PyInfo.site_packages_symlinks` field is available. + + :::{versionadded} VERSION_NEXT_FEATURE + ::: + :::: + ::::{field} uses_builtin_rules :type: bool @@ -51,7 +59,9 @@ def _features_typedef(): features = struct( TYPEDEF = _features_typedef, + # keep sorted precompile = True, + py_info_site_packages_symlinks = True, uses_builtin_rules = not config.enable_pystar, version = _VERSION_PRIVATE if "$Format" not in _VERSION_PRIVATE else "", ) From 1e3ccbebf38f446c21d258ac38184fecbe4102a8 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Sat, 5 Apr 2025 08:47:43 -0700 Subject: [PATCH 11/12] remove debugging fail --- python/private/py_library.bzl | 1 - 1 file changed, 1 deletion(-) diff --git a/python/private/py_library.bzl b/python/private/py_library.bzl index 418fb06bbf..edd0db579f 100644 --- a/python/private/py_library.bzl +++ b/python/private/py_library.bzl @@ -295,7 +295,6 @@ def _get_site_packages_symlinks(ctx): def _repo_relative_short_path(short_path): # Convert `../+pypi+foo/some/file.py` to `some/file.py` if short_path.startswith("../"): - fail(short_path) return short_path[3:].partition("/")[2] else: return short_path From 083e02985e05a9420cce69079c2f28fdcfe9c3c8 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Sat, 5 Apr 2025 09:18:25 -0700 Subject: [PATCH 12/12] fix test --- tests/pypi/whl_library_targets/whl_library_targets_tests.bzl | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/pypi/whl_library_targets/whl_library_targets_tests.bzl b/tests/pypi/whl_library_targets/whl_library_targets_tests.bzl index a042ed0346..f738e03b5d 100644 --- a/tests/pypi/whl_library_targets/whl_library_targets_tests.bzl +++ b/tests/pypi/whl_library_targets/whl_library_targets_tests.bzl @@ -273,6 +273,7 @@ def _test_whl_and_library_deps(env): ), "tags": ["tag1", "tag2"], "visibility": ["//visibility:public"], + "experimental_venvs_site_packages": Label("//python/config_settings:venvs_site_packages"), }, ]) # buildifier: @unsorted-dict-items @@ -335,6 +336,7 @@ def _test_group(env): }), "tags": [], "visibility": ["@pypi__groups//:__pkg__"], + "experimental_venvs_site_packages": Label("//python/config_settings:venvs_site_packages"), }, ]) # buildifier: @unsorted-dict-items