Skip to content
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

Correctly pick up & use Protobuf_PROTOC_EXECUTABLE in cross-compilation #14576

Open
h-vetinari opened this issue Nov 1, 2023 · 19 comments
Open
Assignees
Labels

Comments

@h-vetinari
Copy link
Contributor

It seems that when using find_package(Protobuf CONFIG), any eventual setting of Protobuf_PROTOC_EXECUTABLE is ignored.

This makes it hard for us in conda-forge, because find_package(Protobuf CONFIG) is necessary to avoid picking up CMake's own implementation of finding protobuf (and prefer an up-to-date protobuf-config.cmake; which is necessary for correctly linking abseil for example).

On the other hand, we simply must cross-compile on several architectures (e.g. no osx-arm64 runners at scale), and so if Protobuf_PROTOC_EXECUTABLE gets ignored (which carefully points to the build environment that matches the arch or the agent, and not the target environment for the final binary) we run into Bad CPU type in executable when it calls the wrong protoc.

The work-around we have come up with (and keep reinventing) in several places boils down to (courtesy @traversaro):

 find_package(Protobuf CONFIG)
 if(Protobuf_FOUND)
     message(STATUS "Found protobuf via cmake config")
+    if(Protobuf_PROTOC_EXECUTABLE)
+      set_target_properties(protobuf::protoc PROPERTIES
+        IMPORTED_LOCATION_RELEASE "${Protobuf_PROTOC_EXECUTABLE}"
+      )
+    endif()
 else()
     message(WARNING "Falling back to cmake FindProtobuf as Protobuf was not found via CONFIG")
     find_package(Protobuf REQUIRED)
 endif()

However, not everyone encountering this issue might be able to investigate and come up with this solution, so we keep running into it. I don't see why this shouldn't be supported out of the box, hence this issue.

What version of protobuf and what language are you using?
Version: 4.24.3
Language: C++

What operating system (Linux, Windows, ...) and version?

linux/osx

What runtime / compiler are you using (e.g., python version or gcc version)

What did you do?
Cross-compile a package that requires protoc, pass -DProtobuf_PROTOC_EXECUTABLE=... to CMake invocation

What did you expect to see

Correct protoc (corresponding to Protobuf_PROTOC_EXECUTABLE) gets used.

What did you see instead?

Wrong protoc gets picked up.

Anything else we should know about your project / environment

Some examples from conda-forge:

@h-vetinari h-vetinari added the untriaged auto added to all issues by default when created. label Nov 1, 2023
@hlopko hlopko added the cmake label Nov 2, 2023
@hlopko hlopko removed the untriaged auto added to all issues by default when created. label Nov 2, 2023
@mkruskal-google
Copy link
Member

Thanks for reporting this! If you happen to know what needs to change to fix this I'd be happy to review a PR. I'm not very familiar with cross-compiling via cmake though, so this might take me some time to track down and fix

@h-vetinari
Copy link
Contributor Author

@traversaro, is this something you think you could help with? 🙃

@traversaro
Copy link
Contributor

traversaro commented Nov 12, 2023

@traversaro, is this something you think you could help with? 🙃

Yes, for sure if there is interest upstream I can prepare a PR. As a preliminary analysis, after thinking a bit I am afraid of the the fine details on how we can do this "overloading" of the location and the unintended side-effects, especially when using multiple config CMake generators. Probably a simpler solution is to just make sure that Protobuf_PROTOC_EXECUTABLE by default is defined as protobuf::protoc, and that ${Protobuf_PROTOC_EXECUTABLE} is used in place of protobuf::protoc in

COMMAND protobuf::protoc
. The downside of this solution is that downstream projects that use protobuf::protoc instead of the protobuf_generate CMake function will not work out of the box, but anyhow the fix for those cases would be quite straightforward (use ${Protobuf_PROTOC_EXECUTABLE} in place of protobuf::protoc). To get an idea, we can do something similar to what we did in gazebosim/gz-msgs#392 .

@ekigwana
Copy link

Setting Protobuf_PROTOC_EXECUTABLE used to work. Now it looks like the new variable is protobuf_PROTOC_EXE but as mentioned earlier it gets ignored

@ekigwana
Copy link

