-
Notifications
You must be signed in to change notification settings - Fork 70
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
build: switch yacl examples to bzlmod #495
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.
Hello @tongke6, 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!
Summary of Changes
This pull request focuses on modernizing the YACL examples by switching to Bzlmod for managing external dependencies. This involves migrating from the WORKSPACE
file to MODULE.bazel
for dependency declarations, updating Bazel configurations, and upgrading the Bazel version. Additionally, some documentation and example code paths are updated to reflect the new module structure.
Highlights
- Bzlmod Migration: The primary change is the switch to Bzlmod for managing external dependencies in the YACL examples. This replaces the traditional
WORKSPACE
file withMODULE.bazel
, providing a more modern and scalable approach to dependency management. - Bazel Version Upgrade: The Bazel version used by the examples is upgraded from 7.4.1 to 7.5.0, ensuring compatibility with the latest features and improvements in Bazel.
- Documentation Updates: Documentation and example code paths are updated to reflect the new module structure, ensuring that users can easily find and use the examples.
- Bazel Configuration: The
.bazelrc
file is updated to include registry settings, and other configurations are updated to be compatible with Bzlmod.
Changelog
Click here to see the changelog
- .bazeliskrc
- Upgraded Bazel version from 7.4.1 to 7.5.0.
- docs/locale/zh_CN/LC_MESSAGES/index.po
- Added translations for index.rst.
- docs/locale/zh_CN/LC_MESSAGES/src/develop/example_psi.po
- Added translations for example_psi.rst.
- docs/locale/zh_CN/LC_MESSAGES/src/develop/example_psi_java.po
- Added translations for example_psi_java.rst.
- docs/locale/zh_CN/LC_MESSAGES/src/getting_started.po
- Added translations for GETTING_STARTED.md.
- docs/locale/zh_CN/LC_MESSAGES/src/research/index.po
- Added translations for research/index.rst.
- docs/src/develop/example_psi.rst
- Updated bazel test command to reflect new module structure:
cd examples && bazel test //psi/cpp:ecdh_psi_test
. - Updated bazel build command to reflect new module structure:
bazel build //psi/cpp:ecdh_psi_main -c opt
. - Updated executable paths to reflect new module structure:
./bazel-bin/psi/cpp/ecdh_psi_main --rank=0
and./bazel-bin/psi/cpp/ecdh_psi_main --rank=1
.
- Updated bazel test command to reflect new module structure:
- examples/.bazelrc
- Added registry settings for external dependencies.
- examples/MODULE.bazel
- Migrated from WORKSPACE to MODULE.bazel for dependency management.
- Declared dependencies on yacl, pybind11_bazel, rules_jni, rules_python, spdlog, abseil-cpp, and googletest.
- examples/MODULE.bazel.lock
- Generated lock file for Bzlmod dependencies.
- examples/WORKSPACE
- Removed WORKSPACE file as dependencies are now managed by MODULE.bazel.
- examples/psi/java/BUILD.bazel
- Updated bazel build command to reflect new module structure:
bazel build //psi/java:LibEcdhPsi_deploy.jar
.
- Updated bazel build command to reflect new module structure:
- examples/repositories.bzl
- Removed repositories.bzl file as dependencies are now managed by MODULE.bazel.
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 is currently in preview and 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 to provide feedback.
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.
Trivia time!
What is the primary purpose of a Bazel MODULE.bazel file?
Click here for the answer
The primary purpose of a Bazel MODULE.bazel file is to declare the external dependencies of a Bazel project when using Bzlmod. It replaces the traditional WORKSPACE file for dependency management.
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
The pull request switches the YACL examples to use Bzlmod for managing external dependencies. This is a good step towards modernizing the build system. The changes look generally correct, but I have a few comments and suggestions for improvement.
Summary of Findings
- Inconsistent Bazel target references: The Bazel target references in the documentation are inconsistent. Some refer to the old
//examples/psi/...
style, while others use the new//psi/cpp:...
style. This needs to be consistent. - Missing registry entry: The MODULE.bazel.lock file is missing an entry for the openssl module from the bcr registry.
Merge Readiness
The pull request introduces significant changes to the build system. While the changes appear to be generally correct, the inconsistencies in Bazel target references in the documentation and the missing registry entry should be addressed before merging. I am unable to directly approve this pull request, and users should have others review and approve this code before merging.
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
fix #494