Add the path to the packages version of protoc

[dheater]

Amend patches to add the path to the packages version of protoc to the add_custom_command so it can be found and used

Fixes #1956

Specify library name and version: protobuf/3.11.4

• I've read the guidelines for contributing.
• I've followed the PEP8 style guides for Python code in the recipes.
• I've used the latest Conan client version.
• I've tried at least one configuration locally with the
  conan-center hook activated.

[conan-center-bot]

Some configurations of 'protobuf/3.9.1' failed in build 1 (39c536e6cdcb625dc92da6ac5df8658ce5621c98):

• Linux x86_64, Release, gcc 5, libstdc++ . Options: protobuf:shared-True
  • Hooks errors detected:
    • [HOOK - conan-center.py] pre_export(): ERROR: [CMAKE VERSION REQUIRED (KB-H048)] The test_packages/CMakeLists.txt requires CMake 3.1 at least. Update to 'cmake_minimum_required(VERSION 3.1)'. (https://github.com/conan-io/conan-center-index/wiki/Error-Knowledge-Base#KB-H048)
  • Log download
  • Log error Download
  • Profile
• Access to all the logs

[conan-center-bot]

All green in build 2 (07e4b149ea76196af576a6db3ba4aac6b375e65b)! 😊

• protobuf/3.9.1: Generated 130 packages. All logs here
• protobuf/3.11.4: Generated 130 packages. All logs here

[jgsogo]

I think we should NOT merge this PR.

Only in a native build, we would want to execute the protoc from the ${Protobuf_LIB_DIRS}/../bin directory, so this change will break the recipe for any cross-building scenario as it is overriding the PATH.

Yes, we are overriding the DYLD_LIBRARY_PATH, but due to Macos SIP protection, this is the only way to propagate those variables and yes, it works only in a native build 😖 I'm investigating some other alternatives to bypass this limitation (conan-io/conan#7324) that will work when protoc is linked dynamically with protobuf.dylib in a cross-building scenario.

Nevertheless, I understand the underlying pain it is trying to fix: unless you activate the environment protoc is not found. This is something we are trying to figure out how to tackle, different approaches:

• conan create or recipes consuming this package work because they are using the environment_append or self.run(...., run_environment=True).
• Local build:
  1. (now) use a virtualrunenv generator and activate it, it will add the PATH to the protoc you are consuming
  2. (in the future) Via the new toolchain paradigm it should be possible to add this environment to the CMake toolchain. It will supersede the approach in (1), but we need more feedback from users to evolve the toolchain feature.

[conan-center-bot]

All green in build 3 (2682d327b1b79255fba037c425ca38a7c196fda5)! 😊

• protobuf/3.9.1: Generated 130 packages. All logs here
• protobuf/3.11.4: Generated 130 packages. All logs here

[danimtb]

can't this be something like Protobuf_BIN_DIRS or there is no such variable?

[dheater]

The FindProtobuf documentation suggest that there is a Protobuf_PROTOC_EXECUTABLE but it doesn't seem to be set.
https://cmake.org/cmake/help/latest/module/FindProtobuf.html

But I did find a CONAN_BIN_DIRS_PROTOBUF that looks like a cleaner option.

[Croydon]

This is at least partly due to fact, that we can't mimic having Protobuf_ variables prefixes, we currently have protobuf_. See also #2037

[jgsogo]

The problem is not the variable itself of casing issues. The problem is the value of the variable. Here we want to execute the protoc that belongs to the build context, while this is a CMake file executed for a package in the host context. There is only a find_package(protobuf) that provides everything from the host context, there is no find_package(protobuf THE_ONE_FROM_BUILD_CONTEXT) so none of the variables that are available contain the PATH to the protoc from the build context.

Information for protoc is only known by Conan (it knows the full graph) and Conan need to pass that information to CMake, unless we use some conan-convention (which is not fair), we can only populate the environment (PATH and DYLIB) and use just protoc inside CMake... but:

• it requires to populate the environment: also for local build
• we need to workaround SIP protection in Mac to use shared protoc.

If Protobuf_PROTOC_EXECUTABLE is a standard variable, we can populate it from Conan with the path to the proper protoc (still will fail for Macos because of SIP), but local-builds won't work unless the user adds this variable to the command-line command or the toolchain populates it.

[SSE4]

@jgsogo what about (former) idea to generate wrapper executable, such as (pseudo-code):

#!/usr/bin/env bash

# protoc

source ./conanvirtuanenv_activate.sh

if [[ "${CONAN_CONTEXT}" == "${CONAN_BUILD_CONTEXT}"]] then
  CONAN_PROTOC_EXECUTABLE = ${CONAN_PROTOC_EXECUTABLE_BUILD}
elif [[ "${CONAN_CONTEXT}" == "${CONAN_TARGET_CONTEXT}"]] then
  CONAN_PROTOC_EXECUTABLE = ${CONAN_PROTOC_EXECUTABLE_TARGET}
elif [[ "${CONAN_CONTEXT}" == "${CONAN_HOST_CONTEXT}"]] then
  CONAN_PROTOC_EXECUTABLE = ${CONAN_PROTOC_EXECUTABLE_HOST}
endif

DYLD_LIBRARY_PATH=${DYLD_LIBRARY_PATH}:${CONAN_DYLD_LIBRARY_PATH} ${CONAN_PROTOC_EXECUTABLE} $@

source ./conanvirtualenv_deactivate.sh

from my point of view:

1. it can select the protoc from the right conan context (build/host/target)
2. it can set DYLD_LIBRARY_PATH accordingly
3. the path to the generated protoc wrapper could be populated into the Protobuf_PROTOC_EXECUTABLE

[jgsogo]

Yes, IMO the wrapper approach is the only one that would work for all scenarios, but it requires a feature in Conan (conan-io/conan#7324 and conan-io/conan#7240), with them Conan can generate the wrapper for all the executables in a package and populate PATH environment variable with the path to them (install folder, local working directory,...). I think this is not specific to protoc but to every executable.

For the local development flow, still the toolchain approach need to populate the environment, it isn't right now.

[SSE4]

@jgsogo is it really needed to wait for a feature in conan? e.g. in android_ndk_installer package, we provide such a wrapper for the CMake executable. I guess nothing prevents us from generating wrapper in the recipe right away.

[jgsogo]

I know that wrapper, I've been using it and it is amazing. But that one is a wrapper over CMake and it uses the CONAN_CMAKE_PROGRAM environment variable that Conan reads to run cmake.

Here we can write a protoc wrapper, and we can add an environment variable with it, then our protobuf-config.cmake can read that environment variable and use it to run protoc... alternative would be to wrap cmake and pass Protobuf_PROTOC_EXECUTABLE via the command line.

Both approaches could work (I¡d prefer the first one), but they won't fix the local-build problem which is the issue that originated this PR, it would only workaround the SIP-Macos issue.

BUT, in order to generate the wrapper with the right values, you need the paths to the shared libraries you are using in the current graph, so it has to take the values in runtime, it cannot be generated together with the package. You cannot trust the virtualenv generators (they might not be generated), you need to intercept the environment variables and hardcode them in the wrapper. How can you do it? Who is responsible of writing the wrapper?

[danimtb]

Having a look at the comments I see the issue is far beyond the naming of the bar, so I'd unblock my review

[dheater]

@jgsogo

The virtualrunenv works for my example case. I just need to figure out how to make that work in CLion as part of a whole project build 😏

I tried cross-compiling this recipe as is for an RPi and it fails when trying to run the test_package. I get the same with or without these changes. protoc is built for the target architecture when we need it for the host.

Maybe libprotobuf and protoc should be in separate recipes so you can apply different profiles? Looks like something like that was done before under bintray though, so I assume that changed for good reason.

[conan-center-bot]

All green in build 4 (ee51a926a24ce1f65591c9bc3e956313c8966262)! 😊

• protobuf/3.9.1: Generated 130 packages. All logs here
• protobuf/3.11.4: Generated 130 packages. All logs here

[dheater]

@jgsogo
What do you think of spitting the protoc and protobuf recipes so we can build a cross-compiling version of protoc with a target version of libprotobuf?

[jgsogo]

Splitting won't help with the SIP issue. If you build protoc shared, then you still need to find shared library protobuf.dylib (even if it is in the same protoc package).

[9chu]

Hello guys,
I'm using the cmake-conan wrapper to build my project with the latest version of protobuf as the dependency.
However the PROTOBUF_GENERATE_CPP command will throw the No such file or directory error under such a configuration, seems the PATH environment variable is not set properly.
Hoping this PR will be merged to solve this issue.

[jgsogo]

There is a PR to Conan proposing wrappers around executables, IMHO, that would be the way to go if we don't find any blocker.

[jpilet]

I came up with a better solution I think. The trick is to add a find_program() call in protobuf-config.cmake, which is a file installed by the package. We can then use CONAN_BIN_DIRS_PROTOBUF variable created by conan.

See
master...jpilet:v3.13.0#diff-1d2d68631fd8ae8643ca899c197696909e08e36ad587a9485213dc528f9c534fR174

I know that my changes mix upgrade to version 3.13.0 AND adding this path... if you think my solution is worth merging, I could work on it.

This change significantly reduces the complexity of writing a gRPC package that is easy to consume. It removes the need for findPackage().

[jgsogo]

The problem is not about locating protoc, protoc is already in the PATH (if protobuf package is added as a build-require to the consumer recipe, Conan will add the path to the bin directory of the build-requires automatically) and it works out of the box (cross-compiling scenario works using two profiles).

The problem is about finding the dynamic libraries that protoc might depend on. I would expect find_program to locate protoc application, but does it return the path to all the dynamic libraries needed to run the executable? Because we need to append those paths to the environment in the add_custom_command, otherwise MacOS is going to remove the information about DYLD_LIBRARY_PATHS and it will fail.

[jpilet]

I see. There's a catch with using PATH: if the user has another version of protoc installed in /usr/bin for example, simply calling "protoc" might call the wrong executable. So it is better to force using the right path.

About finding the dynamic libraries: what about using install_name_tool on macOS to set the path to .dylibs relative to the protoc executable?

[jgsogo]

Wow! Didn't know about the install_name_tool Thanks!... this is something to consider if the wrappers/shims approach doesn't work. It will require Conan to copy the executable to some local folder, and apply install_name_tool to use the dynamic libraries we want (probably the same workflow for other OS).

Note.- We cannot modify the executable in the Conan cache for two reasons:

• it will modify the package revision, and
• different graphs might use the executable from the same folder (same package-ID), but due to overrides they can be using different versions for shared libraries upstream.

[prince-chrismc]

Would it be possible to mark this PR as 'blocked' until there is a clear solution? Unless that will distract from the amazing work you guys are doing, in that case dont mind me!

[dheater]

This approach does not appear to be viable at this time. virtualenv works and seems to be a sufficient near-term solution.