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

GH-44448: [C++] Add support for overwriting grpc_cpp_plugin path for cross-compiling #44507

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

kou
Copy link
Member

@kou kou commented Oct 23, 2024

Rationale for this change

We can't use find_package(gRPC) and gRPC::grpc_cpp_plugin for cross-compiling because it's for host. We need grpc_cpp_plugin for target in cross-compiling.

What changes are included in this PR?

Add ARROW_GRPC_CPP_PLUGIN CMake option that overwrites gRPC::grpc_cpp_plugin path found by find_package(gRPC).

Are these changes tested?

Yes.

conda-forge/arrow-cpp-feedstock#1432

Are there any user-facing changes?

Yes.

Copy link

⚠️ GitHub issue #44448 has been automatically assigned in GitHub to PR creator.

@h-vetinari
Copy link
Contributor

Thanks a lot for the quick PR!

It would be really nice if we could also change this where it's invoked, i.e.

COMMAND ${FLIGHT_PROTOC_COMMAND}
"--grpc_out=${CMAKE_CURRENT_BINARY_DIR}"
"--plugin=protoc-gen-grpc=$<TARGET_FILE:gRPC::grpc_cpp_plugin>"
"${FLIGHT_PROTO}")

Currently our builds scripts are doing

sed -ie "s;protoc-gen-grpc.*$;protoc-gen-grpc=${BUILD_PREFIX}/bin/grpc_cpp_plugin\";g" ../src/arrow/flight/CMakeLists.txt

to work around this...

@kou
Copy link
Member Author

kou commented Oct 23, 2024

This will work for the case because this overwrites $<TARGET_FILE:gRPC::grpc_cpp_plugin>.

@h-vetinari
Copy link
Contributor

Confirmed in conda-forge/arrow-cpp-feedstock#1432 that this works, even without the sed command 🥳

@kou kou changed the title GH-44448: [C++] Add support for overriding grpc_cpp_plugin path for cross-compiling GH-44448: [C++] Add support for overwriting grpc_cpp_plugin path for cross-compiling Oct 23, 2024
@kou
Copy link
Member Author

kou commented Oct 23, 2024

Thanks for confirming this. I'll merge this.

@kou kou merged commit 8eccbfe into apache:main Oct 23, 2024
36 of 37 checks passed
@kou kou deleted the cpp-grpc-plugin branch October 23, 2024 05:04
@kou kou removed the awaiting committer review Awaiting committer review label Oct 23, 2024
Copy link

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 8eccbfe.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.

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

Successfully merging this pull request may close these issues.

2 participants