-
Notifications
You must be signed in to change notification settings - Fork 547
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: redirect entrypoints to py_console_script_binary #2063
base: main
Are you sure you want to change the base?
Conversation
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.
In general, not against moving this under private/pypi, but I am not sure if having a single pip.bzl file where we are loading everything from is great. Having a separate file per macro/function allows for much better isolation and cache hits.
What is more, this is a build rule and quite a few things in the pip.bzl are repo rules, except for the compile requirements.
@@ -13,6 +13,8 @@ | |||
# limitations under the License. | |||
|
|||
load("@bazel_skylib//:bzl_library.bzl", "bzl_library") | |||
load("@rules_python//python:defs.bzl", "py_library") |
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 drop rules_python in the load statements in internal code.
|
||
def _compatibility_shim(**kwargs): | ||
# buildifier: disable=print | ||
print("Please use `py_console_script_binary` from `//python:pip_bzl` 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.
I saw people using deprecation attribute in some cases instead of a print (this is in py_proto_library case). That may be less spammy, as this will cause one print invocation per usage of the rule, I think?
Maybe it would be useful to print a buildozer command for replacing the loads?
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.
SGTM. Didn't know about it. Will FIXUP.
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.
For macros, I think there's a doc string syntax that stardoc parses out into a "deprecation" doc text. I think it's either """DEPRECATED: blabla """
or """[DEPRECATED]: blalba"""
? Not entirely sure.
I'm pretty sure our docgen respects it when stardoc emits it.
|
||
```{include} /_includes/py_console_script_binary.md | ||
``` | ||
Please use `py_console_script_binary` from `//python:pip_bzl` 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.
Nit, I think this should be pip.bzl instead of pip_bzl.
Maybe it would be great to write the full load statement to help users?
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.
SGTM. Will FIXUP.
Yeah, I'll echo much of what Ignas said:
I'm -1 on moving the public symbol simply because it seems like needless churn. I guess if you really want to move it, OK. But I think we should keep the old location around for longer than typical because people are still upgrading from workspace->bzlmod, and if they don't have a stable load() path for the entry point replacements, it'll be pretty annoying to go through upgrading. I'm fine with moving the implementation under python/private/pypi. |
@groodt, do you plan (have time) to clean this up before 1.0? |
entrypoints
(originally from setuptools) AKAconsole_scripts
(as defined in pyproject.toml) AKApy_console_scripts
(as defined in rules_python) are a packaging standard.It therefore makes sense to package this code alongside the other packaging rules code and not the python language rules code. The packaging rules code currently sits inside
pypi
. Looking at the code, we can see that it is packaging related because it extracts the contents of.dist-info
directories, which originate from built distributions (wheels).