It seems that when using find_package(Protobuf CONFIG), any eventual setting of Protobuf_PROTOC_EXECUTABLE is ignored.

This makes it hard for us in conda-forge, because find_package(Protobuf CONFIG) is necessary to avoid picking up CMake's own implementation of finding protobuf (and prefer an up-to-date protobuf-config.cmake; which is necessary for correctly linking abseil for example).

On the other hand, we simply must cross-compile on several architectures (e.g. no osx-arm64 runners at scale), and so if Protobuf_PROTOC_EXECUTABLE gets ignored (which carefully points to the build environment that matches the arch or the agent, and not the target environment for the final binary) we run into Bad CPU type in executable when it calls the wrong protoc.

The work-around we have come up with (and keep reinventing) in several places boils down to (courtesy @traversaro):

 find_package(Protobuf CONFIG)
 if(Protobuf_FOUND)
     message(STATUS "Found protobuf via cmake config")
+    if(Protobuf_PROTOC_EXECUTABLE)
+      set_target_properties(protobuf::protoc PROPERTIES
+        IMPORTED_LOCATION_RELEASE "${Protobuf_PROTOC_EXECUTABLE}"
+      )
+    endif()
 else()
     message(WARNING "Falling back to cmake FindProtobuf as Protobuf was not found via CONFIG")
     find_package(Protobuf REQUIRED)
 endif()

However, not everyone encountering this issue might be able to investigate and come up with this solution, so we keep running into it. I don't see why this shouldn't be supported out of the box, hence this issue.

What version of protobuf and what language are you using? Version: 4.24.3 Language: C++

What operating system (Linux, Windows, ...) and version?

linux/osx

What runtime / compiler are you using (e.g., python version or gcc version)

What did you do? Cross-compile a package that requires protoc, pass -DProtobuf_PROTOC_EXECUTABLE=... to CMake invocation

What did you expect to see

Correct protoc (corresponding to Protobuf_PROTOC_EXECUTABLE) gets used.

What did you see instead?

Wrong protoc gets picked up.

Anything else we should know about your project / environment

Some examples from conda-forge:

I had to set IMPORTED_LOCATION_RELWITHDEBINFO so even that seems like fun trap that just cost me 4 hours to discover

@traversaro
Copy link
Contributor

I had to set IMPORTED_LOCATION_RELWITHDEBINFO so even that seems like fun trap that just cost me 4 hours to discover

Yes, I was referring exactly to that with "I am afraid of the the fine details on how we can do this "overloading" of the location and the unintended side-effects, especially when using multiple config CMake generators.", sorry about that. I hope to prepare a concrete proposal to address this in the next week.

Copy link

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

@github-actions github-actions bot added the inactive Denotes the issue/PR has not seen activity in the last 90 days. label Feb 28, 2024
@traversaro
Copy link
Contributor

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

Ops, sorry about that. I forgot to follow up on my original " I hope to prepare a concrete proposal to address this in the next week.".

@github-actions github-actions bot removed the inactive Denotes the issue/PR has not seen activity in the last 90 days. label Feb 29, 2024
@0xDivyanshu-new
Copy link

Any update over here? The suggested workaround doesnt work for me and it throws error saying i cant use set_target_properties on alias. I literally did the same thing as what has been told here!

@traversaro
Copy link
Contributor

Any update over here? The suggested workaround doesnt work for me and it throws error saying i cant use set_target_properties on alias. I literally did the same thing as what has been told here!

How are you including protobuf in your CMake project, via find_package(Protobuf) or add_subdirectory/FetchContent?

@0xDivyanshu-new
Copy link

0xDivyanshu-new commented Mar 18, 2024

if(PROTOBUF_COMPILE)
    include(FetchContent)
    set(FETCHCONTENT_QUIET ON)
    set(FETCHCONTENT_UPDATES_DISCONNECTED ON)
    set(BUILD_SHARED_LIBS OFF)
    set(CMAKE_POSITION_INDEPENDENT_CODE ON)
    find_package(Git REQUIRED)
    set(protobuf_BUILD_TESTS OFF CACHE BOOL "" FORCE)
    set(protobuf_BUILD_CONFORMANCE OFF CACHE BOOL "" FORCE)
    set(protobuf_BUILD_EXAMPLES OFF CACHE BOOL "" FORCE)
    set(protobuf_BUILD_EXPORT OFF CACHE BOOL "" FORCE)
    set(protobuf_WITH_ZLIB OFF CACHE BOOL "" FORCE)
    fetchcontent_declare(
        protobuf GIT_REPOSITORY "https://github.com/protocolbuffers/protobuf.git" GIT_TAG v3.15.6
        PATCH_COMMAND "" SOURCE_SUBDIR cmake
    )
    fetchcontent_makeavailable(protobuf)
    fetchcontent_getproperties(protobuf SOURCE_DIR PROTOBUF_INCLUDE_DIR)

