[BUILD] Fix cross-compilation with protoc

[seanchann]

Change-Id: I931ff0ad1951473acd56ae3f5778ac11f7499caa

Changes

When cross-compiling, the protoc found through find_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.

[netlify]

✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name	Link
🔨 Latest commit	d1d13fe
🔍 Latest deploy log	https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/6751050ff7ff440008a0cc41

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.84%. Comparing base (150256c) to head (d1d13fe).
Report is 1 commits behind head on main.

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     

see 1 file with indirect coverage changes

[lalitb]

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 ?

[seanchann]

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

[owent]

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.

[seanchann]

@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()

[marcalff]

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:

• https://cmake.org/cmake/help/latest/module/FindProtobuf.html

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]

Requesting clarification on - https://github.com/open-telemetry/opentelemetry-cpp/pull/3186/files#r1870315792. Else changes look good.

[ThomsonTan]

Could we add a CI task to verify the cross compilation?

[lalitb]

Thanks.

[marcalff]

LGTM, thanks for the fix.