protobuf: add v4.25.3, v5.26.0

[valgur]

Continues directly from

• protobuf: add version 4.23.1 #16098

[github-actions]

🤖 Beep Boop! This pull request is making changes to 'recipes/protobuf//'.

👋 @Hopobcn you might be interested. 😉

[ghost]

I detected other pull requests that are modifying protobuf/all recipe:

• protobuf new versions #24154
• protobuf : add version 4.25.0 #23095
• Fix protobuf compilation under MSYS2 #22766
• Add protobuf 3.15.8 #22527
• Adding protobuf/3.5.1 #20627
• Fixed issue #18927 #18929

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

[uilianries]

What's the proposal of this change? I see the same protobuf-generate.cmake being installed with 4.25.3:

-- Up-to-date: /Users/uilian/.conan/data/protobuf/4.25.3/_/_/package/799e25793bb4a51e590a0065a670b4676d489bda/lib/cmake/protobuf
-- Installing: /Users/uilian/.conan/data/protobuf/4.25.3/_/_/package/799e25793bb4a51e590a0065a670b4676d489bda/lib/cmake/protobuf/protobuf-module.cmake
-- Installing: /Users/uilian/.conan/data/protobuf/4.25.3/_/_/package/799e25793bb4a51e590a0065a670b4676d489bda/lib/cmake/protobuf/protobuf-generate.cmake
-- Installing: /Users/uilian/.conan/data/protobuf/4.25.3/_/_/package/799e25793bb4a51e590a0065a670b4676d489bda/lib/cmake/protobuf/protobuf-options.cmake
-- Installing: /Users/uilian/.conan/data/protobuf/4.25.3/_/_/package/799e25793bb4a51e590a0065a670b4676d489bda/lib/cmake/protobuf/protobuf-config.cmake
-- Installing: /Users/uilian/.conan/data/protobuf/4.25.3/_/_/package/799e25793bb4a51e590a0065a670b4676d489bda/lib/cmake/protobuf/protobuf-config-version.cmake

[valgur]

I don't know why @toge saw a need for the if/else based on the version, but packaging protobuf-config.cmake is prohibited by the linter.

[uilianries]

but packaging protobuf-config.cmake is prohibited by the linter.

We can skip it from the linter or hooks. Please, revert this change.

[NoneOfUrBusiness12]

Is there any movement on this issue? Would be really nice to get this through to allow #21593 to move forward :)

[uilianries]

Is there any movement on this issue? Would be really nice to get this through to allow #21593 to move forward :)

@NoneOfUrBusiness12 Yes, it's under review. Please, take a look past conversation in this PR (few days/hours). Plus, you can customize your Github to notify about any change in this pull request.

[valgur]

It's a valid error from the linter, though. We should not be including a `protobuf-config.cmake` file in the package that could get picked up by `find_package(protobuf CONFIG)`.

[uilianries]

It's a valid error from the linter, though. We should not be including a protobuf-config.cmake file in the package that could get picked up by find_package(protobuf CONFIG).

@valgur My question is why changing from protobuf-generate to protobuf-protoc now.

[conan-center-bot]

Conan v1 pipeline ✔️

All green in build 24 (6da91e5ebd05ee88cbc164c3cfe0d329ea12b05f):

• protobuf/5.26.1:
  All packages built successfully! (All logs)

• protobuf/4.25.3:
  All packages built successfully! (All logs)

• protobuf/3.20.3:
  All packages built successfully! (All logs)

• protobuf/3.21.12:
  All packages built successfully! (All logs)

• protobuf/3.21.9:
  All packages built successfully! (All logs)

---

Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 24 (6da91e5ebd05ee88cbc164c3cfe0d329ea12b05f):

• protobuf/5.26.1:
  All packages built successfully! (All logs)

• protobuf/4.25.3:
  All packages built successfully! (All logs)

• protobuf/3.21.12:
  All packages built successfully! (All logs)

• protobuf/3.21.9:
  All packages built successfully! (All logs)

• protobuf/3.20.3:
  All packages built successfully! (All logs)

[uilianries]

@valgur Please, revert the test package and keep simplified as before. We applied a specific PR just to simplify that part of this recipe.

[valgur]

See the discussion at that simplifying PR for details. It fixed the CI limitation for build context requirements, everything else was mostly a consequence of that. There was no binary blob to remove from the test, that might be a security concern. Conan basically breaks all of the exported CMake functionality of the project by default, so it is the responsibility of Conan to also ensure that it is still working as intended. It’s not a functional test for the project itself but more of a ”linkage” test for CMake functions. The hacky workaround for macOS is also highly relevant because it will crop up in all Conan v2 recipes using protobuf. It’s better to find a suitable fix here than in the downstream recipes. I have already been bitten by this on CCI.

