-
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
protobuf: allow shared on Macos + add protobuf::protoc imported target #4776
protobuf: allow shared on Macos + add protobuf::protoc imported target #4776
Conversation
previous injection of lib directories was too fragile. It breaks as soon as there is a dependency
I detected other pull requests that are modifying protobuf/all recipe: This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there. |
Some configurations of 'protobuf/3.9.1' failed in build 1 (
|
recipes/protobuf/all/conanfile.py
Outdated
@@ -48,20 +42,23 @@ def configure(self): | |||
if self.options.shared: | |||
del self.options.fPIC | |||
|
|||
if self.settings.os == "Windows" and self.settings.compiler in ["Visual Studio", "clang"] and "MT" in self.settings.compiler.runtime: | |||
if self.settings.compiler.get_safe("runtime") in ["MT", "MTd"]: |
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.
Not sure if we already care about this, but this seems not to be future proof with the settings for the new MSVC compiler
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.
Not a regression in any case, as the old code did not support the MSVC compiler settings either
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 can add "static"
. A new method in tools would be welcome to avoid fragile verifications (something like tools.is_static_runtime(self.settings.compiler)
. There are many recipes like that.
Some configurations of 'protobuf/3.9.1' failed in build 2 (
|
Some configurations of 'protobuf/3.9.1' failed in build 3 (
|
Why? I can't reproduce this error on my Mac. |
Some configurations of 'protobuf/3.9.1' failed in build 4 (
|
Some configurations of 'protobuf/3.9.1' failed in build 5 (
|
94b8820
to
d879ac2
Compare
@avantgardnerio does it break the way you use protoc (#4012)? I guess it was a workaround for cross compilation? If it breaks, I can add |
All green in build 24 (
|
# Find the protobuf compiler within the paths added by Conan, for use below. | ||
find_program(PROTOC_PROGRAM protoc PATHS ENV PATH NO_DEFAULT_PATH) | ||
if(NOT PROTOC_PROGRAM) | ||
set(PROTOC_PROGRAM "protoc") |
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.
It's not clear to me how this is not conflicting with:
369121c#diff-5dfb647bf8fb82769ddb94143d6b47475c5d4d2db2346115ee6b1991a6db6445R112
What will the merge look like?
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.
It's removed. This PR adds protobuf::protoc
imported target and Protobuf_PROTOC_EXECUTABLE
CACHE variable, but it's not very clear for me if it works with two profiles. I would like some feedbacks.
I should turn this PR to draft for the moment, since it could break cross compilation.
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 have pasted some commands below with one way you can test it.
The dual profile build doesn't seem to work.
CMake will find the wrong protoc
(the one from host context instead of the one from build context).
Command:
conan create . 3.15.5@foo/testing \
--profile:host android-ndk-r22-api-21-arm64-v8a-clang-c++_static.txt \
--profile:build default \
--build=protobuf
android-ndk-r22-api-21-arm64-v8a-clang-c++_static.txt:
[settings]
arch=armv8
build_type=Release
compiler=clang
compiler.libcxx=c++_static
compiler.version=11
os=Android
os.api_level=21
[build_requires]
android-ndk/r22
[options]
[env]
Result:
protobuf/3.15.5@foo/testing (test package): Calling build()
...
[1/4] Running cpp protocol buffer compiler on addressbook.proto
FAILED: addressbook.pb.h addressbook.pb.cc
...
/bin/sh: 1: x/.conan/data/protobuf/3.15.5/dirac/testing/package/475278d37419b9e365c649e594b7c9a419a343bb/bin/protoc: Exec format error
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.
Ok so maybe we could keep this find_program
, and use this path for protobuf::protoc
properties.
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.
Yeah, could work. I'm happy to try if you push something later :)
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.
Could you test with a0f4c76 please?
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.
Seems to work just fine. I'm happy with it :)
Failure in build 25 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
All green in build 26 (
|
All green in build 27 (
|
I have some troubles when cross-compiling
build depends protobuf and zib success, build test package, A major error occurred:
|
Thanks for that interesting links ! That was all new to me. I will read and understand it carefully. |
Specify library name and version: lib/1.0
conan-center hook activated.
Modifications in
protobuf-config.cmake.in
were too fragile. They break ifwith_zlib=True
due to expansion of CMake variable with space separators.This PR also provides a real
protobuf::protoc
imported target.