else()
    set(PROTOBUF_INCLUDE_DIR "${CMAKE_CURRENT_LIST_DIR}/build/x64/_deps/protobuf-src")
endif(PROTOBUF_COMPILE)

set(PROTOBUF_INCLUDE_DIR "${PROTOBUF_INCLUDE_DIR}/src")
include_directories(${PROTOBUF_INCLUDE_DIR})
message("PROTOBUF_INCLUDE_DIR value : ${PROTOBUF_INCLUDE_DIR}")
set(Protobuf_INCLUDE_DIR ${PROTOBUF_INCLUDE_DIR})
set(Protobuf_LIBRARIES "${PROTOBUF_INCLUDE_DIR}/protobuf-build/libprotoc.lib")
set(Protobuf_PROTOC_EXECUTABLE "${PROTOBUF_INCLUDE_DIR}/protobuf-build/protoc.exe")

find_package(Protobuf REQUIRED)
set_target_properties(
    "protobuf::protoc" PROPERTIES IMPORTED_LOCATION_RELWITHDEBINFO ${Protobuf_LIBRARIES}
                                  IMPORTED_LOCATION_RELEASE ${Protobuf_PROTOC_EXECUTABLE}
)

I am doing fetchcontent in cmake and then using find_package

@traversaro
Copy link
Contributor

You need to use either FetchContent or use find_package, in general you can't mix the two.

@0xDivyanshu-new
Copy link

But if I am to cross compile protobuf, i would have to build it in machine where cross compilation tools are available. In such cases, if i dont use FetchContent i wont be able to build it using my setup. And if i dont use find_package i wont be able to use protobuf_generate_cpp to generate the headers.

@traversaro
Copy link
Contributor

In the kind of cross-compilation builds to which the workaround in the OP refers, protobuf is cross-compiled and installed before you configure for cross-compilation the package that depends on protobuf, and this cross-compiled protobuf is found via find_package.

@dubvulture
Copy link

Since I'm keeping an eye on this issue, I'll also leave a revised version of OP's for the internet wanderers like me.

find_package(protobuf CONFIG REQUIRED)
if(CMAKE_CROSSCOMPILING)
    find_program(_PROTOBUF_PROTOC_EXECUTABLE protoc)
    if(NOT TARGET protobuf::protoc)
        add_executable(protobuf::protoc IMPORTED)
    endif()
    set_target_properties(protobuf::protoc PROPERTIES
        IMPORTED_LOCATION_RELEASE "${_PROTOBUF_PROTOC_EXECUTABLE}"
        IMPORTED_LOCATION_DEBUG "${_PROTOBUF_PROTOC_EXECUTABLE}"
        IMPORTED_LOCATION_RELWIHDEBINFO "${_PROTOBUF_PROTOC_EXECUTABLE}"
        IMPORTED_LOCATION_NOCONFIG "${_PROTOBUF_PROTOC_EXECUTABLE}"
    )
endif()
  • I'm checking if the protobuf::protoc target exists since cross-compiling protobuf with -Dprotobuf_BUILD_PROTOC_BINARIES=OFF (like I do) does not create the target
  • Added a bunch of IMPORTED_LOCATION_<CONFIG> since IMPORTED_LOCATION gets overriden by those

@rsniezek
Copy link

Since I'm keeping an eye on this issue, I'll also leave a revised version of OP's for the internet wanderers like me.

