vcpkg icon indicating copy to clipboard operation
vcpkg copied to clipboard

Extend triplet detection in CMake toolchain

Open dg0yt opened this issue 3 years ago • 7 comments

  • What does your PR fix?

    • In manifest mode: If the host triplet is initially undefined , let the vcpkg tool determine it.
      • This changes the setup of CMAKE_PROGRAM_PATH, selecting host tools instead of target tools (subject to VCPKG_USE_HOST_TOOLS).
      • In turn, this makes tools like pkgconf (for find_package(PkgConfig)) etc. readily available for manifest mode.
      • However, it also breaks access to scripts like 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.)
    • In general: Use environment variable VCPKG_DEFAULT_TRIPLET to initialize the target triplet if unset. This merely complements the new behaviour for the default host triplet.
    • Related issues: https://github.com/microsoft/vcpkg/issues/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 --all and committed the result?

    not needed.

Remarks

Submitting this as draft, but for immediate feedback. This might be complemented by passing VCPKG_HOST_TRIPLET in vcpkg_cmake_configure. Background: https://github.com/microsoft/vcpkg/pull/22392#issuecomment-1162724678 ff. (CC @Neumann-A)

dg0yt avatar Jul 02 '22 07:07 dg0yt

This might be complemented by passing VCPKG_HOST_TRIPLET in vcpkg_cmake_configure.

Consider this proposal before approval/merge, to avoid another would rebuild for users.

dg0yt avatar Jul 06 '22 07:07 dg0yt

This might be complemented by passing VCPKG_HOST_TRIPLET in vcpkg_cmake_configure.

Consider this proposal before approval/merge, to avoid another would rebuild for users.

(And another change to vcpkg_find_acquire_program to accept pkgconf, to skip msys2 downloads.)

dg0yt avatar Jul 06 '22 07:07 dg0yt

Let's wait for some professional advices.

JackBoosY avatar Jul 11 '22 07:07 JackBoosY

Ping. 2 weeks with no feedback.

dg0yt avatar Jul 26 '22 17:07 dg0yt

Why draft this PR?

JackBoosY avatar Sep 16 '22 08:09 JackBoosY

Because this comment asks for more work:

@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

I might be able to do the tool stuff, but probably won't be able to solve the MSBuild task.

dg0yt avatar Sep 16 '22 10:09 dg0yt

So I will temporary remove vcpkg-team-review tag until this item is resolved.

JackBoosY avatar Sep 19 '22 02:09 JackBoosY

This might be complemented by passing VCPKG_HOST_TRIPLET in vcpkg_cmake_configure.

Consider this proposal before approval/merge, to avoid another would rebuild for users.

@dg0yt I am interested in this change being included. I've been experimenting with cross-compiling, and passing VCPKG_HOST_TRIPLET to vcpkg_cmake_configure is part of a changeset I wanted to propose, but then I found this PR.


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 patch
diff --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 patch

Note the changes to the vcpkg tool CMake files and the new usage of ${VCPKG_HOST_TRIPLET}.

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": [

ekilmer avatar Nov 14 '22 21:11 ekilmer

/azp run

Cheney-W avatar Feb 08 '24 02:02 Cheney-W

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Feb 08 '24 02:02 azure-pipelines[bot]

The bgfx package could use this change in order to properly get host tools when cross compiling.

Currently, the best solution is to inject PATH to find_program in the template cmake config which includes vcpkg-specific best guesses. This in turn becomes complicated with Apple moving away from x64 and into arm64, we have to add more possible "host" paths.

Setting VCPKG_HOST_TRIPLET in cmake before configuring a manifest project would be a nightmare explosion of cmake presets / duplicate code determining the platform.

Using HOST_TRIPLET during package build is also promising as a way to inject e.g. ${CMAKE_CURRENT_LIST_DIR}/../../../arm64-osx/tools/bgfx into find_program but could cause corner case issues if moving packages from different platforms.

bwrsandman avatar May 19 '24 17:05 bwrsandman

Setting VCPKG_HOST_TRIPLET in cmake before configuring a manifest project would be a nightmare explosion of cmake presets / duplicate code determining the platform.

If you are building nativly just setting VCPKG_HOST_TRIPLET should be enough

Neumann-A avatar May 22 '24 19:05 Neumann-A

If you are building nativly just setting VCPKG_HOST_TRIPLET should be enough

This requires knowing ahead of time what the host will be. On a project which can run on multiple platforms this is currently impossible (or at least prohibitively difficult to sus out).

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 VCPKG_HOST_TRIPLET defined.

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.

bwrsandman avatar May 22 '24 23:05 bwrsandman