…

On Mon, May 20, 2024, 09:54 Martin Valgur ***@***.***> wrote: No On Mon, May 20, 2024, 09:15 Uilian Ries ***@***.***> wrote: > @valgur <https://github.com/valgur> Please, revert the test package and > keep simplified as before. We applied a specific PR just to simplify that > part of this recipe. > > — > Reply to this email directly, view it on GitHub > <#21622 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AANSD4CMAH6HBE4JA46PSMLZDGIHBAVCNFSM6AAAAABAIARYPOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJZG42DCOJZGU> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

[saschasc]

@valgur @RubenRBS @uilianries Any further progress in this PR?

It would be great to have progress in this issue to be able to upgrade GRPC. The latest available GRPC version currently has two high severity CVEs.

[jcar87]

Close as superseded by #24154

Conan basically breaks all of the exported CMake functionality of the
project by default, so it is the responsibility of Conan to also ensure
that it is still working as intended. It’s not a functional test for the
project itself but more of a ”linkage” test for CMake functions.

The upstream project does mostly 3 things in their provided CMake configuration:

1. set up the library targets
2. set up the executable target (protobuf::protoc)
3. set up the cmake macro protobuf_generate and the legacy protobuf_generate_cpp for compatibility

(1) is taken care of CMakeDeps, and (3) is expressed in the package info - the logic to generate those functions is bundled inside the package completely unchanged by the recipe. The trickier bit is (2) as CMakeDeps does not currently support defining imported executable targets.

I can see why the logic in the recipe was confusing - for earlier versions of protobuf, the macro logic was in protobuf-config.cmake, so the recipe was doing 3 things: prevent that file from attempting to load the -targets.cmake and related files, retain the macro/function logic, and inject (2) for the executable targets. This certainly did not stand the test of time. I have refactored this to split (2) to a separate file that is included as a cmake build module instead. Newer versions of protobuf are easier, because protobuf-generate.cmake is a separate standalone file - this is still packaged unchanged.

In order to test the post-conditions of (2) and (3), we've added additional logic in the test https://github.com/conan-io/conan-center-index/blob/master/recipes/protobuf/all/test_package/CMakeLists.txt#L24-L30
So we test:

• the library targets
• the executable target is well-defined
• the function is defined
• we invoke the executable (with self.run())

From these, we are satisfied that we are protected against regressions - especially considering that we do not alter the protobuf_generate logic in any way, shape or form. We are unlikely to consider reverting to what was before. If time proves us wrong, we will be the first to correct course and fix it.

The hacky workaround for macOS is also highly relevant because it will crop
up in all Conan v2 recipes using protobuf. It’s better to find a suitable
fix here than in the downstream recipes. I have already been bitten by this
on CCI.

I'm not sure this is actually happening.
When other recipes use protobuf, this is what happens on CI:

• for Conan 1.x, all dependencies are static, even when a consumer is shared so DYLD_LIBRARY_PATH is irrelevant. (there may be few exceptions, if a recipe forces the dependency to be shared and we are operating with one profile).
• for Conan 2, we pass -o "*:shared=True" when building a shared library - but this applies to the host profile, as per the conan documentation. The build profile, which applies when solving tool_requires (for consumers that need protoc), will remain with the default which is shared=False - so again, DYLD_LIBRARY_PATH is unlikely to apply and cause any issues.

Of course, if one forces a cmake project in the test_package to execute an executable from the host context in the test_package, this would be an issue - don't think it's worth it if the executable is already run via self.run().

So given this, I'm not sure whether this is a widespread issue. We are more concerned about the users and whether they experience this, rather than CI. For CI we know how to address this transparently without altering the recipes. If this becomes a widespread issue for users, we can provide advise or prioritise a fix in the Client.

I would advise that if a PR claims to add new versions, that it adds new versions rather than try to add changes that were already discarded in earlier discussions. By all means, ensuring the test package is robust is a good thing, which can be handled separately to the changes in the recipe.

[jcar87]

@saschasc - new versions of protobuf are already available. We will consider PRs to add new versions of gRPC, but bear in mind that we will try to coordinate also updating consumers of gRPC (like google-cloud-cpp), to avoid version conflicts. I have a feeling finding a version of abseil that works with all will be tricky - but I could be wrong.