-
-
Notifications
You must be signed in to change notification settings - Fork 639
build: Starlarkify python flags #3334
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
dougthor42
left a comment
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.
One gazelle-specific comment, otherwise the Gazelle stuff LGTM.
I'll leave final approval to Richard or Aignas
aignas
left a comment
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.
Could you please add a note in Changes section of the changelog to note that bazel_skylib has been upgraded to 1.8.2? See the previous mentions for bazel_skylib for an example.
rickeylev
left a comment
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.
impl mostly looks good, though the logic for checking python_path i'm not sure of. Various nits about project-local styles.
|
@gemini-code-assist /review |
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.
Code Review
This pull request continues the effort to migrate native Python flags to Starlark flags, which is a great improvement for the maintainability and future-proofing of rules_python. The changes introduce Starlark flags for --python_path, --build_python_zip, and --incompatible_default_to_explicit_init_py, and correctly update the transition logic to support both native and Starlark versions during the migration period. The upgrade of bazel_skylib to support the scope = universal attribute is also a necessary and well-executed change.
The implementation is solid and covers all the required aspects, including updating tests and examples. I have a couple of minor suggestions to improve the usability of the new flags by adding help text and making an error message more descriptive. Overall, this is a high-quality contribution.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
Just commenting for other readers since I encountered this while upgrading This change means If you're using an older version of |
Add starlark flags for
--python_path,--build_python_zipand--incompatible_default_to_explicit_init_py.--build_python_zipis changed tobooleaninstead ofTristatewith default value set toTrueonwindowsandFalseotherwise.scope = universalattribute is added to the starlark flags so they can be propagated to exec config on Bazel 9. This required upgradingbazel_skylibversion to havescopeattribute defined.Work towards: #3252
cc @gregestren