Define toolchain to override PackageMetadataInfo
#75
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @Yannic, 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 introduces a new toolchain mechanism designed to provide greater flexibility in managing PackageMetadataInfo. By defining new providers and a dedicated rule, it enables users to override package metadata for specific package groups, facilitating more dynamic and configurable build processes without altering the original package definitions.
Highlights
- New Providers for Metadata Overrides: Introduced
PackageMetadataOverrideInfoandPackageMetadataToolchainInfoto enable a structured way of overriding package metadata for specific sets of packages. - New Toolchain Rule: Added the
package_metadata_toolchainrule, which allows users to declare and configure thePackageMetadataToolchainInfofor their build environment. - Enhanced Documentation: Comprehensive documentation has been added for the new providers and the toolchain rule, ensuring clarity on their usage and purpose.
- BUILD File Refactoring: The
BUILDfiles responsible for generating documentation and managing internal dependencies have been refactored for improved maintainability and conciseness.
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.
Code Review
This pull request introduces a new toolchain to override PackageMetadataInfo, which is a valuable addition. However, the current implementation has a couple of critical issues that render the feature unusable: a required module is not loaded, and there is no mechanism for users to create the necessary override providers. Additionally, I've found several typos and documentation gaps that should be addressed to improve clarity and usability. My detailed feedback is in the comments below.
|
|
||
| return [ | ||
| info, | ||
| platform_common.ToolchainInfo( |
There was a problem hiding this comment.
platform_common is used here but it is not loaded. This will cause a build error. You need to add load("@bazel_tools//core:platform_common.bzl", "platform_common") at the top of the file.
| "metadata_overrides": attr.label_list( | ||
| mandatory = False, | ||
| providers = [ | ||
| PackageMetadataOverrideInfo, | ||
| ], | ||
| doc = """ | ||
| """.strip(), | ||
| ), |
There was a problem hiding this comment.
The metadata_overrides attribute expects a list of targets that provide PackageMetadataOverrideInfo. However, this pull request does not introduce any rule that can create and return a PackageMetadataOverrideInfo provider. This makes the entire override mechanism unusable for end-users.
A new rule, for example package_metadata_override, is needed. This rule would likely take packages (e.g., a package_group target label) and metadata (e.g., a package_metadata target label) as attributes, and instantiate/return a PackageMetadataOverrideInfo provider.
| def package_metadata_toolchain( | ||
| # Disallow unnamed attributes. | ||
| *, | ||
| # `_package_metadata_toolchain` attributes. | ||
| name, | ||
| # Common attributes (subset since this target is non-configurable). | ||
| visibility = None): | ||
| _package_metadata_toolchain( | ||
| # `_package_metadata_toolchain` attributes. | ||
| name = name, | ||
|
|
||
| # Common attributes. | ||
| visibility = visibility, | ||
| applicable_licenses = [], | ||
| ) |
There was a problem hiding this comment.
The package_metadata_toolchain macro does not accept a metadata_overrides attribute. This prevents users from providing any overrides to the toolchain. You should add metadata_overrides to the macro's signature and pass it to the underlying _package_metadata_toolchain rule.
Note that this is related to my other comment about the lack of a rule to produce PackageMetadataOverrideInfo. Both issues need to be addressed for this feature to be usable.
| def package_metadata_toolchain( | |
| # Disallow unnamed attributes. | |
| *, | |
| # `_package_metadata_toolchain` attributes. | |
| name, | |
| # Common attributes (subset since this target is non-configurable). | |
| visibility = None): | |
| _package_metadata_toolchain( | |
| # `_package_metadata_toolchain` attributes. | |
| name = name, | |
| # Common attributes. | |
| visibility = visibility, | |
| applicable_licenses = [], | |
| ) | |
| def package_metadata_toolchain( | |
| # Disallow unnamed attributes. | |
| *, | |
| # `_package_metadata_toolchain` attributes. | |
| name, | |
| metadata_overrides = [], | |
| # Common attributes (subset since this target is non-configurable). | |
| visibility = None): | |
| _package_metadata_toolchain( | |
| # `_package_metadata_toolchain` attributes. | |
| name = name, | |
| metadata_overrides = metadata_overrides, | |
| # Common attributes. | |
| visibility = visibility, | |
| applicable_licenses = [], | |
| ) |
|
|
||
| Defines an override for `PackageMetadataInfo` for a set of packages. | ||
|
|
||
| > **Fields in this provider are not covered by the stability gurantee.** |
There was a problem hiding this comment.
There is a typo in "gurantee". It should be "guarantee". This typo also appears in other new documentation and docstrings.
| > **Fields in this provider are not covered by the stability gurantee.** | |
| > **Fields in this provider are not covered by the stability guarantee.** |
| | <a id="package_metadata_toolchain-name"></a>name | <p align="center"> - </p> | none | | ||
| | <a id="package_metadata_toolchain-visibility"></a>visibility | <p align="center"> - </p> | `None` | |
There was a problem hiding this comment.
The descriptions for the name and visibility attributes are unhelpful placeholders (-). Please provide meaningful descriptions for these common attributes to improve documentation clarity. For example, for name: "A unique name for this target."
|
|
||
| Defines an override for `PackageMetadataInfo` for a set of packages. | ||
|
|
||
| > **Fields in this provider are not covered by the stability gurantee.** |
There was a problem hiding this comment.
There is a typo in "gurantee". It should be "guarantee".
| > **Fields in this provider are not covered by the stability gurantee.** | |
| > **Fields in this provider are not covered by the stability guarantee.** |
|
|
||
| Toolchain for `package_metadata`. | ||
|
|
||
| > **Fields in this provider are not covered by the stability gurantee.** |
There was a problem hiding this comment.
There is a typo in "gurantee". It should be "guarantee".
| > **Fields in this provider are not covered by the stability gurantee.** | |
| > **Fields in this provider are not covered by the stability guarantee.** |
| | <a id="package_metadata_toolchain-name"></a>name | <p align="center"> - </p> | none | | ||
| | <a id="package_metadata_toolchain-visibility"></a>visibility | <p align="center"> - </p> | `None` | |
There was a problem hiding this comment.
The descriptions for the name and visibility attributes are unhelpful placeholders (-). Please provide meaningful descriptions for these common attributes to improve documentation clarity. For example, for name: "A unique name for this target."
| doc = """ | ||
| Defines an override for `PackageMetadataInfo` for a set of packages. | ||
| > **Fields in this provider are not covered by the stability gurantee.** |
There was a problem hiding this comment.
There is a typo in the docstring: "gurantee" should be "guarantee".
| doc = """ | ||
| Toolchain for `package_metadata`. | ||
| > **Fields in this provider are not covered by the stability gurantee.** |
There was a problem hiding this comment.
There is a typo in the docstring: "gurantee" should be "guarantee".
No description provided.