Extend triplet detection in CMake toolchain #25529
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if("${Z_VCPKG_PRINT_CONFIG_LOGTEXT}" MATCHES [["host_triplet": "([^"][^"]*)"]]) | ||
| set(VCPKG_HOST_TRIPLET "${CMAKE_MATCH_1}" CACHE INTERNAL "Host triplet") | ||
| else() |
There was a problem hiding this comment.
use string(JSON) if cmake is new enough
There was a problem hiding this comment.
Yes. But I respect the minimum version of CMake supported for user projects, which is 3.7 ATM.
There was a problem hiding this comment.
@BillyONeal would prefer to implement a command that only prints the required information (z-print-config --property=host-triplet or similar)
@ras0219-msft wants this to work for MSBuild too
There was a problem hiding this comment.
Seems implemented now:
E:\all\vcpkg>vcpkg z-print-config
{
"buildtrees": "E:\\all\\vcpkg\\buildtrees",
"default_triplet": "x64-windows-release",
"downloads": "E:\\vcpkg_cache\\downloads",
"host_triplet": "x64-windows-release",
"installed": "E:\\all\\vcpkg\\installed",
"manifest_mode_enabled": false,
"packages": "E:\\all\\vcpkg\\packages",
"tools": "E:\\vcpkg_cache\\downloads\\tools",
"vcpkg_root": "E:\\all\\vcpkg",
"versions_output": "E:\\all\\vcpkg\\buildtrees\\versioning_\\versions"
}
| if(NOT DEFINED VCPKG_TARGET_TRIPLET AND NOT "$ENV{VCPKG_DEFAULT_TRIPLET}" STREQUAL "") | ||
| set(VCPKG_TARGET_TRIPLET "$ENV{VCPKG_DEFAULT_TRIPLET}") | ||
| endif() |
There was a problem hiding this comment.
case should be moved into the if block below.
There was a problem hiding this comment.
I considered that, but I preferred to avoid another location which sets CACHE{VCPKG_TARGET_TRIPLET}.
There was a problem hiding this comment.
For me a elseif(NOT "$ENV{VCPKG_DEFAULT_TRIPLET}" STREQUAL "") is logically clearer.
There was a problem hiding this comment.
We cannot make this change because it breaks a common scenario:
- The user has set
VCPKG_DEFAULT_TRIPLETtox64-windowsbecause they (rightly) believe it should be the default for unqualified ports in the command line. - From then on, every project they configure will be forced to use x64-windows, even if it's x86/arm64/etc.
If a user wants to always use a certain vcpkg triplet, I think the right approach would be to create their own cmake toolchain file with set(VCPKG_TARGET_TRIPLET ...)\ninclude(../vcpkg.cmake), and use $CMAKE_TOOLCHAIN_FILE to automagically add it to every configure. VCPKG_DEFAULT_TRIPLET is only intended as a convenience to control the user's CLI interface.
There was a problem hiding this comment.
2. From then on, every project they configure will be forced to use x64-windows, even if it's x86/arm64/etc.
I fail to see the point. For new projects, it uses an environment variable named ...DEFAULT... to override the implicit default, but it doesn't cut the user's option to pass VCPKG_TARGET_TRIPLET to cmake. For existing projects, VCPKG_TARGET_TRIPLET is cached anyways, so this block will not have an effect.
JackBoosY
added
the
category:vcpkg-feature
Consider this proposal before approval/merge, to avoid another would rebuild for users. |
(And another change to |
JackBoosY
added
the
requires:vcpkg-team-review
|
Let's wait for some professional advices. |
|
Ping. 2 weeks with no feedback. |
vicroms
added
requires:author-response
and removed
requires:vcpkg-team-review
| if(NOT DEFINED VCPKG_TARGET_TRIPLET AND NOT "$ENV{VCPKG_DEFAULT_TRIPLET}" STREQUAL "") | ||
| set(VCPKG_TARGET_TRIPLET "$ENV{VCPKG_DEFAULT_TRIPLET}") | ||
| endif() |
There was a problem hiding this comment.
We cannot make this change because it breaks a common scenario:
- The user has set
VCPKG_DEFAULT_TRIPLETtox64-windowsbecause they (rightly) believe it should be the default for unqualified ports in the command line. - From then on, every project they configure will be forced to use x64-windows, even if it's x86/arm64/etc.
If a user wants to always use a certain vcpkg triplet, I think the right approach would be to create their own cmake toolchain file with set(VCPKG_TARGET_TRIPLET ...)\ninclude(../vcpkg.cmake), and use $CMAKE_TOOLCHAIN_FILE to automagically add it to every configure. VCPKG_DEFAULT_TRIPLET is only intended as a convenience to control the user's CLI interface.
| @@ -461,10 +464,24 @@ if(VCPKG_MANIFEST_MODE AND VCPKG_MANIFEST_INSTALL AND NOT Z_VCPKG_CMAKE_IN_TRY_C | |||
|
|
|||
| set(Z_VCPKG_ADDITIONAL_MANIFEST_PARAMS) | |||
|
|
|||
| if(DEFINED VCPKG_HOST_TRIPLET AND NOT VCPKG_HOST_TRIPLET STREQUAL "") | |||
| list(APPEND Z_VCPKG_ADDITIONAL_MANIFEST_PARAMS "--host-triplet=${VCPKG_HOST_TRIPLET}") | |||
| if(NOT DEFINED VCPKG_HOST_TRIPLET OR VCPKG_HOST_TRIPLET STREQUAL "") | |||
There was a problem hiding this comment.
I don't see the change to CMAKE_PROGRAM_PATH -- is that not yet added to this PR or is it a subtle side effect?
There was a problem hiding this comment.
In particular line 551:
vcpkg/scripts/buildsystems/vcpkg.cmake
Lines 549 to 574 in ddef0a3
There was a problem hiding this comment.
It really still breaks finding the installed host pkgconf...
JavierMatosD
added
requires:vcpkg-team-review
|
Why draft this PR? |
|
Because this comment asks for more work:
I might be able to do the tool stuff, but probably won't be able to solve the MSBuild task. |
|
So I will temporary remove |
JackBoosY
removed
the
requires:vcpkg-team-review
@dg0yt I am interested in this change being included. I've been experimenting with cross-compiling, and passing Apologies if the following is off-topic. I'm happy to make a new issue or a PR, but I didn't want to do a world rebuild if there's already a relevant PR. To expand, I have the following patch (and branch) to help fix host-tool finding (at least for CMake ports), which requires always setting the host triplet. Click to see vcpkg script patchdiff --git a/ports/vcpkg-cmake/vcpkg_cmake_configure.cmake b/ports/vcpkg-cmake/vcpkg_cmake_configure.cmake
index 832bbf700..83ab8c98d 100644
--- a/ports/vcpkg-cmake/vcpkg_cmake_configure.cmake
+++ b/ports/vcpkg-cmake/vcpkg_cmake_configure.cmake
@@ -159,6 +159,7 @@ function(vcpkg_cmake_configure)
vcpkg_list(APPEND arg_OPTIONS
"-DVCPKG_CHAINLOAD_TOOLCHAIN_FILE=${VCPKG_CHAINLOAD_TOOLCHAIN_FILE}"
"-DVCPKG_TARGET_TRIPLET=${TARGET_TRIPLET}"
+ "-DVCPKG_HOST_TRIPLET=${HOST_TRIPLET}"
"-DVCPKG_SET_CHARSET_FLAG=${VCPKG_SET_CHARSET_FLAG}"
"-DVCPKG_PLATFORM_TOOLSET=${VCPKG_PLATFORM_TOOLSET}"
"-DCMAKE_EXPORT_NO_PACKAGE_REGISTRY=ON"
diff --git a/scripts/buildsystems/vcpkg.cmake b/scripts/buildsystems/vcpkg.cmake
index cc6ed29d1..9028a346e 100644
--- a/scripts/buildsystems/vcpkg.cmake
+++ b/scripts/buildsystems/vcpkg.cmake
@@ -230,6 +230,10 @@ if(NOT DEFINED CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO)
endif()
endif()
+if(VCPKG_HOST_TRIPLET)
+ set(VCPKG_HOST_TRIPLET "${VCPKG_HOST_TRIPLET}" CACHE STRING "Vcpkg host triplet (ex. x86-windows)" FORCE)
+endif()
+
if(VCPKG_TARGET_TRIPLET)
# This is required since a user might do: 'set(VCPKG_TARGET_TRIPLET somevalue)' [no CACHE] before the first project() call
# Latter within the toolchain file we do: 'set(VCPKG_TARGET_TRIPLET somevalue CACHE STRING "")' which
@@ -239,7 +243,9 @@ if(VCPKG_TARGET_TRIPLET)
# configure call will see the user value as the more recent value. The same logic must be applied to all cache values within this file!
# The FORCE keyword is required to ALWAYS lift the user provided/previously set value into a CACHE value.
set(VCPKG_TARGET_TRIPLET "${VCPKG_TARGET_TRIPLET}" CACHE STRING "Vcpkg target triplet (ex. x86-windows)" FORCE)
-elseif(CMAKE_GENERATOR_PLATFORM MATCHES "^[Ww][Ii][Nn]32$")
+endif()
+
+if(CMAKE_GENERATOR_PLATFORM MATCHES "^[Ww][Ii][Nn]32$")
set(Z_VCPKG_TARGET_TRIPLET_ARCH x86)
elseif(CMAKE_GENERATOR_PLATFORM MATCHES "^[Xx]64$")
set(Z_VCPKG_TARGET_TRIPLET_ARCH x64)
@@ -357,6 +363,8 @@ if(EMSCRIPTEN)
set(Z_VCPKG_TARGET_TRIPLET_PLAT emscripten)
endif()
+# Default host triplet is same as default target triplet
+set(VCPKG_HOST_TRIPLET "${Z_VCPKG_TARGET_TRIPLET_ARCH}-${Z_VCPKG_TARGET_TRIPLET_PLAT}" CACHE STRING "Vcpkg host triplet (ex. x86-windows)")
set(VCPKG_TARGET_TRIPLET "${Z_VCPKG_TARGET_TRIPLET_ARCH}-${Z_VCPKG_TARGET_TRIPLET_PLAT}" CACHE STRING "Vcpkg target triplet (ex. x86-windows)")
set(Z_VCPKG_TOOLCHAIN_DIR "${CMAKE_CURRENT_LIST_DIR}")
and then I can use the changes to fix the target of protobuf/grpc tools. My first attempt was #26621, where there was a suggestion to fix the target instead of patching everyone downstream, which is now the following patch, and it seems to work when building the ports I had to modify in #26621. Click to see protobuf/grpc host tool target patchNote the changes to the diff --git a/ports/grpc/gRPCTargets-vcpkg-tools.cmake b/ports/grpc/gRPCTargets-vcpkg-tools.cmake
index 1ed3509c9..2500384dc 100644
--- a/ports/grpc/gRPCTargets-vcpkg-tools.cmake
+++ b/ports/grpc/gRPCTargets-vcpkg-tools.cmake
@@ -1,8 +1,10 @@
-file(GLOB GRPC_PLUGINS "${_IMPORT_PREFIX}/../@HOST_TRIPLET@/tools/grpc/grpc_*_plugin*")
+file(GLOB GRPC_PLUGINS "${_IMPORT_PREFIX}/../${VCPKG_HOST_TRIPLET}/tools/grpc/grpc_*_plugin*")
foreach(PLUGIN ${GRPC_PLUGINS})
get_filename_component(PLUGIN_NAME "${PLUGIN}" NAME_WE)
- add_executable(gRPC::${PLUGIN_NAME} IMPORTED)
+ if(NOT TARGET gRPC::${PLUGIN_NAME})
+ add_executable(gRPC::${PLUGIN_NAME} IMPORTED)
+ endif()
set_property(TARGET gRPC::${PLUGIN_NAME} APPEND PROPERTY IMPORTED_CONFIGURATIONS RELEASE)
set_target_properties(gRPC::${PLUGIN_NAME} PROPERTIES
IMPORTED_LOCATION_RELEASE "${PLUGIN}"
diff --git a/ports/grpc/portfile.cmake b/ports/grpc/portfile.cmake
index b4060a043..8e8ec995d 100644
--- a/ports/grpc/portfile.cmake
+++ b/ports/grpc/portfile.cmake
@@ -82,10 +82,10 @@ if (gRPC_BUILD_CODEGEN)
grpc_cpp_plugin
grpc_ruby_plugin
)
-else()
- configure_file("${CMAKE_CURRENT_LIST_DIR}/gRPCTargets-vcpkg-tools.cmake" "${CURRENT_PACKAGES_DIR}/share/grpc/gRPCTargets-vcpkg-tools.cmake" @ONLY)
endif()
+configure_file("${CMAKE_CURRENT_LIST_DIR}/gRPCTargets-vcpkg-tools.cmake" "${CURRENT_PACKAGES_DIR}/share/grpc/gRPCTargets-vcpkg-tools.cmake" COPYONLY)
+
file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug/share" "${CURRENT_PACKAGES_DIR}/debug/include")
vcpkg_copy_pdbs()
diff --git a/ports/protobuf/portfile.cmake b/ports/protobuf/portfile.cmake
index 102c6af3e..3fa5a1342 100644
--- a/ports/protobuf/portfile.cmake
+++ b/ports/protobuf/portfile.cmake
@@ -12,13 +12,13 @@ vcpkg_from_github(
compile_options.patch
)
-string(COMPARE EQUAL "${TARGET_TRIPLET}" "${HOST_TRIPLET}" protobuf_BUILD_PROTOC_BINARIES)
string(COMPARE EQUAL "${VCPKG_LIBRARY_LINKAGE}" "dynamic" protobuf_BUILD_SHARED_LIBS)
string(COMPARE EQUAL "${VCPKG_CRT_LINKAGE}" "static" protobuf_MSVC_STATIC_RUNTIME)
vcpkg_check_features(OUT_FEATURE_OPTIONS FEATURE_OPTIONS
FEATURES
zlib protobuf_WITH_ZLIB
+ codegen protobuf_BUILD_PROTOC_BINARIES
)
if(VCPKG_TARGET_IS_UWP)
@@ -40,7 +40,6 @@ vcpkg_cmake_configure(
-Dprotobuf_MSVC_STATIC_RUNTIME=${protobuf_MSVC_STATIC_RUNTIME}
-Dprotobuf_BUILD_TESTS=OFF
-DCMAKE_INSTALL_CMAKEDIR:STRING=share/protobuf
- -Dprotobuf_BUILD_PROTOC_BINARIES=${protobuf_BUILD_PROTOC_BINARIES}
-Dprotobuf_BUILD_LIBPROTOC=${protobuf_BUILD_LIBPROTOC}
${FEATURE_OPTIONS}
)
@@ -58,34 +57,36 @@ function(protobuf_try_remove_recurse_wait PATH_TO_REMOVE)
endif()
endfunction()
-protobuf_try_remove_recurse_wait("${CURRENT_PACKAGES_DIR}/debug/include")
-
-if(NOT DEFINED VCPKG_BUILD_TYPE OR VCPKG_BUILD_TYPE STREQUAL "release")
- vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/share/protobuf/protobuf-targets-release.cmake"
- "\${_IMPORT_PREFIX}/bin/protoc${VCPKG_HOST_EXECUTABLE_SUFFIX}"
- "\${_IMPORT_PREFIX}/tools/protobuf/protoc${VCPKG_HOST_EXECUTABLE_SUFFIX}"
- )
-endif()
+function(protobuf_fix_protoc_path PROTOC_EXECUTABLE_NAME)
+ if(NOT DEFINED VCPKG_BUILD_TYPE OR VCPKG_BUILD_TYPE STREQUAL "release")
+ vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/share/protobuf/protobuf-targets-release.cmake"
+ "\${_IMPORT_PREFIX}/bin/${PROTOC_EXECUTABLE_NAME}"
+ "\${Protobuf_PROTOC_EXECUTABLE}"
+ )
+ endif()
-if(NOT DEFINED VCPKG_BUILD_TYPE OR VCPKG_BUILD_TYPE STREQUAL "debug")
- file(READ "${CURRENT_PACKAGES_DIR}/debug/share/protobuf/protobuf-targets-debug.cmake" DEBUG_MODULE)
- string(REPLACE "\${_IMPORT_PREFIX}" "\${_IMPORT_PREFIX}/debug" DEBUG_MODULE "${DEBUG_MODULE}")
- string(REPLACE "\${_IMPORT_PREFIX}/debug/bin/protoc${EXECUTABLE_SUFFIX}" "\${_IMPORT_PREFIX}/tools/protobuf/protoc${EXECUTABLE_SUFFIX}" DEBUG_MODULE "${DEBUG_MODULE}")
- file(WRITE "${CURRENT_PACKAGES_DIR}/share/protobuf/protobuf-targets-debug.cmake" "${DEBUG_MODULE}")
-endif()
+ if(NOT DEFINED VCPKG_BUILD_TYPE OR VCPKG_BUILD_TYPE STREQUAL "debug")
+ file(READ "${CURRENT_PACKAGES_DIR}/debug/share/protobuf/protobuf-targets-debug.cmake" DEBUG_MODULE)
+ string(REPLACE "\${_IMPORT_PREFIX}" "\${_IMPORT_PREFIX}/debug" DEBUG_MODULE "${DEBUG_MODULE}")
+ string(REPLACE "\${_IMPORT_PREFIX}/debug/bin/${PROTOC_EXECUTABLE_NAME}" "\${Protobuf_PROTOC_EXECUTABLE}" DEBUG_MODULE "${DEBUG_MODULE}")
+ file(WRITE "${CURRENT_PACKAGES_DIR}/share/protobuf/protobuf-targets-debug.cmake" "${DEBUG_MODULE}")
+ endif()
+endfunction()
-protobuf_try_remove_recurse_wait("${CURRENT_PACKAGES_DIR}/debug/share")
+protobuf_try_remove_recurse_wait("${CURRENT_PACKAGES_DIR}/debug/include")
if(protobuf_BUILD_PROTOC_BINARIES)
if(VCPKG_TARGET_IS_WINDOWS)
vcpkg_copy_tools(TOOL_NAMES protoc AUTO_CLEAN)
+ protobuf_fix_protoc_path("protoc.exe")
else()
vcpkg_copy_tools(TOOL_NAMES protoc protoc-${version}.0 AUTO_CLEAN)
+ protobuf_fix_protoc_path("protoc-${version}.0${EXECUTABLE_SUFFIX}")
endif()
-else()
- file(COPY "${CURRENT_HOST_INSTALLED_DIR}/tools/${PORT}" DESTINATION "${CURRENT_PACKAGES_DIR}/tools")
endif()
+protobuf_try_remove_recurse_wait("${CURRENT_PACKAGES_DIR}/debug/share")
+
vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/share/${PORT}/protobuf-config.cmake"
"if(protobuf_MODULE_COMPATIBLE)"
"if(ON)"
@@ -120,9 +121,6 @@ endforeach()
vcpkg_fixup_pkgconfig()
-if(NOT protobuf_BUILD_PROTOC_BINARIES)
- configure_file("${CMAKE_CURRENT_LIST_DIR}/protobuf-targets-vcpkg-protoc.cmake" "${CURRENT_PACKAGES_DIR}/share/${PORT}/protobuf-targets-vcpkg-protoc.cmake" COPYONLY)
-endif()
+configure_file("${CMAKE_CURRENT_LIST_DIR}/vcpkg-cmake-wrapper.cmake" "${CURRENT_PACKAGES_DIR}/share/${PORT}/vcpkg-cmake-wrapper.cmake" COPYONLY)
-configure_file("${CMAKE_CURRENT_LIST_DIR}/vcpkg-cmake-wrapper.cmake" "${CURRENT_PACKAGES_DIR}/share/${PORT}/vcpkg-cmake-wrapper.cmake" @ONLY)
file(INSTALL "${SOURCE_PATH}/LICENSE" DESTINATION "${CURRENT_PACKAGES_DIR}/share/${PORT}" RENAME copyright)
diff --git a/ports/protobuf/protobuf-targets-vcpkg-protoc.cmake b/ports/protobuf/protobuf-targets-vcpkg-protoc.cmake
deleted file mode 100644
index 245adf560..000000000
--- a/ports/protobuf/protobuf-targets-vcpkg-protoc.cmake
+++ /dev/null
@@ -1,8 +0,0 @@
-# Create imported target protobuf::protoc
-add_executable(protobuf::protoc IMPORTED)
-
-# 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}"
-)
diff --git a/ports/protobuf/vcpkg-cmake-wrapper.cmake b/ports/protobuf/vcpkg-cmake-wrapper.cmake
index 542a16c2b..c7e483e9b 100644
--- a/ports/protobuf/vcpkg-cmake-wrapper.cmake
+++ b/ports/protobuf/vcpkg-cmake-wrapper.cmake
@@ -11,6 +11,6 @@ if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.3)
cmake_policy(POP)
endif()
-find_program(Protobuf_PROTOC_EXECUTABLE NAMES protoc PATHS "${CMAKE_CURRENT_LIST_DIR}/../../../@HOST_TRIPLET@/tools/protobuf" NO_DEFAULT_PATH)
+find_program(Protobuf_PROTOC_EXECUTABLE NAMES protoc PATHS "${CMAKE_CURRENT_LIST_DIR}/../../../${VCPKG_HOST_TRIPLET}/tools/protobuf" NO_DEFAULT_PATH)
_find_package(${ARGS})
diff --git a/ports/protobuf/vcpkg.json b/ports/protobuf/vcpkg.json
index 1a709ec62..17730dcbc 100644
--- a/ports/protobuf/vcpkg.json
+++ b/ports/protobuf/vcpkg.json
@@ -18,7 +18,11 @@
"host": true
}
],
+ "default-features": [ "codegen" ],
"features": {
+ "codegen": {
+ "description": "Build protoc code generator tool"
+ },
"zlib": {
"description": "ZLib based features like Gzip streams",
"dependencies": [ |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
The bgfx package could use this change in order to properly get host tools when cross compiling. Currently, the best solution is to inject Setting Using |
If you are building nativly just setting |
This requires knowing ahead of time what the host will be. For example, I currently have a cmake preset of a project in manifest mode with native (windows, linux, osx) and cross compile (android, ios, webasm, x86). These presets don't have Take ios development and github action building for example, the mac-13 runner is amd64 and the mac-latest is arm64. I would have to make a new preset to define the host based on the host architecture * the target platforms. webasm is another example afaik you can build this from mac, windows and linux. Then I have to consider if someone will want to build the project from a raspberry pi or a 32 bit windows etc. Those explode. |
What does your PR fix?
CMAKE_PROGRAM_PATH, selecting hosttoolsinstead of targettools(subject toVCPKG_USE_HOST_TOOLS).pkgconf(forfind_package(PkgConfig)) etc. readily available for manifest mode.curl-config.(This could be resolved by install such scripts to a different location than regular tools. They also need to reflect the build type.)
VCPKG_DEFAULT_TRIPLETto initialize the target triplet if unset.This merely complements the new behaviour for the default host triplet.
vcpkg.cmake toolchain file not using triplet environment variables #23607
Which triplets are supported/not supported? Have you updated the CI baseline?
unchanged, no
Does your PR follow the maintainer guide?
yes
If you have added/updated a port: Have you run
./vcpkg x-add-version --alland committed the result?not needed.
Remarks
Submitting this as draft, but for immediate feedback.
This might be complemented by passing
VCPKG_HOST_TRIPLETinvcpkg_cmake_configure.Background: #22392 (comment) ff. (CC @Neumann-A)