Add protobuf 3.12.3 #2037
Add protobuf 3.12.3 #2037
Conversation
|
The problem with enabling protobuflite is that protoc is not built.
|
|
Some configurations of 'protobuf/3.9.1' failed in build 1 (
|
Why? if options.lite is enable, it means the user only wants it.
Do you mean changing only cmake/install.cmake ? Why?
I didn't get, lite generates a different binary package. |
Seems like we need to stop patching and finding a definitive alternative. |
Because
I meant we won't have to patch the files to disable building protobuf/protoc when protobuflite is enabled.
The built package will contain both libprobotuf.so and libprotobuflite.so, but the cpp_info.libs will only contain one (or both) of them. This is just a proposal. |
By applying my proposal, then protoc will be available always. |
|
@madebr thanks for the details. Indeed your idea sounds correct, as we are using all together and build context can help for other cases, the patches are no longer required. I'll update following your idea, thanks again |
|
Some configurations of 'protobuf/3.9.1' failed in build 2 (
|
|
@uilianries please fix conflicts, fix the build, and then re-request the review(s) |
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
|
Do we agree that it is completely acceptable to have libprotobuf, libprotobuf-lite and protoc all in one package? I don't see why not as long as you don't link against the entire package (which is always bad I guess) there should be no danger of linking against libprotobuf and libprotobuf-lite at the same time We could also change this behaviour only for protobuf > 3.12 to not break anyone |
|
I think I got pretty far, but run into protocolbuffers/protobuf#7759 |
|
@Croydon Makes sense to me. |
@madebr it requires the new components feature first, custom cmake filename. |
|
@sourcedelica I noticed this too. We currently explicitly patching this, as we did not had the protobuf::protoc target before. I'm seeing what I can do |
|
which each time causes an error like I'm not sure if we can provide these properties somehow or If I need to patch this file heavily to get the values otherwise? |
Without patching it seems to build everything by default in one go. My current idea is to provide a |
|
I spend quite some time with protobuf/protoc yesterday and basically went through hell and back. I still encounter some problems that CMake can't find the targets despite Conan generating exactly those targets... |
|
I think we should split this. Add the new version in a new PR and leave adding components and refactoring here |
|
@Croydon |
|
I agree with @Croydon. The case of this library is quite singular and getting components right might take some more tries. I'd suggest adding the new version with the minimal require changes to the recipe and patches and then resume the work here with components and cross-building |
|
Git error in build 3: |
* fix CMake file names to be lowercase * replace huge version dependent patches which a few replace_in_file which are much easier to maintain * protoc is now always available; removing all non-protoc ways in the test_package
|
Recipe syntax error in build 39: |
|
All green in build 40 (
|
|
@madebr @prince-chrismc @SpaceIm @SSE4 (& co): Please review (again)😄 |
|
No more changes, let's merge and save them for the next PR 🚀 I know a lot of us are dying to get this in CCI |
|
Critical things should always be fixed, but I agree, no more nit-picking in this one please. I will also very soon open a PR for version 3.13.0 anyway (if not someone else will be faster) 😄 |
| tools.replace_in_file( | ||
| os.path.join(self._source_subfolder, "cmake", "protobuf-config.cmake.in"), | ||
| """COMMAND protobuf::protoc | ||
| ARGS --${protobuf_generate_LANGUAGE}_out ${_dll_export_decl}${protobuf_generate_PROTOC_OUT_DIR} ${_protobuf_include_path} ${_abs_file} |
There was a problem hiding this comment.
@Croydon
Has this problem been resolved?
| os.path.join(self._source_subfolder, "cmake", "protobuf-module.cmake.in"), | ||
| '# Version info variable', | ||
| 'endif()', | ||
| ) |
There was a problem hiding this comment.
@Croydon
Has this problem been resolved?
| self.cpp_info.components["libprotobuf-lite"].builddirs = [self._cmake_install_base_path] | ||
| self.cpp_info.components["libprotobuf-lite"].build_modules.extend([ | ||
| os.path.join(self._cmake_install_base_path, "protobuf-generate.cmake"), | ||
| os.path.join(self._cmake_install_base_path, "protobuf-module.cmake"), | ||
| os.path.join(self._cmake_install_base_path, "protobuf-options.cmake"), | ||
| ]) |
There was a problem hiding this comment.
Is it correct to only include these files when lite=True?
Doesn't the hooks complain when lite=False, because it will find .cmake files, not added as a build module?
There was a problem hiding this comment.
It is also included for the libprotobuf component
There was a problem hiding this comment.
Ah thanks, I see.
Now I'm curious whether these modulse will be included once or twice.
|
GitHub behaves sometimes strange for me in the last few days. @madebr I can't directly response to your two In some cases the location of Protoc might still fail, but there is another PR open for it, we shouldn't block this one further since it works in many, maybe most, cases The other line replacement is just to effectively disable a larger part of the CMake file without having version specific patches which are much harder to maintain |
They are responses on remarks by another user. |
I believe the issues are hidden by then fold mechanics, if you use the "files changes" tab they still appear (and you can reply) I am pretty sure they are.. but the diff a few lines down did not mark them as outdated |
| "# CONAN PATCH include(\"${CMAKE_CURRENT_LIST_DIR}/protobuf-targets.cmake\")" | ||
| ) | ||
| if tools.Version(self.version) < "3.12.0": | ||
| tools.replace_in_file( |
There was a problem hiding this comment.
@Croydon
I won't object here, but why did you use tools.replace_in_file here, instead of a patch?
There was a problem hiding this comment.
They would be more version specific and are much harder to maintain then those few replacements
|
I'm testing this in my project by following This is on Ubuntu 20.04 with |
|
@dheater We need much more information to understand what you are doing. Conan version, what Conan generators you used, the CMake code... |
Conan version is 1.31.2. |
|
If you are just installing the protobuf recipe and then just execute a cmake build with the |
Ah. I see. The Thanks! |
|
@dheater |
| del self.options.fPIC | ||
|
|
||
| compiler_version = Version(self.settings.compiler.version.value) | ||
| if compiler_version < "14": | ||
| raise ConanInvalidConfiguration("On Windows Protobuf can only be built with " |
There was a problem hiding this comment.
Should this piece of code be executed on Linux?
Because I saw this on Linux right now :-(
$ conan create . protobuf/3.12.4@ -o protobuf:shared=True
Exporting package recipe
protobuf/3.12.4 exports: File 'conandata.yml' found. Exporting it...
protobuf/3.12.4 exports: Copied 1 '.yml' file: conandata.yml
protobuf/3.12.4 exports_sources: Copied 1 '.txt' file: CMakeLists.txt
protobuf/3.12.4 exports_sources: Copied 2 '.patch' files: upstream-issue-7567-no-export-template-define.patch, upstream-pr-7761-cmake-regex-fix.patch
protobuf/3.12.4: A new conanfile.py version was exported
protobuf/3.12.4: Folder: /home/pfarre/.conan/data/protobuf/3.12.4/_/_/export
protobuf/3.12.4: Exported revision: fcab376e471eb6443f1f3fd012065226
Configuration:
[settings]
arch=x86_64
arch_build=x86_64
build_type=Release
compiler=gcc
compiler.libcxx=libstdc++
compiler.version=9
os=Linux
os_build=Linux
[options]
protobuf:shared=True
[build_requires]
[env]
ERROR: protobuf/3.12.4: Invalid configuration: On Windows Protobuf can only be built with Visual Studio 2015 or higher.
There was a problem hiding this comment.
Yup that's a bug.
You can't build a shared protobuf using a current compiler.
(or it's a feature and you'll have to wait until 2024 when gcc 14 gets released 😄 )
Specify library name and version: protobuf/3.12.3
closes #2031
Protobuf it's a special case:
All targets are lower-case:
protobuf::libprotobuf,protobuf::libprotobuf-lite,protobuf::libprotoc,protobuf::protocHowever, variables and the cmake find file are CamelCase:
FindProtobuf.cmake
Protobuf_LIBRARY
Protobuf_PROTOC_LIBRARY
...
@danimtb What's the magic for using Protobuf for the file name, but protobuf as target namespace?
/cc @madebr
conan-center hook activated.