-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
googleapis - dynamically build cmake project #11651
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 83d2b2fgoogleapis/cci.20210730
|
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 9f9748egoogleapis/cci.20210730
|
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 359ecb0googleapis/cci.20210730
|
This comment has been minimized.
This comment has been minimized.
# Tweaks | ||
# - Inconvenient macro names from usr/include/sys/syslimits.h in some macOS SDKs. | ||
# Patched here: https://github.com/protocolbuffers/protobuf/commit/f138d5de2535eb7dd7c8d0ad5eb16d128ab221fd | ||
if tools.Version(self.deps_cpp_info["protobuf"].version) <= "3.21.2" and self.settings.os == "Macos": |
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.
Probably just for some macOS versions...
self.build_requires('protobuf/3.21.1') | ||
# CMake >= 3.20 is required. There is a proto with dots in the name 'k8s.min.proto' and CMake fails to generate project files | ||
if not self._cmake_new_enough: | ||
self.build_requires('cmake/3.23.2') |
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.
Set unconditionally and simplify?
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 thats a fair choice, it's fairly complicated code
Hooks produced the following warnings for commit 2e866e1googleapis/cci.20210730
|
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.
Please, @SpaceIm , I would appreciate your review here. My next steps are to prepare Thanks |
This comment has been minimized.
This comment has been minimized.
All green in build 28 (
|
if self.settings.compiler == "gcc" and tools.Version(self.settings.compiler.version) <= "5": | ||
raise ConanInvalidConfiguration("Build with GCC 5 fails") | ||
|
||
if self.settings.compiler in ["Visual Studio", "msvc"] 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.
should we use the new tools helper?
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.
Two nits but no blockers
I found myself trapped here (#11624 (comment)), it was not possible to use different versions of
googleapis
for different versions ofgoogle-cloud-cpp
libraries because of the different package id computed forgrpc
after the override.One alternative would be to heavily patch the build-system files of
google-cloud-cpp
to work with any version ofgoogleapis
(and keep building the protos inside), alternatively, we can keep patching thegoogle-cloud-cpp
but to use precompiled binaries ofgoogleapis
.The second approach is probably more sound (and CC-friendly) as both libraries
grpc
andgoogle-cloud-cpp
can link to the same binaries (instead of building the same protos twice) and probably the ABI ofgoogleapis
is more compatible than the build-system files that are hardcoded ingoogle-cloud-cpp
(these files depend on the protos)Here I'm making a proposal, probably someone has a better idea (and less hacky).
PS1.- Tried using Bazel, but I feel like entering the rabbit hole... maybe someone with more experience using it...
Some TODOs:
protobuf