-
-
Notifications
You must be signed in to change notification settings - Fork 620
Make py_console_script_binary
compatible with symbolic macros
#3195
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
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.
Thank you for the contribution!
- Is there any way we can add a test for this?
- Could you please add a
CHANGELOG.md
note to let people know that this fixes an issue?
c91459a
to
d3d11ba
Compare
@aignas thank you for your review! sure, i am looking into it ATM. in the dev-guide i read that integration tests are discuraged. in c91459a i quickly tried to add a simpler build test, however that failed in CI, presumably because bazel 7 doesnt support symbolic macros. so do you think in this case it would be okay to add a new integration test and ignore (if possible) bazel 7 tests there? |
|
||
if kwargs.pop("srcs", None): | ||
fail("passing 'srcs' attribute to py_console_script_binary is unsupported") | ||
|
||
py_console_script_gen( | ||
name = "_{}_gen".format(name), | ||
name = name + "_gen", |
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 is making the target as foo_gen
instead of _foo_gen
that it previously was.
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.
Yes, this naming change is intentional. The underscore prefix violates Bazel's symbolic macro naming rules. Per the documentation [1], targets created by a symbolic macro must be named either the same as the macro or prefixed by the macro name followed by _, ., or -. The new naming satisfies this requirement.
[1] https://bazel.build/extending/macros - "Naming conventions for targets created"
@@ -73,13 +73,13 @@ def py_console_script_binary( | |||
Defaults to empty string. | |||
**kwargs: Extra parameters forwarded to `binary_rule`. | |||
""" | |||
main = "rules_python_entry_point_{}.py".format(name) | |||
main = name + "_entry_point.py" |
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.
Just out of curiosity, could you please explain why you've selected to use this?
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, too, is required for symbolic macro compatibility. The original .format()
approach created filenames that also violated the naming rules [1].
[1] https://bazel.build/extending/macros - "Naming conventions for targets created"
Fixes target naming to follow symbolic macro conventions introduced in Bazel 8.0.