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

[protobuf][protoc][grpc] Better support when cross compiling #26621

Closed

Conversation

ekilmer
Copy link
Contributor

@ekilmer ekilmer commented Aug 31, 2022

protoc tool should always be built and be compatible for the target
system.

Fix upb finding protoc executable to better support cross compilation.

Describe the pull request

  • What does your PR fix?

    N/A

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    Yes

  • Does your PR follow the maintainer guide?

    I believe so, yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    Yes

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

github-actions[bot]
github-actions bot previously approved these changes Aug 31, 2022
@Neumann-A
Copy link
Contributor

invalid. Use a host dependency

@JonLiu1993 JonLiu1993 added the category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. label Sep 1, 2022
@ekilmer
Copy link
Contributor Author

ekilmer commented Sep 1, 2022

Use a host dependency

Are you talking about using the "--host-triplet" option and this document https://github.com/microsoft/vcpkg/blob/master/docs/users/host-dependencies.md?

I am executing the following:

./vcpkg install --triplet=arm64-osx --host-triplet=x64-osx-release grpc[codegen]

with the (maybe weird) intention of building a vcpkg installation directory that can be delivered to an arm64 machine as a pre-built dependency cache (basically vcpkg export). Without these changes, I cannot precompile an arm64 protoc executable due to the host and target triplet check and copying of the host (x64) protoc to the target (arm64) tools directory.

There are also references to x64-osx-release in the CMake config files of the arm64-osx packages, which won't work when an arm64 machine tries to run things (with the gRPC plugins being the primary offender). I don't quite understand why there are *-vcpkg-tools.cmake and protobuf-targets-vcpkg-protoc.cmake files or what they're used for.

I understand that arm64 Macs can run x86 binaries with Rosetta, but I am going for correctness here by ensuring correct architecture binaries in their respective directories and CMake configurations that work when faced with the above scenario.

I am also now realizing that I don't understand the whole picture of how vcpkg handles cross-compilation. Is there a better way to accomplish what I'm trying to do?

github-actions[bot]
github-actions bot previously approved these changes Sep 1, 2022
@ekilmer ekilmer changed the title [protobuf][protoc] Better support when cross compiling [protobuf][protoc][grpc] Better support when cross compiling Sep 1, 2022
@ekilmer ekilmer force-pushed the fix-protobuf-cross-compile-pr branch from f07c54f to ada7387 Compare September 1, 2022 17:45
github-actions[bot]
github-actions bot previously approved these changes Sep 1, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/cartographer/vcpkg.json
  • ports/offscale-libetcd-cpp/vcpkg.json
  • ports/srpc/vcpkg.json

Valid values for the license field can be found in the documentation

github-actions[bot]
github-actions bot previously approved these changes Sep 2, 2022
`protoc` tool should always be built and be compatible for the target
system.

Fix upb finding protoc executable and support cross compilation.

grpc remove targets file
Comment on lines +30 to +38
add_custom_command(
OUTPUT ${common_protofiles}
- COMMAND protobuf::protoc
+ COMMAND "${Protobuf_PROTOC_EXECUTABLE}"
ARGS --cpp_out=${generated_path} -I ${protodir_path} ${protos}
- DEPENDS ${protos} protobuf::protoc
+ DEPENDS ${protos}
COMMENT "Running cpp protocol buffer compiler on ${protos}"
VERBATIM )
Copy link
Contributor

Choose a reason for hiding this comment

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

can we not simply fix the target protobuf::protoc for crosscompiling instead of patching everyone downstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I'm unsure of the best solution, and I'm still trying to wrap my head around CMake cross compilation best practices and what vcpkg is doing.

I was under the impression that it's probably best to expect that any exported targets from a find_package call are all the same architecture, OS, etc... However, as I keep investigating all of this, I wonder how useful an exported executable target is if it needs to be replaced or goes unused during cross compilation. It seems like a cross-compilation-compatible solution would either:

  1. Rely on find_program to find all host executables that will be run during the build
  2. Separate config files for library and executable targets where the user would need to either specify an exact *_DIR path to the host config file or (after much testing) rely on find_package with NO_CMAKE_PATH, NO_CMAKE_FIND_ROOT_PATH, and HINTS ${CMAKE_SYSTEM_PREFIX_PATH} options to search only the host directories,1 i.e.,
    find_package(Protobuf CONFIG HINTS ${CMAKE_SYSTEM_PREFIX_PATH} NO_CMAKE_PATH NO_CMAKE_FIND_ROOT_PATH)

fix the target protobuf::protoc

I believe what you're describing is attempted with these lines

find_program(Protobuf_PROTOC_EXECUTABLE NAMES protoc PATHS "${CMAKE_CURRENT_LIST_DIR}/../../tools/protobuf" NO_DEFAULT_PATH)

# Import target "protobuf::protoc" for configuration "Release"
set_property(TARGET protobuf::protoc APPEND PROPERTY IMPORTED_CONFIGURATIONS RELEASE)
set_target_properties(protobuf::protoc PROPERTIES
IMPORTED_LOCATION_RELEASE "${Protobuf_PROTOC_EXECUTABLE}"
)

I've removed these files in my PR, but I should investigate this again as you suggest. However, these files, being only in vcpkg, would hide downstream projects' issues if they tried to cross compile outside of vcpkg without also fixing the target. What I mean is that I think there is room for a patch in upstream protobuf for better cross compilation support, and I'm wondering what a good solution is and want to test it here.

list(INSERT CMAKE_SYSTEM_PREFIX_PATH 0 "${VCPKG_INSTALLED_DIR}/${VCPKG_HOST_TRIPLET}") # CMake 3.15 is required for list(PREPEND ...).

Footnotes

  1. Vcpkg currently doesn't touch CMAKE_SYSTEM_PREFIX_PATH, so we'd want to add the path to the host triplet installation root. I've done very light testing locally by adding the following to scripts/buildsystem/vcpkg.cmake:

@JonLiu1993
Copy link
Member

Pinging @ekilmer for response. Is work still being done for this PR?

@ekilmer
Copy link
Contributor Author

ekilmer commented Oct 12, 2022

@JonLiu1993 I think I need to approach this differently. I haven't had much time to investigate, so I'll close this for now.

Maybe I'll reopen this or another PR in the future when I have more time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants