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.

Sign up for GitHub

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

Jump to bottom

Extend triplet detection in CMake toolchain #25529

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
+19 −2
Draft

Extend triplet detection in CMake toolchain #25529

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
22be132
Always detect host triplet in manifest mode
dg0yt Jul 2, 2022
ddef0a3
Use 'VCPKG_DEFAULT_TRIPLET' for toolchain
dg0yt Jul 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions scripts/buildsystems/vcpkg.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,9 @@ if(NOT DEFINED CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO)
endif()
endif()

if(NOT DEFINED VCPKG_TARGET_TRIPLET AND NOT "$ENV{VCPKG_DEFAULT_TRIPLET}" STREQUAL "")
set(VCPKG_TARGET_TRIPLET "$ENV{VCPKG_DEFAULT_TRIPLET}")
endif()
Comment on lines +230 to +232
Copy link
Contributor

Choose a reason for hiding this comment

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

case should be moved into the if block below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered that, but I preferred to avoid another location which sets CACHE{VCPKG_TARGET_TRIPLET}.

Copy link
Contributor

Choose a reason for hiding this comment

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

For me a elseif(NOT "$ENV{VCPKG_DEFAULT_TRIPLET}" STREQUAL "") is logically clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

We cannot make this change because it breaks a common scenario:

  1. The user has set VCPKG_DEFAULT_TRIPLET to x64-windows because they (rightly) believe it should be the default for unqualified ports in the command line.
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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
Expand Down Expand Up @@ -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 "")
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In particular line 551:

vcpkg/scripts/buildsystems/vcpkg.cmake

Lines 549 to 574 in ddef0a3

option(VCPKG_SETUP_CMAKE_PROGRAM_PATH "Enable the setup of CMAKE_PROGRAM_PATH to vcpkg paths" ON)
set(VCPKG_CAN_USE_HOST_TOOLS OFF)
if(DEFINED VCPKG_HOST_TRIPLET AND NOT VCPKG_HOST_TRIPLET STREQUAL "")
set(VCPKG_CAN_USE_HOST_TOOLS ON)
endif()
cmake_dependent_option(VCPKG_USE_HOST_TOOLS "Setup CMAKE_PROGRAM_PATH to use host tools" ON "VCPKG_CAN_USE_HOST_TOOLS" OFF)
unset(VCPKG_CAN_USE_HOST_TOOLS)
if(VCPKG_SETUP_CMAKE_PROGRAM_PATH)
set(tools_base_path "${VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/tools")
if(VCPKG_USE_HOST_TOOLS)
set(tools_base_path "${VCPKG_INSTALLED_DIR}/${VCPKG_HOST_TRIPLET}/tools")
endif()
list(APPEND CMAKE_PROGRAM_PATH "${tools_base_path}")
file(GLOB Z_VCPKG_TOOLS_DIRS LIST_DIRECTORIES true "${tools_base_path}/*")
file(GLOB Z_VCPKG_TOOLS_FILES LIST_DIRECTORIES false "${tools_base_path}/*")
file(GLOB Z_VCPKG_TOOLS_DIRS_BIN LIST_DIRECTORIES true "${tools_base_path}/*/bin")
file(GLOB Z_VCPKG_TOOLS_FILES_BIN LIST_DIRECTORIES false "${tools_base_path}/*/bin")
list(REMOVE_ITEM Z_VCPKG_TOOLS_DIRS ${Z_VCPKG_TOOLS_FILES} "") # need at least one item for REMOVE_ITEM if CMake <= 3.19
list(REMOVE_ITEM Z_VCPKG_TOOLS_DIRS_BIN ${Z_VCPKG_TOOLS_FILES_BIN} "")
string(REPLACE "/bin" "" Z_VCPKG_TOOLS_DIRS_TO_REMOVE "${Z_VCPKG_TOOLS_DIRS_BIN}")
list(REMOVE_ITEM Z_VCPKG_TOOLS_DIRS ${Z_VCPKG_TOOLS_DIRS_TO_REMOVE} "")
list(APPEND Z_VCPKG_TOOLS_DIRS ${Z_VCPKG_TOOLS_DIRS_BIN})
foreach(Z_VCPKG_TOOLS_DIR IN LISTS Z_VCPKG_TOOLS_DIRS)
list(APPEND CMAKE_PROGRAM_PATH "${Z_VCPKG_TOOLS_DIR}")
endforeach()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It really still breaks finding the installed host pkgconf...

execute_process(
COMMAND "${Z_VCPKG_EXECUTABLE}" z-print-config
--vcpkg-root "${Z_VCPKG_ROOT_DIR}"
OUTPUT_VARIABLE Z_VCPKG_PRINT_CONFIG_LOGTEXT
ERROR_VARIABLE Z_VCPKG_PRINT_CONFIG_LOGTEXT
RESULT_VARIABLE Z_VCPKG_PRINT_CONFIG_RESULT
)
if("${Z_VCPKG_PRINT_CONFIG_LOGTEXT}" MATCHES [["host_triplet": "([^"][^"]*)"]])
set(VCPKG_HOST_TRIPLET "${CMAKE_MATCH_1}" CACHE INTERNAL "Host triplet")
else()
Comment on lines +475 to +477
Copy link
Contributor

Choose a reason for hiding this comment

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

use string(JSON) if cmake is new enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. But I respect the minimum version of CMake supported for user projects, which is 3.7 ATM.

Copy link
Member

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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"
}

message(STATUS "Determing host triplet before install - failed")
z_vcpkg_add_fatal_error("vcpkg install failed. See logs for more information: ${Z_VCPKG_BOOTSTRAP_LOG}")
endif()
endif()

list(APPEND Z_VCPKG_ADDITIONAL_MANIFEST_PARAMS "--host-triplet=${VCPKG_HOST_TRIPLET}")

if(VCPKG_OVERLAY_PORTS)
foreach(Z_VCPKG_OVERLAY_PORT IN LISTS VCPKG_OVERLAY_PORTS)
list(APPEND Z_VCPKG_ADDITIONAL_MANIFEST_PARAMS "--overlay-ports=${Z_VCPKG_OVERLAY_PORT}")
Expand Down