[BUILD] Fix cross-compilation with protoc#3186
Conversation
✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3186 +/- ##
==========================================
- Coverage 87.86% 87.84% -0.01%
==========================================
Files 195 195
Lines 6151 6151
==========================================
- Hits 5404 5403 -1
- Misses 747 748 +1 |
Change-Id: I931ff0ad1951473acd56ae3f5778ac11f7499caa
| # directly for fallback | ||
| if(NOT PROTOBUF_PROTOC_EXECUTABLE) | ||
| set(PROTOBUF_PROTOC_EXECUTABLE protobuf::protoc) | ||
| endif() |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
Yes. You're right. We need this part.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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()
There was a problem hiding this comment.
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.
lalitb
left a comment
lalitb
left a comment
There was a problem hiding this comment.
Requesting clarification on - https://github.com/open-telemetry/opentelemetry-cpp/pull/3186/files#r1870315792. Else changes look good.
|
Could we add a CI task to verify the cross compilation? |
Change-Id: I931ff0ad1951473acd56ae3f5778ac11f7499caa
marcalff
changed the title
[BUILD] Fix cross-compilation with protoc (open-telemetry#3186)
5 participants
Change-Id: I931ff0ad1951473acd56ae3f5778ac11f7499caa
Changes
When cross-compiling, the
protocfound throughfind_package(Protobuf)is not a binary that can be executed on the host and cannot be run. In the case of cross-compilation, it is necessary to specify the executable protoc program on the host.