[vcpkg] Fix `find_program` search path host tools by ekilmer · Pull Request #31033 · microsoft/vcpkg

ekilmer · 2023-04-21T18:52:49Z

Changes comply with the maintainer guide
SHA512s are updated for each updated download
The "supports" clause reflects platforms that may be fixed by this new version
Any fixed CI baseline entries are removed from that file.
Any patches that are no longer applied are deleted from the port's directory.
The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
Only one version is added to each modified port's versions file.

Description

During port building, the scripts/buildsystems/vcpkg.cmake toolchain
file needs to know the host triplet in order to correctly set up
CMAKE_PROGRAM_PATH (to include the host triplet installation prefix)
and CMAKE_FIND_ROOT_PATH_MODE_PROGRAM (to prevent including
CMAKE_FIND_ROOT_PATH, which includes the target triplet's installation
prefix and always appears first in searching during find_* calls) so
that find_program only looks in the host triplet installation
prefix and system path(s).

We also remove the addition of the target triplet installation prefix to
CMAKE_PREFIX_PATH because these paths are always searched first in
find_program (and other find_* commands) and there is no way to
selectively turn it off for only find_program. Removing this doesn't
break things because we already set CMAKE_FIND_ROOT_PATH, which is
searched first by the find_* commands (unless you turn it off like we
do for find_program during crosscompilation).

The issue this commit solves can be evidenced by inspecting the search
paths of the find_* calls for CMake projects when host and target
triplets differ. The CMake search paths can be inspected by setting
CMAKE_FIND_DEBUG_MODE=ON.

For instance, on Intel x86_64 macOS, if we try to crosscompile fmt to
arm64, we should first modify ports/fmt/portfile.cmake and add
-DCMAKE_FIND_DEBUG_MODE=ON to vcpkg_cmake_configure and install:

./vcpkg \
  install \
  --host-triplet=x64-osx-release \
  --triplet=arm64-osx-release \
  --binarysource=clear \
  fmt

After it is done building, open the configuration log file
(buildtrees/fmt/config-arm64-osx-release-out.log) and notice the paths
listed after find_program considered the following locations. Before
this fix, the included paths contained the target triplet installation
prefix, which is incorrect. After this commit, only the host triplet and
host system paths are included, which is correct.

Note: macOS and different architectures aren't required to reproduce
the issue, but it makes this fix more compelling and obvious---you don't
want to find and run an executable of a different architecture. To
reproduce on any system, you only need to specify a different host and
target triplets.

