search

microsoft / vcpkg

C++ Library Manager for Windows, Linux, and MacOS
MIT License
23.16k stars 6.38k forks source link

vcpkg.cmake toolchain file not using triplet environment variables #23607

Open HunterZ opened 2 years ago

HunterZ commented 2 years ago

Describe the bug Documentation for using vcpkg with MinGW at https://github.com/microsoft/vcpkg/blob/master/docs/users/mingw.md says to define the following environment variables to point it to the correct triplet for the compiler:

export VCPKG_DEFAULT_TRIPLET=x64-mingw-dynamic
export VCPKG_DEFAULT_HOST_TRIPLET=x64-mingw-dynamic

Having done this, the vcpkg/scripts/buildsystems/vcpkg.cmake toolchain file still internally determines a triplet of x64-windows which it stores in my CMakeCache.txt like so:

//Vcpkg target triplet (ex. x86-windows)
VCPKG_TARGET_TRIPLET:STRING=x64-windows

The toolchain file should honor the environment variable setting! Having to set and environment variable and muck around with overriding CMake variables is terrible. This should also be mentioned in the article at https://github.com/microsoft/vcpkg/blob/master/docs/users/mingw.md because as written, those instructions don't get you to a working environment.

Environment

To Reproduce Steps to reproduce the behavior:

  1. install MSYS2+MinGW64 environment
  2. clone+install vcpkg into MSYS2 /opt/vcpkg, per instructions at https://github.com/microsoft/vcpkg/blob/master/docs/users/mingw.md
  3. set environment variables as mentioned above
  4. edit vscode cmake-tools config to use vcpkg toolchain file
  5. install libtcod package via vcpkg
  6. create CMake project with libtcod dependency, per libtcod package instructions

Expected behavior CMake configure succeeds in finding and configuring libtcod import target.

Failure logs log.txt

Additional context I was able to prove that VCPKG_TARGET_TRIPLET:STRING=x64-windows is the problem by manually changing the cache file setting to VCPKG_TARGET_TRIPLET:STRING=x64-mingw-dynamic.

JackBoosY commented 2 years ago

cc @dg0yt

JackBoosY commented 2 years ago 
[cmake] CMake Error at C:/msys64/opt/vcpkg/scripts/buildsystems/vcpkg.cmake:805 (_find_package):
[cmake]   Could not find a package configuration file provided by "libtcod" with any
[cmake]   of the following names:
[cmake] 
[cmake]     libtcodConfig.cmake
[cmake]     libtcod-config.cmake
JackBoosY commented 2 years ago

Can't repro in x86-windows triplet.

dg0yt commented 2 years ago

Failure logs log.txt

... shows no indication of x64-windows.

4. edit vscode cmake-tools config to use vcpkg toolchain file

Ensure that vscode uses the same values of the environment variables. The export ... instructions affect the shell session only, and everything which is started within that session.

In the end, I think this is a VS Code context problem. vcpkg.cmake does use the environment variables as expected, but it does not see the variable value you expect it to see.

Maybe it is easier if you skip 5. and start with manifest mode. This will ensure the right dependencies for your project's triplet.

HunterZ commented 2 years ago

Can't repro in x86-windows triplet.

That completely misses the point of the issue report, which is that the vcpkg.cmake toolchain file does not pick x64-mingw-dynamic as the triplet when invoked in a MinGW64 MSYS environment that has been configured per vcpkg instructions.

Failure logs log.txt

... shows no indication of x64-windows.

It's not my fault that vcpkg.cmake doesn't log the triplet determination anywhere. As mentioned in the "additional context" section in my issue report, however, I confirmed via inspection and modification of CMakeCache.txt that the problem is the incorrect triplet determination by vcpkg.cmake.

  1. edit vscode cmake-tools config to use vcpkg toolchain file

Ensure that vscode uses the same values of the environment variables. The export ... instructions affect the shell session only, and everything which is started within that session.

In the end, I think this is a VS Code context problem. vcpkg.cmake does use the environment variables as expected, but it does not see the variable value you expect it to see.

Maybe it is easier if you skip 5. and start with manifest mode. This will ensure the right dependencies for your project's triplet.

It's not vscode, it's vcpkg.cmake. Here, look, I've reproduced it purely on the command line: log2.txt

