protobuf: simplify test package

[jcar87]

Simplify test_package for protobuf such that the only tested reference is the one that was just built. conan graph build-rder does not take into consideration the build order from the test_package, and forcing a build order in our CI pipeline is very suboptimal (we want to build packages in parallel as much as possible, if resources allow).

The logic is the one that we should follow in all test_packages: the minimal test such that we ensure what we just built works - it was never meant to be comprehensive nor complex.

Also remove test_v1_package, as there is no point in backporting the changes to it.

[ghost]

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

• protobuf : add version 4.25.0 #23095
• Fix protobuf compilation under MSYS2 #22766
• Add protobuf 3.15.8 #22527
• protobuf: add v4.25.3, v5.26.0 #21622
• Adding protobuf/3.5.1 #20627
• Fixed issue #18927 #18929
• protobuf: add version 4.23.1 #16098

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

[Ahajha]

@sophieeihpos @valgur This should help with missing binaries problems we've seen in other PRs

[Croydon]

😥

[valgur]

I'm not a fan of the removing the protobuf-specific function calls in CMake. Their correct functioning is not a given for Conan, where the corresponding CMake modules must be explicitly handled in package() and package_info(). I would recommend adding a VirtualRunEnv(self).generate(scope="build") instead to make protoc available for CMake, which I assume is the reason for the switch to self.run().

I updated my PR with the above approach: 02fbd30

conan graph build-order does not take into consideration the build order from the test_package, and forcing a build order in our CI pipeline is very suboptimal (we want to build packages in parallel as much as possible, if resources allow).

Huh. I really appreciate the background info on the build-context requirements failures. Thanks!
It does not sound like it should be a fundamental limitation for Conan and/or C3I, though? Could this be fixed in the future?
If not, we should document this in the CCI docs and package_templates. That would be a somewhat disappointing outcome, though, since test_package also tends to double as a usage example for the packages and would force us to include an anti-pattern instead of demonstrating best practices for Conan recipes. The effect of self.requires(run=True) on the dependency graph is not super obvious for users less familiar with Conan.

[jcar87]

It does not sound like it should be a fundamental limitation for Conan and/or C3I, though? Could this be fixed in the future?

We are currently working on a redesign of the CI pipeline that will take this into account - especially for any package that has a tool_requires against the tested reference. That said, we will reserve it for cases that are very justified, we still want to simplify the test_packages - the approach we're likely to go with is that recipes that have a use for a more complex (less superficial) test package, will need to have a separate one.

Since test_package also tends to double as a usage example for the packages and would force us to include an anti-pattern

I'm not sure where the mindset comes from, since that was never the intended purpose of test_packages in Conan.
From the very inception of Conan, the test_package is not meant to be a usage example - but a minimal test that a package can be consumed (@memsharded may be able to provide more historical context here). This translates to the following points:

1. can the consumer build system (e.g. CMake) issue a find_package and find it from Conan?
2. are the target names referenced in the build system satisfied by Conan? (e.g. ZLIB::ZLIB)
3. can the compiler resolve an #include <> for a file inside that should be located inside the package folder?
4. can the linker satisfy symbols from a library (.a, .so, .dylib, .lib) that lives inside the package folder?
5. and optionally, if the package contains executables, can the consumer run an executable and will it be found inside the package?

Anything beyond this is tends to be unnecessary, and is well beyond the scope of what we do in Conan Center: we very explicitly do not want to test the functionality - we want to ensure it builds correctly, and that it can be consumed.
That is, the zlib test package does not need to test whether compressing works, the libpng test package does not need to test whether an image file can be opened - so long as we can call a function in the test package source file that satisfies the above points. Some of these are "harmless" because they are run very quickly, but otherwise they are a complete waste of CI resources Now, I can understand sometimes that as a maintainer it can be easier to base the test_package on the most minimal example provided by the upstream library (e.g. in their readme or documentation) - but this sometimes leads to silly outcomes like having a test_package attempt to play a sound through the sound card...

As for this PR, I'm confident that it satisfies the 5 points I mention above. And I'm happy to talk to the team and amend documentation if this needs to be clarified. The only one that would be in question is (1) - but bear in mind that the logic to locate protoc in CMake is still being executed just fine, we're just not letting cmake invoke protoc at build time. It's a small risk and will consider a different approach once we are running our redesign pipeline.

I updated my PR with the above approach: 02fbd30

