-
Notifications
You must be signed in to change notification settings - Fork 548
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
fix(gazelle): ensure that gazelle helper modules are on PYTHONPATH #1590
Conversation
Before this change there was a bug in how the parsing helpers were being used in case we were using Python 3.11 toolchain, which is using a more strict version of the entrypoint template. This change moves the python code to a different location to ensure that the top level package is something more unique than just "python" and updates the non-bzlmod tests to run under 3.11. We also change ".bazelrc" to use explicit "__init__.py" definition to avoid non-reproducible errors in the future. Fixes #1589
This means that we are replicating the setup that would be used by our users more closely. Note, that there is a seemingly benign error message: ```console $ bazel run //:gazelle ... INFO: Running command line: bazel-bin/gazelle gazelle: .../rules_python/gazelle/python/lifecycle.go:26:3: pattern helper.zip: matched no files ```
@rickeylev, what do you think about cherry-picking this into a |
7d67276
to
0ba7c9c
Compare
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.
cherry-picking this into a 0.27.1 release?
+1, I agree
import sys | ||
|
||
# NOTE @aignas 2023-12-02: Use absolute imports with respect to WORKSPACE root. |
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.
It would be better to not rely on the repo root being added to sys.path -- that's a behavior I plan to disable once the Starlark impl can be enabled.
But, I can understand this is just a bugfix CL, so this is fine for now. Is Gazelle able to under PyInfo.imports, or have some equivalent?
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.
This made me realize that I just needed to use imports = ["."]
on the py_binary
that is being used here to fix the error and no moving of files is actually needed. Thanks!
This reverts commit 75aa9cc. This is optional
The build is failing on Windows with:
|
…1590) Before this change there was a bug in how the parsing helpers were being used in case we were using Python 3.11 toolchain, which is using a more strict version of the entrypoint template. This change adds `imports = ["."]` to ensure that the gazelle helper components are on PYTHONPATH and updates the non-bzlmod tests to run under 3.11. We also: * Change `.bazelrc` to use explicit `__init__.py` definition to avoid non-reproducible errors in the future. * Add a dedicated `gazelle_binary` that uses `DEFAULT_LANGUAGES` *and* `//python`. Fixes #1589
…bazelbuild#1527) Bazel at head enables bzlmod by default, but the requirements.bzl entry_point functions aren't supported under bzlmod. Until workspace support is entirely dropped, explicitly disable bzlmod for the pip_repository_entry_points test. Work towards bazelbuild#1590
Before this change there was a bug in how the parsing helpers were being
used in case we were using Python 3.11 toolchain, which is using a more
strict version of the entrypoint template. This change adds
imports = ["."]
to ensure that the gazelle helper components are on PYTHONPATH and updates
the non-bzlmod tests to run under 3.11.
We also:
.bazelrc
to use explicit__init__.py
definition to avoidnon-reproducible errors in the future.
gazelle_binary
that usesDEFAULT_LANGUAGES
and//python
.Fixes #1589