-
-
Notifications
You must be signed in to change notification settings - Fork 593
refactor: reimplement writing namespace pkgs in Starlark #2882
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This is basically a reimplementation of https://github.com/bazel-contrib/rules_python/blob/main/python/private/pypi/whl_installer/namespace_pkgs.py right? Mostly LGTM. Is it handling the "# The root of the directory should never be an implicit namespace" condition?
I think you mean: "if pipstar is enabled, don't run the python version" This doesn't have to be tied to pipstar. It can just be moved to be part of the pip-generated BUILD files. e.g.
|
54136c5
to
98827e5
Compare
Ran: $ bazel build //docs --@rules_python//python/config_settings:venvs_site_packages=yes
INFO: Analyzed target //docs:docs (0 packages loaded, 0 targets configured).
ERROR: /home/aignas/src/github/aignas/rules_python/docs/BUILD.bazel:45:12: Sphinx building html for //docs:docs failed: (Exit 2): sphinx-build failed: error executing SphinxBuildDocs command (from target //docs:docs) bazel-out/k8-opt-exec-ST-d57f47055a04/bin/docs/sphinx-build --show-traceback --builder html --quiet --jobs auto --fresh-env --write-all bazel-out/k8-fastbuild/bin/docs/_docs/_sources ... (remaining 1 argument skipped)
Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
Traceback (most recent call last):
File "/home/aignas/.cache/bazel/_bazel_aignas/6f0de8c9128ee8d5dbf27ba6dcc48bdd/sandbox/linux-sandbox/6/execroot/_main/bazel-out/k8-opt-exec-ST-d57f47055a04/bin/docs/sphinx-build.runfiles/_main/docs/_sphinx-build.venv/lib/python3.11/site-packages/sphinx/cmd/build.py", line 496, in build_main
app = Sphinx(
^^^^^^^
File "/home/aignas/.cache/bazel/_bazel_aignas/6f0de8c9128ee8d5dbf27ba6dcc48bdd/sandbox/linux-sandbox/6/execroot/_main/bazel-out/k8-opt-exec-ST-d57f47055a04/bin/docs/sphinx-build.runfiles/_main/docs/_sphinx-build.venv/lib/python3.11/site-packages/sphinx/application.py", line 256, in __init__
self.setup_extension(extension)
File "/home/aignas/.cache/bazel/_bazel_aignas/6f0de8c9128ee8d5dbf27ba6dcc48bdd/sandbox/linux-sandbox/6/execroot/_main/bazel-out/k8-opt-exec-ST-d57f47055a04/bin/docs/sphinx-build.runfiles/_main/docs/_sphinx-build.venv/lib/python3.11/site-packages/sphinx/application.py", line 437, in setup_extension
self.registry.load_extension(self, extname)
File "/home/aignas/.cache/bazel/_bazel_aignas/6f0de8c9128ee8d5dbf27ba6dcc48bdd/sandbox/linux-sandbox/6/execroot/_main/bazel-out/k8-opt-exec-ST-d57f47055a04/bin/docs/sphinx-build.runfiles/_main/docs/_sphinx-build.venv/lib/python3.11/site-packages/sphinx/registry.py", line 466, in load_extension
metadata = setup(app)
^^^^^^^^^^
File "/home/aignas/.cache/bazel/_bazel_aignas/6f0de8c9128ee8d5dbf27ba6dcc48bdd/sandbox/linux-sandbox/6/execroot/_main/bazel-out/k8-opt-exec-ST-d57f47055a04/bin/docs/sphinx-build.runfiles/_main/docs/_sphinx-build.venv/lib/python3.11/site-packages/autodoc2/__init__.py", line 8, in setup
from .sphinx.extension import setup as _setup
File "/home/aignas/.cache/bazel/_bazel_aignas/6f0de8c9128ee8d5dbf27ba6dcc48bdd/sandbox/linux-sandbox/6/execroot/_main/bazel-out/k8-opt-exec-ST-d57f47055a04/bin/docs/sphinx-build.runfiles/_main/docs/_sphinx-build.venv/lib/python3.11/site-packages/autodoc2/sphinx/extension.py", line 18, in <module>
from autodoc2.sphinx.autodoc import AutodocObject
File "/home/aignas/.cache/bazel/_bazel_aignas/6f0de8c9128ee8d5dbf27ba6dcc48bdd/sandbox/linux-sandbox/6/execroot/_main/bazel-out/k8-opt-exec-ST-d57f47055a04/bin/docs/sphinx-build.runfiles/_main/docs/_sphinx-build.venv/lib/python3.11/site-packages/autodoc2/sphinx/autodoc.py", line 12, in <module>
from autodoc2.sphinx.utils import (
File "/home/aignas/.cache/bazel/_bazel_aignas/6f0de8c9128ee8d5dbf27ba6dcc48bdd/sandbox/linux-sandbox/6/execroot/_main/bazel-out/k8-opt-exec-ST-d57f47055a04/bin/docs/sphinx-build.runfiles/_main/docs/_sphinx-build.venv/lib/python3.11/site-packages/autodoc2/sphinx/utils.py", line 15, in <module>
from autodoc2.utils import WarningSubtypes
File "/home/aignas/.cache/bazel/_bazel_aignas/6f0de8c9128ee8d5dbf27ba6dcc48bdd/sandbox/linux-sandbox/6/execroot/_main/bazel-out/k8-opt-exec-ST-d57f47055a04/bin/docs/sphinx-build.runfiles/_main/docs/_sphinx-build.venv/lib/python3.11/site-packages/autodoc2/utils.py", line 10, in <module>
from typing_extensions import Required
ModuleNotFoundError: No module named 'typing_extensions'
Exception occurred:
File "/home/aignas/.cache/bazel/_bazel_aignas/6f0de8c9128ee8d5dbf27ba6dcc48bdd/sandbox/linux-sandbox/6/execroot/_main/bazel-out/k8-opt-exec-ST-d57f47055a04/bin/docs/sphinx-build.runfiles/_main/docs/_sphinx-build.venv/lib/python3.11/site-packages/autodoc2/utils.py", line 10, in <module>
from typing_extensions import Required
ModuleNotFoundError: No module named 'typing_extensions'
The full traceback has been saved in /tmp/sphinx-err-twbss_8e.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.
A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!
Target //docs:docs failed to build
Use --verbose_failures to see the command lines of failed build steps. I checked the
And I think the problem is that the |
98827e5
to
4f1798b
Compare
As found in bazel-contrib#2882 testing, packages like `typing-extensions` which have `.py` files at the root of the `site-packages` folder don't work and it seems that the comment about `rules_python` being too eager is only half-correct. Since `namespace_pkgs` are no longer there, we can just include all of the files and if there are collisions, they will be highlighted as build errors. Now the following works: ``` bazel build //docs --@rules_python//python/config_settings:venvs_site_packages=yes ``` Stacked on bazel-contrib#2882. Work towards bazel-contrib#2156
With this PR I would like to facilitate the implementation of the venv layouts because we can in theory take the srcs and the data within the py_library and then use the expand_template to write the extra Python files if the namespace_pkgs flag is enabled. The old Python code has been removed and the extra generated files are written out with bazel_skylib write_file as a POC. This could be optimized by writing once and then doing symlinks to the original instead of writing the files, but I am not sure how this would behave, maybe fine? The implicit namespace_pkg init files are included to py_library if the site-packages config flag is set to false and I think this may help with continuing the implementation. NOTE, it seems that the pycross code that we have is using the namespace_pkg Python code, so that will be removed later in one PR. Work towards #2156
bb150a7
to
4ed19e0
Compare
As found in bazel-contrib#2882 testing, packages like `typing-extensions` which have `.py` files at the root of the `site-packages` folder don't work and it seems that the comment about `rules_python` being too eager is only half-correct. Since `namespace_pkgs` are no longer there, we can just include all of the files and if there are collisions, they will be highlighted as build errors. Now the following works: ``` bazel build //docs --@rules_python//python/config_settings:venvs_site_packages=yes ``` Stacked on bazel-contrib#2882. Work towards bazel-contrib#2156
@@ -217,6 +217,15 @@ string_flag( | |||
visibility = ["//visibility:public"], | |||
) | |||
|
|||
config_setting( | |||
name = "is_venvs_site_packages", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use an internal name, or factor out a define_internal_flags() function (similar to what pypi).
@@ -0,0 +1,2 @@ | |||
# __path__ manipulation added by bazel-contrib/rules_python to support namespace pkgs. | |||
__path__ = __import__("pkgutil").extend_path(__path__, __name__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name implies it's a template, but I don't see any template variable names?
|
||
load("@bazel_skylib//rules:copy_file.bzl", "copy_file") | ||
|
||
_ext = struct( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.dylib (mac) and .dll (windows) are additional extension module filename extensions
Namespace packages can be created in one of three ways. They are detailed here: | ||
https://packaging.python.org/guides/packaging-namespace-packages/#creating-a-namespace-package | ||
|
||
'pkgutil-style namespace packages' (2) and 'pkg_resources-style namespace packages' (3) works in Bazel, but | ||
'native namespace packages' (1) do not. | ||
|
||
We ensure compatibility with Bazel of method 1 by converting them into method 2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please preserve this comment somewhere (namespace_pkgs.bzl ?). The link and why this legacy method is used were really helpful when I came across this code.
@@ -389,6 +386,8 @@ def _whl_library_impl(rctx): | |||
metadata_name = metadata.name, | |||
metadata_version = metadata.version, | |||
requires_dist = metadata.requires_dist, | |||
# TODO @aignas 2025-05-17: maybe have a build flag for this instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless there's a commitment to resolve a TODO, leave them out.
(IMHO: no build flag needed)
# TODO @aignas 2025-05-17: maybe have a build flag for this instead |
@@ -457,6 +456,8 @@ def _whl_library_impl(rctx): | |||
name = whl_path.basename, | |||
dep_template = rctx.attr.dep_template or "@{}{{name}}//:{{target}}".format(rctx.attr.repo_prefix), | |||
entry_points = entry_points, | |||
# TODO @aignas 2025-05-17: maybe have a build flag for this instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# TODO @aignas 2025-05-17: maybe have a build flag for this instead |
As found in #2882 testing, packages like `typing-extensions` which have `.py` files at the root of the `site-packages` folder don't work and it seems that the comment about `rules_python` being too eager is only half-correct. Since `namespace_pkgs` are no longer there, we can just include all of the files and if there are collisions, they will be highlighted as build errors. Now the following works: ``` bazel build //docs --@rules_python//python/config_settings:venvs_site_packages=yes ``` Work towards #2156
With this PR I would like to facilitate the implementation of the venv
layouts because we can in theory take the
srcs
and thedata
withinthe
py_library
and then use theexpand_template
to write the extraPython files if the namespace_pkgs flag is enabled.
The old Python code has been removed and the extra generated files are
written out with
bazel_skylib
copy_file
.The implicit
namespace_pkg
init files are included topy_library
if the
site-packages
config flag is set to false and I think thismay help with continuing the implementation, but it currently is still
not working as expected (see comment).
Work towards #2156