your proposed approach is likely to fail when you are cross-building protobuf: you are forcing CMake to execute, at build time, the protoc in the host package, even in cases where it will not run at all, like a different architecture. I would discourage this, as we need to bear in mind that there are users that build their own binaries, supporting more platforms than we do.

[jcar87]

A couple of added points:

since test_package also tends to double as a usage example for the packages and would force us to include an anti-pattern instead of demonstrating best practices for Conan recipes.

It is the longstading opinion of the team that test packages like this were already an anti-pattern and against our originally intended best practice.

The effect of self.requires(run=True) on the dependency graph is not super obvious for users less familiar with Conan.

I've added a comment in the line explaining why run=True is needed - I could probably clarify more - the reason is that the package_type is library, so a requires only takes libraries into account, not the executable runtime.

If we are concerned that the logic inside protobuf-generate.cmake is not being tested, we can leave the logic to invoke protobuf_generate - perhaps even double check what the resulting command looks like, short of letting cmake invoke protoc during the build. The combination of self.run() and that check would mean that the usage is still covered, and the package is still correct.

[valgur]

I'm not sure where the mindset comes from, since that was never the intended purpose of test_packages in Conan.

While, yes, the small test snippets act as a light usage example for the packaged library, that's not at all what I meant. That part is abundantly clear to me by now. It was about the test_package as a usage example for a consuming recipe. The case of having to use a package as both a requires() and a tool_requires() together with respective VirtualEnvs is something that takes some getting accustomed to as a new user of Conan and the conanfile.py in test_package acts as a good place to demonstrate the correct usage, if possible. The same goes for the CMakeLists.txt, to a lesser degree.

test packages like this were already an anti-pattern

I assume you mean the source code in this test_package? Or something else? I don't honestly see what's overly or needlessly complicated in this test_package, tbh? Dropping a couple of lines from the .cpp file is pretty meaningless for a library that takes more than ten minutes to build.

your proposed approach is likely to fail when you are cross-building protobuf

Oh. Good point. Thanks! Might be able to check CMAKE_CROSSCOMPILING in the CMakeLists.txt as well, maybe.

[jcar87]

I don't honestly see what's overly or needlessly complicated in this test_package, tbh?

I meant to say, the test package is meant to test the package reference that was just built (during conan create), and not two different package IDs for the same recipe. Either one (as a requires) or the other (as a tool_requires) but not both. If you test different ones, you need to be "externally" aware of the build order implications - thus creating the problem we are experiencing.

i'm confident what Im proposing in this PR tests both - if you test the executable of a normal requires, it also acts a proxy for testing it when it is consumed as a tool_requires.

[jcar87]

I'm not sure where the mindset comes from, since that was never the intended purpose of test_packages in Conan.

While, yes, the small test snippets act as a light usage example for the packaged library, that's not at all what I meant. That part is abundantly clear to me by now. It was about the test_package as a usage example for a consuming recipe. The case of having to use a package as both a requires() and a tool_requires() together with respective VirtualEnvs is something that takes some getting accustomed to as a new user of Conan and the conanfile.py in test_package acts as a good place to demonstrate the correct usage, if possible. The same goes for the CMakeLists.txt, to a lesser degree.

test packages like this were already an anti-pattern

I assume you mean the source code in this test_package? Or something else? I don't honestly see what's overly or needlessly complicated in this test_package, tbh? Dropping a couple of lines from the .cpp file is pretty meaningless for a library that takes more than ten minutes to build.

your proposed approach is likely to fail when you are cross-building protobuf

Oh. Good point. Thanks! Might be able to check CMAKE_CROSSCOMPILING in the CMakeLists.txt as well, maybe.

One of the things we are trying to do is centralise the aspect of "show how it is consumed" in the Conan Center website: e.g. https://conan.io/center/recipes/zlib. At the moment we try to show a best effort in terms of consuming recipes - but we want to add the ability to customise what is visible for each recipe - protobuf is a good example where it would be useful to show how to use the dual requirements.

[conan-center-bot]

Conan v1 pipeline ✔️

All green in build 4 (ec8725910d0ed71c9d300e4fbc4ffb1135a0f19e):

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

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

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

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

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

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

• protobuf/3.19.6:
  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 3 (ec8725910d0ed71c9d300e4fbc4ffb1135a0f19e):

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

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

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

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

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

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

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

[SpaceIm]

Well, this PR exposes this recipe to regressions. protobuf provides custom cmake macros, which have to be specially handled in this recipe to work properly for consumers, and it's not tested anymore.