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.

Sign up for GitHub

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

[BUILD] Fix cross-compilation with protoc #3186

Merged
merged 2 commits into from
Dec 6, 2024
Merged

[BUILD] Fix cross-compilation with protoc #3186

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cfd38b4
Fix the bug that protoc cannot be found in cross-compilation
seanchann Dec 4, 2024
d1d13fe
Fix the bug that protoc cannot be found in cross-compilation
seanchann Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,12 +428,16 @@ if(WITH_OTLP_GRPC
endif()
# Latest Protobuf imported targets and without legacy module support
if(TARGET protobuf::protoc)
project_build_tools_get_imported_location(PROTOBUF_PROTOC_EXECUTABLE
protobuf::protoc)
# If protobuf::protoc is not a imported target, then we use the target
# directly for fallback
if(NOT PROTOBUF_PROTOC_EXECUTABLE)
set(PROTOBUF_PROTOC_EXECUTABLE protobuf::protoc)
if(CMAKE_CROSSCOMPILING AND Protobuf_PROTOC_EXECUTABLE)
set(PROTOBUF_PROTOC_EXECUTABLE ${Protobuf_PROTOC_EXECUTABLE})
else()
project_build_tools_get_imported_location(PROTOBUF_PROTOC_EXECUTABLE
protobuf::protoc)
# If protobuf::protoc is not a imported target, then we use the target
# directly for fallback
if(NOT PROTOBUF_PROTOC_EXECUTABLE)
set(PROTOBUF_PROTOC_EXECUTABLE protobuf::protoc)
endif()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this PR has removed the fallback for cases where protobuf::protoc is not a target. Earlier there was elseif(Protobuf_PROTOC_EXECUTABLE) block to handle such scenarios ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. You're right. We need this part.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please also support cached PROTOBUF_PROTOC_EXECUTABLE variable? The old version of find script in cmake use PROTOBUF_PROTOC_EXECUTABLE, but the new version use Protobuf_PROTOC_EXECUTABLE.

Copy link
Contributor Author

@seanchann seanchann Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@owent Is this what you mean? Check the PROTOBUF_PROTOC_EXECUTABLE variable within the branch?

# Latest Protobuf imported targets and without legacy module support
  if(TARGET protobuf::protoc)
    if(CMAKE_CROSSCOMPILING AND Protobuf_PROTOC_EXECUTABLE)
      set(PROTOBUF_PROTOC_EXECUTABLE ${Protobuf_PROTOC_EXECUTABLE})
    else()
      project_build_tools_get_imported_location(PROTOBUF_PROTOC_EXECUTABLE
                                                protobuf::protoc)
      # If protobuf::protoc is not a imported target, then we use the target
      # directly for fallback
      if(NOT PROTOBUF_PROTOC_EXECUTABLE)
        set(PROTOBUF_PROTOC_EXECUTABLE protobuf::protoc)
      endif()
    endif()
  elseif(Protobuf_PROTOC_EXECUTABLE)
    # Some versions of FindProtobuf.cmake uses mixed case instead of uppercase
    set(PROTOBUF_PROTOC_EXECUTABLE ${Protobuf_PROTOC_EXECUTABLE})
  elseif(PROTOBUF_PROTOC_EXECUTABLE)
    set(PROTOBUF_PROTOC_EXECUTABLE ${PROTOBUF_PROTOC_EXECUTABLE})
  endif()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my understanding, @owent comment could apply here also:

    if(CMAKE_CROSSCOMPILING AND Protobuf_PROTOC_EXECUTABLE)
      set(PROTOBUF_PROTOC_EXECUTABLE ${Protobuf_PROTOC_EXECUTABLE})
    elseif(CMAKE_CROSSCOMPILING AND PROTOBUF_PROTOC_EXECUTABLE) # <----------- Fallback here
      set(PROTOBUF_PROTOC_EXECUTABLE ${PROTOBUF_PROTOC_EXECUTABLE})
    else()
...

According to CMake doc:

Changed in version 3.6: All input and output variables use the Protobuf_ prefix. Variables with PROTOBUF_ prefix are still supported for compatibility.

Given that opentelemetry-cpp requires CMake 3.10, using only Protobuf_PROTOC_EXECUTABLE seem sufficient.

endif()
elseif(Protobuf_PROTOC_EXECUTABLE)
# Some versions of FindProtobuf.cmake uses mixed case instead of uppercase
Expand Down
Loading