find_package(protobuf CONFIG REQUIRED)
if(CMAKE_CROSSCOMPILING)
    find_program(_PROTOBUF_PROTOC_EXECUTABLE protoc)
    if(NOT TARGET protobuf::protoc)
        add_executable(protobuf::protoc IMPORTED)
    endif()
    set_target_properties(protobuf::protoc PROPERTIES
        IMPORTED_LOCATION_RELEASE "${_PROTOBUF_PROTOC_EXECUTABLE}"
        IMPORTED_LOCATION_DEBUG "${_PROTOBUF_PROTOC_EXECUTABLE}"
        IMPORTED_LOCATION_RELWIHDEBINFO "${_PROTOBUF_PROTOC_EXECUTABLE}"
        IMPORTED_LOCATION_NOCONFIG "${_PROTOBUF_PROTOC_EXECUTABLE}"
    )
endif()
  • I'm checking if the protobuf::protoc target exists since cross-compiling protobuf with -Dprotobuf_BUILD_PROTOC_BINARIES=OFF (like I do) does not create the target
  • Added a bunch of IMPORTED_LOCATION_<CONFIG> since IMPORTED_LOCATION gets overriden by those

Thank you, this works for me in yocto cross compilation. Any chances that this could be merged?

resiliencetheatre added a commit to resiliencetheatre/rpi4edgemapdisplay that referenced this issue Aug 17, 2024
…ore recent v27.3 protobuf

so I could get rid of this error when running latest meshtastic command:

edgemap:~# meshtastic --info
Connected to radio

Aborting due to: MessageToJson() got an unexpected keyword argument 'always_print_fields_with_no_presence'

But as you would guess, playing with evil Google is not that easy. They changed build environment to
cmake and decided to get rid of cross compilation support [1]. And giving really nice answer for
developers asking about it [2]:

“Unfortunately our cross compile setup is extremely specific to the docker images we
use and we aren't staffed to support a general cross compiling setup.”

So this happens when IqT fundend docker meets IqT funded Google. My take on this is
that resilience targeted projects should stay away from both of these names.

Resolution:

I included patch (0001-remove-always_print_fields_with_no_presence.patch) to remove
newly added option and meshtastic works now again with protobuf v21.12.

[1] protocolbuffers/protobuf#14576
[2] protocolbuffers/protobuf#17347
Copy link

github-actions bot commented Sep 8, 2024

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago. This issue will be closed and archived after 14 additional days without activity.

@github-actions github-actions bot added the inactive Denotes the issue/PR has not seen activity in the last 90 days. label Sep 8, 2024
@traversaro
Copy link
Contributor

The issue is still present.

@github-actions github-actions bot removed the inactive Denotes the issue/PR has not seen activity in the last 90 days. label Sep 9, 2024
@justusranvier
Copy link

I've combined and modified some of the workarounds posted above to optionally use protoc provided by vcpkg:

if(CMAKE_CROSSCOMPILING)
  if(NOT DEFINED ${PROJECT_NAME}_PROTOBUF_PROTOC_EXECUTABLE)
    if(DEFINED VCPKG_HOST_TRIPLET)
      set(${PROJECT_NAME}_PROTOBUF_PROTOC_EXECUTABLE
          "${VCPKG_INSTALLED_DIR}/${VCPKG_HOST_TRIPLET}/tools/protobuf/protoc"
      )
    else()
      find_program(
        ${PROJECT_NAME}_PROTOBUF_PROTOC_EXECUTABLE
        protoc
        REQUIRED
      )
    endif()
  endif()

  if(NOT TARGET protobuf::protoc)
    add_executable(protobuf::protoc IMPORTED)
  endif()

  set_target_properties(
    protobuf::protoc
    PROPERTIES
      IMPORTED_LOCATION_RELEASE "${${PROJECT_NAME}_PROTOBUF_PROTOC_EXECUTABLE}"
      IMPORTED_LOCATION_DEBUG "${${PROJECT_NAME}_PROTOBUF_PROTOC_EXECUTABLE}"
      IMPORTED_LOCATION_RELWIHDEBINFO "${${PROJECT_NAME}_PROTOBUF_PROTOC_EXECUTABLE}"
      IMPORTED_LOCATION_NOCONFIG "${${PROJECT_NAME}_PROTOBUF_PROTOC_EXECUTABLE}"
  )

  message(STATUS "using protoc found at: ${${PROJECT_NAME}_PROTOBUF_PROTOC_EXECUTABLE}")
endif()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

9 participants