-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[google-cloud-cpp] Add recipe for Google Cloud Services #5424
Conversation
Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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 my comment will break... but it's a minor detail
✔️
This comment has been minimized.
This comment has been minimized.
All green in build 19 (
|
if self.options.shared: | ||
del self.options.fPIC | ||
|
||
if self.settings.os == 'Windows' and self.options.shared: |
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.
If this configuration is not working, why adding set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON)
in CMakeLists.txt?
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.
Well... I tried to too, and probably it is possible, but without a local Windows to test and try I gave up. Also, it will probably build for older VS versions... Someone will take care in another PR if needed 😊
def package_info(self): | ||
# Using componen google-cloud-common, so it reduces the risk of conflict (pkgconfig generator) |
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.
Looking at installed pkgconfig files, pkg_config files are not properly emulated. This component google-cloud-common
could have been called common
with pkgconfig name google_cloud_cpp_common
.
CMake targets seem ok, but generators can't emulate official config files, and google-cloud-cpp generates more than one config file.
Actually, there is no google-cloud-cpp-config.cmake
file upstream.
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 find your suggestion much more convenient. From the consumer perspective, I prefer to use google-cloud-cpp::common
over google-cloud-cpp::google-cloud-common
to refer to that component. I think it is worth a PR and probably a suggestion to follow for other PRs, wdyt @prince-chrismc ?
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.
From the CMake perspective google-cloud-cpp::common
should be working since we added those names 🤔 both names are currently available AFAIK.
My original comment stems for all the times Space flagged this potential issue, I did not check the pkg_config log but we could have done the inverse as is suggested. So I have no objections here.
Instead of changing the component name we could simple just add the corrected name? Avoid changing it's usage in the recipe
self.cpp_info.components["google-cloud-common"].names["pkg_config"] = "google_cloud_cpp_common"
I assume they did not use "common" since it was too common xD
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.
My comment was about Conan components, not related to CMake targets. From a consumer package you will refer to that component as google-cloud-cpp::google-cloud-common
(now). I'm voting to changing it back to google-cloud-cpp::common
.
CMake targets are not changed. Pkgconfig would require the line you suggested.
Specify library name and version: google-cloud-cpp/1.26.1
Pushing this recipe, so we can use C++ to create GCP microservices... for ConanCenter itself 🤔
This recipe requires:
ExternalProject_Add
here:googleapis
, using it will require patching these sources, and it doesn't offer tags/releases. I'll add that that reference later.Also, I will rebase/squash this PR once dependencies are merged.
close #5212