This log file is conclusive proof that vcpkg.cmake is not using the environment variables to inform triplet determination, and is therefore failing in a MinGW64 MSYS2 environment configured per https://github.com/microsoft/vcpkg/blob/master/docs/users/mingw.md

HunterZ commented 2 years ago

Update - I've dug into your vcpkg.cmake script via both inspection and cmake --trace commands, and it looks like there are actually two problems here:

  1. vcpkg.cmake doesn't reference environment variables VCPKG_DEFAULT_HOST_TRIPLET or VCPKG_DEFAULT_TRIPLET at all.
  2. The triplet determination logic performs an if(MINGW) check in an apparent attempt to detect a MinGW environment, but this fails due to the fact that a toolchain file runs long before CMake is able to invoke C:/msys64/mingw64/share/cmake-3.22.3/Modules/Platform/Windows-GNU.cmake in order to set that variable. This method of detecting MinGW is therefore fatally flawed.

It would probably be better to replace the if(MINGW) check with an MSYSTEM environment variable check, e.g. if("$ENV{MSYSTEM}" MATCHES "MINGW").

dg0yt commented 2 years ago

Let's take a step back and look at the documentation:

HunterZ commented 2 years ago

Let's take a step back and look at the documentation:

This is why I ultimately suggested the elseif("$ENV{MSYSTEM}" MATCHES "MINGW") (or similar) fix for vcpkg.cmake, which doesn't depend on those environment variables. I have tested this change in my own vcpkg install, and it successfully deduces the MinGW triplet. The currently implemented elseif(MINGW) check is bogus and not doing anyone any good.

I think it is important to understand that the initial triplet needs to be made permanent (by a CMake cache variable) to avoid messing up the build by accident, after changing the environment variable for another build, or after forgetting to set it after login.

Hold on, I'm pretty sure that's not how CMake cache variables work. Cache variables do not change after the cache is populated during configure, even when you re-run the configure step. The only ways they can change after cache creation are:

dg0yt commented 2 years ago

I think it is important to understand that the initial triplet needs to be made permanent (by a CMake cache variable) to avoid messing up the build by accident, after changing the environment variable for another build, or after forgetting to set it after login.

Hold on, I'm pretty sure that's not how CMake cache variables work. Cache variables do not change after the cache is populated during configure, even when you re-run the configure step. The only ways they can change after cache creation are:

* Use of `FORCE` in a script (which nobody is suggesting here)

* Manually editing them in the cache file (not relevant)

* Blowing away the entire cache and regenerating from scratch, in which case you absolutely _do_ want it to be affected by the current environment - and then it will stick because of the way cache variables work

That's why I'm saying. A cache variable is suited to make the triplet choice permanent enough.

HunterZ commented 2 years ago

That's why I'm saying. A cache variable is suited to make the triplet choice permanent enough.

I'm confused then how anything I've suggested contradicts that. vcpkg.cmake's triplet determination logic stores the result in a VCPKG_TARGET_TRIPLET cache variable when the cache is first generated, and then it doesn't change. Therefore it will always reflect the environment in which the CMake project was configured. Therefore making the determination based on environment variables when the project is configured is good.

Anyway, what about my elseif("$ENV{MSYSTEM}" MATCHES "MINGW") (or similar) suggested fix for vcpkg.cmake, which doesn't depend on those environment variables? I have tested this change in my own vcpkg install, and it successfully deduces the MinGW triplet. The currently implemented elseif(MINGW) check is bogus and not doing anyone any good.

dg0yt commented 2 years ago

Anyway, what about my elseif("$ENV{MSYSTEM}" MATCHES "MINGW") (or similar) suggested fix for vcpkg.cmake, which doesn't depend on those environment variables? I have tested this change in my own vcpkg install, and it successfully deduces the MinGW triplet. The currently implemented elseif(MINGW) check is bogus and not doing anyone any good.

It should be considered but would need to deal with the actual value.

IMO there are three options when VCPKG_TARGET_TRIPLET is not set.

Background:

HunterZ commented 2 years ago

It should probably be noted that the three options listed don't have to be mutually exclusive, as it would be possible to implement cascading logic that checks two or even all three of these conditions in some order of preference.

dg0yt commented 2 years ago

It should probably be noted that the three options listed don't have to be mutually exclusive, as it would be possible to implement cascading logic that checks two or even all three of these conditions in some order of preference.

