Conversation
Signed-off-by: Appu Goundan <appu@google.com>
![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 @loosebazooka, 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 updates the project's Bazel configuration to utilize rules_cc for building C/C++ binaries, moving away from the native.cc_binary rule. This change aims to leverage the dedicated rules_cc repository for C/C++ build logic, potentially offering more features or better maintainability.
Highlights
- Dependency Update: Added rules_cc as a new Bazel dependency in MODULE.bazel to enable its use across the project.
- C/C++ Rule Migration: Modified private/oci/cc_image.bzl to import cc_binary from @rules_cc and replace the native.cc_binary call with the new cc_binary rule.
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 migrates from using native.cc_binary to cc_binary from rules_cc by adding rules_cc as a dependency. This is a good move towards using modern C++ rules in Bazel. My review includes a suggestion to improve the cc_image macro to make it more robust by ensuring the C++ binary is built for the correct target platform, preventing potential runtime errors.
| def cc_image(name, srcs, base): | ||
| native.cc_binary( | ||
| cc_binary( | ||
| name = "%s_binary" % name, | ||
| srcs = srcs, | ||
| ) |
There was a problem hiding this comment.
The cc_binary target should be constrained to build for the correct platform of the OCI image. Without this, bazel build might build the binary for the host platform, which could be different from the target image's platform, leading to a runtime exec format error.
To make this macro more robust and ensure correctness, I suggest adding target_compatible_with to the cc_image signature and passing it to cc_binary. This allows callers to specify platform constraints.
For example, callers could then specify target_compatible_with = ["@platforms//os:linux", "@platforms//cpu:x86_64"] for an amd64 linux image.
I've also updated the string formatting to use simple concatenation, which is generally more readable than the % operator for this case. You could also consider passing **kwargs to allow for more cc_binary attributes to be configured.
| def cc_image(name, srcs, base): | |
| native.cc_binary( | |
| cc_binary( | |
| name = "%s_binary" % name, | |
| srcs = srcs, | |
| ) | |
| def cc_image(name, srcs, base, target_compatible_with = None): | |
| cc_binary( | |
| name = name + "_binary", | |
| srcs = srcs, | |
| target_compatible_with = target_compatible_with, | |
| ) |
|
🌳 🔄 Image Check |
2 participants
instead of native-cc rules which appear to be unable to find
ldon my machine