-
-
Notifications
You must be signed in to change notification settings - Fork 636
feat: allow registering arbitrary settings for py_binary transitions #3248
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
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.
Summary of Changes
Hello @rickeylev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the configuration capabilities of rules_python by allowing py_binary, py_test, and py_wheel rules to apply arbitrary build settings on a per-target basis. This feature addresses common use cases such as managing multiple PyPI dependency closures, overriding flags for py_wheel builds, and setting specific configurations like bootstrap_impl or venv site packages for individual targets. The changes involve a new module extension for registering custom settings, a new attribute on the rules themselves, and internal refactoring to support this flexible configuration mechanism.
Highlights
- Arbitrary Setting Transitions: Introduces the ability for
py_binary,py_test, andpy_wheelrules to transition on arbitrary build settings, enabling more flexible configuration per target. - New
config_settingsAttribute: Adds aconfig_settingsattribute to executable Python rules, allowing users to specify custom flag values for individual targets without needing to wrap them inwith_cfg. - Module Extension for Custom Settings: Implements a new
rules_python_configmodule extension with anadd_transition_settingtag, providing a bzlmod API for users to register their own custom settings for transitions. - Enhanced Default Transitions: Most existing
rules_pythonconfiguration settings are now automatically included in the default transition inputs and outputs forpy_binary,py_test, andpy_wheel. - Multi-PyPI Dependency Management Guide: Adds new documentation demonstrating how to manage common dependencies across multiple PyPI versions using the new
config_settingsfeature and custom flags. - Code Refactoring and Cleanup: Refactors and removes defunct
py_reconfigcode fromsh_py_run_test.bzl, consolidating it into a dedicatedpy_reconfig.bzlfile for better organization. - Standardized Label Handling: Introduces new utility functions and files (
common_labels.bzl,transition_labels.bzl) to standardize and normalize the handling of build setting labels used in transitions.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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 introduces a powerful feature allowing users to register arbitrary settings for py_binary, py_test, and py_wheel to transition on. This is a significant improvement for managing complex dependency graphs, especially in monorepos with multiple PyPI dependency sets. The implementation is well-structured, introducing a new bzlmod extension, a repository rule to generate configuration, and refactoring existing rules to use this new mechanism. The addition of tests and documentation is also commendable. My feedback is mostly focused on minor corrections in the documentation to ensure clarity for users.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…into pypi.flag
|
/gemini 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 introduces a powerful feature allowing users to register arbitrary settings for py_binary, py_test, and py_wheel transitions. The implementation is well-structured, introducing a new bzlmod extension //python/extensions:config.bzl and a config_settings attribute on the relevant rules. The changes are accompanied by thorough tests and new documentation, which is excellent.
My review focuses on a few minor documentation and docstring improvements to enhance clarity and correctness. Overall, this is a solid contribution that addresses several important use cases for rules_python users.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
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.
LGTM in general.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…into pypi.flag
…into pypi.flag
This implements the ability for users to add additional settings that py_binary, py_test,
and py_wheel can transition on.
There were three main use cases motivating this feature:
py_wheel.site packages, etc.
It also adds most of our config settings to the the transition inputs/outputs for those
rules, which allows users to per-target force particular settings without having to
use e.g.
with_cfgto wrap a target with the desired transition settings. It alsolets use avoid adding dozens of attributes (one per setting); today there are
about 17 flags.
Under the hood, this works by having a bzlmod api that users can pass labels to. These
labels are put into a generated bzl file, which the rules load and add to their
list of transition inputs/outputs. On the target level, the
config_settingsattribute,which is a
dict[label, str], can be set to change the particular flags of interest.Along the way...