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

fix: better support for cross-compilation #11782

Merged
merged 4 commits into from
Jun 6, 2023

Conversation

coryan
Copy link
Contributor

@coryan coryan commented Jun 5, 2023

Allow overrides for the protobuf::protoc and gRPC::grpc_cpp_plugin targets. On cross-compilation builds the targets point to the binaries for the target, but we need binaries that can run on the host build.

Fixes #11779


This change is Reviewable

Allow overrides for the `protobuf::protoc` and `gRPC::grpc_cpp_plugin`
targets. On cross-compilation builds the targets point to the binaries
for the *target*, but we need binaries that can run on the *host*
build.
@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (0c7f077) 93.79% compared to head (bac7581) 93.79%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #11782   +/-   ##
=======================================
  Coverage   93.79%   93.79%           
=======================================
  Files        1834     1834           
  Lines      165867   165867           
=======================================
+ Hits       155574   155576    +2     
+ Misses      10293    10291    -2     

see 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment on lines 206 to 209
if (NOT DEFINED _gRPC_CPP_PLUGIN_EXECUTABLE)
set(_gRPC_CPP_PLUGIN_EXECUTABLE
$<TARGET_FILE:grpc::grpc_cpp_plugin>)
endif ()

Choose a reason for hiding this comment

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

It should be safe even for cross-compilation to locate grpc-cpp wherever Protobuf_PROTOC_EXECUTABLE is. This is what we ended up doing for open-telemetry, for example.

That would save you a magical, private variable, but to be clear: that's a perfectly fine solution as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems intriguing. If I am following the suggestion one would install protoc and grpc_cpp_plugin in $PREFIX/bin or something similar? Unfortunately vcpkg installs protoc in $PREFIX/tools/protobuf/protoc and the gRPC plugins in $PREFIX/tools/grpc/*.

I can give that a try once I get something working across all our builds.

Copy link

@h-vetinari h-vetinari Jun 5, 2023

Choose a reason for hiding this comment

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

If I am following the suggestion one would install protoc and grpc_cpp_plugin in $PREFIX/bin or something similar?

To be honest I don't know how exactly CMake does it, but the idea is to search grpc_cpp_plugin relative to protoc. For cross-compilation (in conda-forge), the latter will be in $BUILD_PREFIX/bin, which will similarly be correct for grpc, while for native compilation, protoc will be in $PREFIX/bin and again that suits grpc too.

Copy link

@h-vetinari h-vetinari Jun 5, 2023

Choose a reason for hiding this comment

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

Unfortunately vcpkg installs protoc in $PREFIX/tools/protobuf/protoc and the gRPC plugins in $PREFIX/tools/grpc/*.

Yeah, that might mean it's not a directly suitable idea for you here. 😑

I don't know how the path traversal in find_program(<something> HINTS ${PATH} ...) would look like. Perhaps it's still salvageable, but we'd have to try to find out...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know how the path traversal in find_program(<something> HINTS ${PATH} ...) would look like. Perhaps it's still salvageable, but we'd have to try to find out...

Seems like we are getting to diminishing returns pretty quickly. The default works for most builds, and a CMake flag should work in all (I hope) other cases. Seems like each package manager would require a different HINT entry and at the point we might as well keep the setting in the package manager recipe.

Choose a reason for hiding this comment

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

My guess was that CMake will exhaustively search the entire folder hierarchy under HINT, and then the hint of "start with protoc" would also have been correct for vcpkg. But I don't care about this very much, it was just a thought to save you some complexity

@coryan coryan marked this pull request as ready for review June 6, 2023 15:13
@coryan coryan requested a review from a team as a code owner June 6, 2023 15:13
@coryan coryan enabled auto-merge (squash) June 6, 2023 15:20
@coryan coryan merged commit bab5b47 into googleapis:main Jun 6, 2023
@coryan coryan deleted the fix-some-cross-compilation-problems branch June 6, 2023 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support cross-compilation: accept a different protoc binary
3 participants