-
Notifications
You must be signed in to change notification settings - Fork 366
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
fix(builds): require grpc >= 1.65.4 for async_throughput_benchmark.cc #14751
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14751 +/- ##
==========================================
- Coverage 93.40% 93.39% -0.01%
==========================================
Files 2316 2316
Lines 207863 207863
==========================================
- Hits 194147 194132 -15
- Misses 13716 13731 +15 ☔ View full report in Codecov by Sentry. |
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.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @ddelgrosso1)
google/cloud/storage/benchmarks/async_throughput_benchmark.cc
line 17 at r1 (raw file):
#include "google/cloud/internal/port_platform.h" // TODO(#14750): Remove this guard once vcpkg has a newer version of grpc #if GRPC_CPP_VERSION_MAJOR >= 1 && GRPC_CPP_VERSION_MINOR >= 65 && \
I would keep the old and new if
statements separate.
google/cloud/storage/benchmarks/async_throughput_benchmark.cc
line 18 at r1 (raw file):
// TODO(#14750): Remove this guard once vcpkg has a newer version of grpc #if GRPC_CPP_VERSION_MAJOR >= 1 && GRPC_CPP_VERSION_MINOR >= 65 && \ GRPC_CPP_VERSION_PATCH >= 4 && GOOGLE_CLOUD_CPP_STORAGE_HAVE_GRPC && \
The version check is going to have to be more complicated to account for something like version 1.66.1. Something like:
#if GRPC_CPP_VERSION_MAJOR > 1 || \
GRPC_CPP_VERSION_MAJOR == 1 && GRPC_CPP_VERSION_MINOR > 65 || \
GRPC_CPP_VERSION_MAJOR == 1 && GRPC_CPP_VERSION_MINOR == 65 && GRPC_CPP_VERSION_PATCH >= 4
bb2db04
to
7c5e4cd
Compare
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.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @scotthart)
google/cloud/storage/benchmarks/async_throughput_benchmark.cc
line 17 at r1 (raw file):
Previously, scotthart (Scott Hart) wrote…
I would keep the old and new
if
statements separate.
Done.
google/cloud/storage/benchmarks/async_throughput_benchmark.cc
line 18 at r1 (raw file):
Previously, scotthart (Scott Hart) wrote…
The version check is going to have to be more complicated to account for something like version 1.66.1. Something like:
#if GRPC_CPP_VERSION_MAJOR > 1 || \ GRPC_CPP_VERSION_MAJOR == 1 && GRPC_CPP_VERSION_MINOR > 65 || \ GRPC_CPP_VERSION_MAJOR == 1 && GRPC_CPP_VERSION_MINOR == 65 && GRPC_CPP_VERSION_PATCH >= 4
Done.
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.
Reviewed 1 of 2 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ddelgrosso1)
This change is