The initial wording was "alternatives", but I changed that to "options" before posting ;-) IMHO it is better to keep the main toolchain file simple.

aminya commented 2 years ago

I am adding Mingw detection to project_options. My code invokes an external CMake process. Then in the current CMake process, I parse the CMakeCache.txt file to detect the compiler and its architecture.

However, I noticed that vcpkg doesn't respect VCPKG_DEFAULT_TRIPLET and VCPKG_DEFAULT_HOST_TRIPLET. It seems to be using MinGW variable and defaults to dynamic toolchain.
https://github.com/aminya/project_options/pull/126

How can I change this to use the static toolchain (if the license allows)?

dg0yt commented 2 years ago

While created independent of this issue, https://github.com/microsoft/vcpkg/pull/25529 might resolve the issue.

HunterZ commented 2 years ago

While created independent of this issue, #25529 might resolve the issue.

This seems like it might be a viable implementation of the third bullet under your previous list of options, as vcpkg z-print-config does seem to return the correct host_triplet in my MINGW64 environment:

$ vcpkg z-print-config
{
  "buildtrees": "C:\\msys64\\opt\\vcpkg\\buildtrees",
  "default_triplet": "x64-mingw-dynamic",
  "downloads": "C:\\msys64\\opt\\vcpkg\\downloads",
  "host_triplet": "x64-mingw-dynamic",
  "installed": "C:\\msys64\\opt\\vcpkg\\installed",
  "manifest_mode_enabled": false,
  "packages": "C:\\msys64\\opt\\vcpkg\\packages",
  "tools": "C:\\msys64\\opt\\vcpkg\\downloads\\tools",
  "vcpkg_root": "C:\\msys64\\opt\\vcpkg",
  "versions_output": "C:\\msys64\\opt\\vcpkg\\buildtrees\\versioning_\\versions"
}

Of course, this is because I have set the environment variables like so:

VCPKG_DEFAULT_HOST_TRIPLET=x64-mingw-dynamic
VCPKG_DEFAULT_TRIPLET=x64-mingw-dynamic

...and unsetting them reverts host_triplet to x64-windows.

Note that the proposed change does not address the likelihood that it's physically impossible for the elseif(MINGW) check to ever evaluate to true, thus making it almost certainly technical debt. It may be worth keeping this issue open to capture that loose end, even if/when the pull request is approved.

HunterZ commented 2 years ago

That's because the tool chain script doesn't bother to log its triplet determination. As I mentioned in my report, however, it recorded in my cake cache that it had erroneously chosen the x64-windows triplet.

vscode was launched from a mingw64 msys2 shell specifically so that those environment variables would be present. Is this not sufficient?

On Thu, Mar 17, 2022, 1:44 AM Kai Pastor @.***> wrote:

Failure logs log.txt https://github.com/microsoft/vcpkg/files/8282013/log.txt

... shows no indication of x64-windows.

  1. edit vscode cmake-tools config to use vcpkg toolchain file

Ensure that vscode uses the same values of the environment variables. The export ... instructions affect the shell session only, at everything which is started within that session.

In the end, I think this is a VS Code setup problem. vcpkg.cmake does use the environment variables as expected, but it does not see the variable value you expect it to see.

Maybe it is easier if you skip 5. and start with manifest mode. This will ensure the right dependencies for your project's triplet.

— Reply to this email directly, view it on GitHub https://github.com/microsoft/vcpkg/issues/23607#issuecomment-1070543201, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2TC4K4QO57NQO22KI253VALWFBANCNFSM5Q577QIA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

HunterZ commented 2 years ago

As described in the issue details and reproduction steps, this is specifically about trying to get gcpkg to use the mingw triplet.

Testing against x86-windows triplet is not a valid method to reproduce the issue.

On Wed, Mar 16, 2022, 11:53 PM Jack·Boos·Yu @.***> wrote:

Can't repro in x86-windows triplet.

— Reply to this email directly, view it on GitHub https://github.com/microsoft/vcpkg/issues/23607#issuecomment-1070382215, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2TC5OEZRYSHGPGDY2XNDVALJIFANCNFSM5Q577QIA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

JonLiu1993 commented 1 year ago

This issue hasn’t been updated in 3 month, if it is still an issue, please reopen this issue.

dg0yt commented 3 months ago

@JonLiu1993 Look, this issue is still relevant