-
Notifications
You must be signed in to change notification settings - Fork 628
[email protected] #6945
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
base: main
Are you sure you want to change the base?
[email protected] #6945
Conversation
|
Hello @pawbhard, @rishesh007, @markdroth, @sergiitk, @yuanweiz, @asheshvidyut, @sreenithi, modules you maintain (grpc) have been updated in this PR. |
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 version 1.78.0-pre1 for the grpc module. The changes are generally well-structured for a new module version in the Bazel Central Registry. However, there are a couple of areas that need improvement. The MODULE.bazel file uses an http_archive for a Python dependency, which goes against bzlmod best practices and should be managed through pip. Additionally, the presubmit.yml configuration is missing a build target for the Windows platform, leading to incomplete test coverage. Addressing these points will improve the maintainability and robustness of this module.
| http_archive( | ||
| name = "typing_extensions", | ||
| build_file = "//third_party:typing_extensions.BUILD", | ||
| sha256 = "bf6f56b36d8bc9156e518eb1cc37a146284082fa53522033f772aefbecfd15fc", | ||
| strip_prefix = "typing_extensions-4.12.2", | ||
| url = "https://github.com/python/typing_extensions/archive/4.12.2.tar.gz", | ||
| ) |
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.
The use of http_archive to fetch typing_extensions is discouraged in bzlmod. This creates a private dependency for the grpc module, which can lead to issues for downstream users if any part of grpc's public API relies on it. Python dependencies should be managed via pip.parse from rules_python to ensure they are properly integrated into the Bazel dependency graph. Please consider adding typing_extensions to your requirements.bazel.lock file and removing this http_archive declaration.
| - "@grpc//:grpcpp_admin" | ||
| - "@grpc//:grpcpp_channelz" | ||
| - "@grpc//:grpcpp_csds" | ||
| - "@grpc//:grpcpp_orca_service" |
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.
The verify_targets_on_windows task is missing the @grpc//examples/protos/... build target, which is present in the other verify_targets tasks. For consistency and to ensure full build coverage across all platforms, this target should also be included for Windows if it's expected to build there.
- "@grpc//:grpcpp_orca_service"
- "@grpc//examples/protos/..."|
@bazel-io skip_check unstable_url |
|
@bazel-io skip_check incompatible_flags |
Release: https://github.com/grpc/grpc/releases/tag/v1.78.0-pre1
Automated by Publish to BCR