fix: redirect entrypoints to py_console_script_binary #2063
Conversation
There was a problem hiding this comment.
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.
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.
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.
SGTM. Didn't know about it. Will FIXUP.
There was a problem hiding this comment.
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.
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?
|
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-infodirectories, which originate from built distributions (wheels).