During port building, the `scripts/buildsystems/vcpkg.cmake` toolchain file needs to know the host triplet in order to correctly set up `CMAKE_PROGRAM_PATH` (to include the host triplet installation prefix) and `CMAKE_FIND_ROOT_PATH_MODE_PROGRAM` (to prevent including `CMAKE_FIND_ROOT_PATH`, which includes the target triplet's installation prefix and always appears first in searching during `find_*` calls) so that `find_program` _only_ looks in the host triplet installation prefix and system path(s). We also remove the addition of the target triplet installation prefix to `CMAKE_PREFIX_PATH` because these paths are _always_ searched first in `find_program` (and other `find_*` commands) and there is no way to selectively turn it off for only `find_program`. Removing this doesn't break things because we already set `CMAKE_FIND_ROOT_PATH`, which is searched first by the `find_*` commands (unless you turn it off like we do for `find_program` during crosscompilation). --------------------------------------------------------------------- The issue this commit solves can be evidenced by inspecting the search paths of the `find_*` calls for CMake projects when host and target triplets differ. The CMake search paths can be inspected by setting `CMAKE_FIND_DEBUG_MODE=ON`. For instance, on Intel x86_64 macOS, if we try to crosscompile `fmt` to arm64, we should first modify `ports/fmt/portfile.cmake` and add `-DCMAKE_FIND_DEBUG_MODE=ON` to `vcpkg_cmake_configure` and install: ```sh ./vcpkg \ --host-triplet=x64-osx-release \ --triplet=arm64-osx-release \ --binarysource=clear \ fmt ``` After it is done building, open the configuration log file (`buildtrees/fmt/config-arm64-osx-release-out.log`) and notice the paths listed after `find_program considered the following locations`. Before this fix, the included paths contained the _target triplet_ installation prefix, which is incorrect. After this commit, only the host triplet and host system paths are included, which is correct. Note: macOS and different architectures aren't _required_ to reproduce the issue, but it makes this fix more compelling and obvious---you don't want to find and run an executable of a different architecture. To reproduce on any system, you only need to specify a different host and target triplets.

dg0yt · 2023-04-21T19:04:36Z

Be aware of the side effects. There may still be some ports which rely on the target's tools/<dependency>/bin/<dependency>-config scripts.
It is not enough to pass the target triplet when building ports. It must be set also for building user projects. This is the hard part, and the point where Extend triplet detection in CMake toolchain #25529 is waiting for things which must be delivered by the tool.

dg0yt · 2023-04-21T19:06:56Z

We also remove the addition of the target triplet installation prefix to
CMAKE_PREFIX_PATH

This may break pkg_check_modules from https://cmake.org/cmake/help/latest/module/FindPkgConfig.html

ekilmer · 2023-04-21T19:31:16Z

Be aware of the side effects. There may still be some ports which rely on the target's tools/<dependency>/bin/<dependency>-config scripts.

This is like depending on llvm-config or curl-config? These should be found through find_program, right? Do you have a good example? Otherwise, I will try searching or debugging what pops up in CI.

It is not enough to pass the target triplet when building ports. It must be set also for building user projects. This is the hard part, and the point where Extend triplet detection in CMake toolchain #25529 is waiting for things which must be delivered by the tool.

Do you mean not enough to pass the host triplet? Your PR seems to touch mostly manifest-mode logic, which fixes an additional issue and not everything that this PR fixes (or so I claim), right? This PR is still valid even with your PR?

We also remove the addition of the target triplet installation prefix to
CMAKE_PREFIX_PATH

This may break pkg_check_modules from https://cmake.org/cmake/help/latest/module/FindPkgConfig.html

😬 That looks a bit unfortunate... Thank you for pointing that out! I will investigate more.

Neumann-A · 2023-04-21T19:33:03Z

scripts/buildsystems/vcpkg.cmake

    endif()
    set("${list}" "${${list}}" PARENT_SCOPE)
 endfunction()
-z_vcpkg_add_vcpkg_to_cmake_path(CMAKE_PREFIX_PATH "")


Check the blame/history. As far as I remember there was a comment for the empty path.

Neumann-A · 2023-04-21T19:33:55Z

scripts/cmake/vcpkg_configure_cmake.cmake

    vcpkg_list(APPEND arg_OPTIONS
        "-DVCPKG_CHAINLOAD_TOOLCHAIN_FILE=${VCPKG_CHAINLOAD_TOOLCHAIN_FILE}"
        "-DVCPKG_TARGET_TRIPLET=${TARGET_TRIPLET}"
+        "-DVCPKG_HOST_TRIPLET=${HOST_TRIPLET}"


I think this file should never be touched again.

LilyWangLL · 2023-06-02T02:09:23Z

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

ekilmer · 2023-06-02T21:39:25Z

@LilyWangLL I have been unable to spend any time on this PR, but I still plan on revisiting when I have some time to sit down and figure out the cause of errors.

LilyWangLL · 2023-08-02T02:57:40Z

Closing this PR since it seems that no progress is being made. Please ping us to reopen if work is still being done.

ekilmer · 2024-01-16T23:36:15Z

@LilyWangLL Could you please reopen this (as a draft, until CI passes or not)? I've done more testing, and I think my new solution is better.

LilyWangLL · 2024-01-17T02:02:40Z

@LilyWangLL Could you please reopen this (as a draft, until CI passes or not)? I've done more testing, and I think my new solution is better.

The reopen button is invalid, and the message is: The fix-find-program-search-path-host-tools branch was force-pushed or recreated.
Could you please create a new PR?

ekilmer added 2 commits April 21, 2023 14:50

Update verison DB

24cb27b

Neumann-A reviewed Apr 21, 2023

View reviewed changes

Cheney-W assigned LilyWangLL Apr 23, 2023

LilyWangLL added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Apr 23, 2023

LilyWangLL closed this Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vcpkg] Fix `find_program` search path host tools#31033

[vcpkg] Fix `find_program` search path host tools#31033
ekilmer wants to merge 2 commits intomicrosoft:masterfrom
trail-of-forks:fix-find-program-search-path-host-tools

ekilmer commented Apr 21, 2023 •

edited

Loading

Uh oh!

dg0yt commented Apr 21, 2023

Uh oh!

dg0yt commented Apr 21, 2023

Uh oh!

ekilmer commented Apr 21, 2023

Uh oh!

Neumann-A Apr 21, 2023

Uh oh!

Neumann-A Apr 21, 2023

Uh oh!

LilyWangLL commented Jun 2, 2023

Uh oh!

ekilmer commented Jun 2, 2023

Uh oh!

LilyWangLL commented Aug 2, 2023

Uh oh!

ekilmer commented Jan 16, 2024

Uh oh!

LilyWangLL commented Jan 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

ekilmer commented Apr 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

dg0yt commented Apr 21, 2023

Uh oh!

dg0yt commented Apr 21, 2023

Uh oh!

ekilmer commented Apr 21, 2023

Uh oh!

Neumann-A Apr 21, 2023

Choose a reason for hiding this comment

Uh oh!

Neumann-A Apr 21, 2023

Choose a reason for hiding this comment

Uh oh!

LilyWangLL commented Jun 2, 2023

Uh oh!

ekilmer commented Jun 2, 2023

Uh oh!

LilyWangLL commented Aug 2, 2023

Uh oh!

ekilmer commented Jan 16, 2024

Uh oh!

LilyWangLL commented Jan 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ekilmer commented Apr 21, 2023 •

edited

Loading