-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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] Allow protobuf to be crosscompiled. #14165
Conversation
Protobuf port now asks for the host protoc, so .proto files can be built.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is always a need to generate protobuf tools.
ports/protobuf/portfile.cmake
Outdated
if(CMAKE_HOST_WIN32 AND NOT VCPKG_TARGET_ARCHITECTURE MATCHES "x64" AND NOT VCPKG_TARGET_ARCHITECTURE MATCHES "x86") | ||
if(CMAKE_HOST_WIN32) | ||
set(HOST_PROTOBUF_TRIPLET "x86-windows") | ||
if(NOT VCPKG_TARGET_ARCHITECTURE MATCHES "x64" AND NOT VCPKG_TARGET_ARCHITECTURE MATCHES "x86") | ||
set(protobuf_BUILD_PROTOC_BINARIES OFF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the uwp triplet also need to generate binaries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, it was disabled before my change, was there any reason why ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the reviewer ignored it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it is built on UWP.
NOT (VCPKG_TARGET_IS_WINDOWS AND NOT VCPKG_TARGET_IS_UWP) =>
NOT (TRUE AND NOT TRUE) =>
NOT (TRUE AND FALSE) =>
NOT (FALSE) =>
TRUE
Am i right ?
ports/protobuf/portfile.cmake
Outdated
if(protobuf_BUILD_PROTOC_BINARIES) | ||
file(GLOB EXECUTABLES ${CURRENT_PACKAGES_DIR}/bin/protoc*) | ||
foreach(E IN LISTS EXECUTABLES) | ||
file(INSTALL ${E} DESTINATION ${CURRENT_PACKAGES_DIR}/tools/${PORT} | ||
PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE GROUP_READ GROUP_WRITE GROUP_EXECUTE WORLD_READ) | ||
endforeach() | ||
else() | ||
file(GLOB EXECUTABLES ${_VCPKG_INSTALLED_DIR}/${HOST_PROTOBUF_TRIPLET}/tools/${PORT}/protoc*) | ||
foreach(E IN LISTS EXECUTABLES) | ||
file(INSTALL ${E} DESTINATION ${CURRENT_PACKAGES_DIR}/tools/${PORT}/ | ||
PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE GROUP_READ GROUP_WRITE GROUP_EXECUTE WORLD_READ) | ||
endforeach() | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We prefert to use vcpkg_copy_tools
instead of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem is that its not generic enought. protoc is a symbolic link on OS that are not Windows:
ll installed/x64-linux/tools/protobuf/
lrwxrwxrwx 1 root root 15 oct. 22 14:11 protoc -> protoc-3.13.0.0*
-rwxrwxr-- 1 root root 5602472 oct. 22 14:11 protoc-3.13.0.0*
So if I use vcpkg_copy_tools, it will only copy the symbolic link. And protoc has the version appended to it, thats why I use the wildcard instead of vcpkg_copy_tools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think we should expand vcpkg_copy_tools
instead of modifying it here. Please open an issue to report this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think we should expand
vcpkg_copy_tools
instead of modifying it here. Please open an issue to report this.
I think this is already tracked in #11547 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I wont open a new issue then.
ports/protobuf/portfile.cmake
Outdated
elseif(CMAKE_HOST_UNIX) | ||
set(HOST_PROTOBUF_TRIPLET "x64-linux") | ||
if(NOT VCPKG_TARGET_ARCHITECTURE MATCHES "x64" AND NOT VCPKG_TARGET_ARCHITECTURE MATCHES "x86") | ||
set(protobuf_BUILD_PROTOC_BINARIES OFF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above.
Co-authored-by: Jack·Boos·Yu <47264268+JackBoosY@users.noreply.github.com>
I can't find any VCPKG_HOST_ARCHITECTURE, any idea ? ${CMAKE_SYSTEM_PROCESSOR} and ${CMAKE_HOST_SYSTEM_PROCESSOR} seem empty. I'd like to do something like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ARM and ARM64 linux support
fa5194e
to
c615203
Compare
c615203
to
756068c
Compare
Requires cross-talk with #14318 |
Adding a new variable to VCPKG like VCPKG_HOST_TRIPLET would be nice for cross-compilation. Thats what I triied to achieve in this pr for protoc. |
To clarify: this PR tries to solve the cross compilation problems for protobuf, by repeating much of our existing triplet system within the protobuf port; we aren't fans of doing something like that because it's fragile, and it suggests that vcpkg the tool needs a more complete solution to these building a tool that we use in the build scenarios. The other PR #14318 solves a smaller subset of target architectures but gets both protobuf and grpc, so neither is a true superset of the other. Building some kind of support for "tool ports" which use the host rather than the target settings is certainly on our radar but we don't want to expand the existing workarounds drastically right now. Leaving this PR and the other open because at such time we address this problem more completely we're going to want to make sure both these are addressed well. |
Hi, yes I can understand that. |
Could you please resolve the conflicts? |
Is it actually worthing it ? BillyONeal seemed to say that this pr won't be merged until a suitable solution to build host tools is found. At least thats my understanding. |
@Nemirtingas I think @NancyLi1013 just saw this PR get pinged due to "reopening" and didn't follow the whole thread. |
Ah right, Thoses 2 "comment" buttons... I often hit the wrong one 🤐 |
Sorry for my above comments. As @BillyONeal said, I didn't follow the whole thread. |
Protobuf port now asks for the host protoc, so .proto files can be built.
Describe the pull request
It adds a check for the native protoc binary for cross-compilation like the case when you were cross-compiling on Windows for another OS.
What does your PR fix?
Fixes [protobuf] Handle cross-compilation on Linux host #10156 and possibly all cross-compilations for protoc.
Which triplets are supported/not supported?
Default Windows, Linux and MacOS. Didn't test the community pack;
Have you updated the CI baseline?
No
Does your PR follow the maintainer guide?
I think so.