Ignore the VCPKG_ROOT environment variable when we detect a vcpkg_root. #725
Conversation
This resolves https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1634907 In actions/runner-images#6196 we have yet another CI system that is trying to helpfully set VCPKG_ROOT for customers to find their vcpkg instance which yet again breaks classic-mode-as-submodule customers. This change mitigates the issue by ignoring the environment variable when we detect a valid VCPKG_ROOT, which the user can override by using the command line option.
# Conflicts: # include/vcpkg/base/messages.h # locales/messages.en.json # locales/messages.json # src/vcpkg/base/messages.cpp
| @@ -5,13 +5,13 @@ if (-not $IsLinux -and -not $IsMacOS) { | |||
| $env:_VCPKG_TEST_UNTRACKED = "b" | |||
|
|
|||
| $x = Run-Vcpkg "--overlay-triplets=$PSScriptRoot/../e2e_ports/env-passthrough" env "echo %_VCPKG_TEST_TRACKED% %_VCPKG_TEST_TRACKED2% %_VCPKG_TEST_UNTRACKED% %_VCPKG_TEST_UNTRACKED2%" | |||
| if ($x -ne "%_VCPKG_TEST_TRACKED% %_VCPKG_TEST_TRACKED2% %_VCPKG_TEST_UNTRACKED% %_VCPKG_TEST_UNTRACKED2%") | |||
| if ($x -ne "%_VCPKG_TEST_TRACKED% %_VCPKG_TEST_TRACKED2% %_VCPKG_TEST_UNTRACKED% %_VCPKG_TEST_UNTRACKED2%`r`n") | |||
There was a problem hiding this comment.
Will this work on Linux? We should run the end-to-end tests on our Linux agent as well.
There was a problem hiding this comment.
We do. But this test starts with if (-not $IsLinux -and -not $IsMacOS) {. (The test wouldn't work on Linux or MacOS because they don't use %s)
| if (auto entry = obj.get(VCPKG_ROOT_ARG_NAME)) | ||
| { | ||
| auto as_sv = entry->string(VCPKG_LINE_INFO); | ||
| args.vcpkg_root_dir_arg.emplace(as_sv.data(), as_sv.size()); | ||
| } | ||
|
|
||
| if (auto entry = obj.get(VCPKG_ROOT_ENV_NAME)) | ||
| { | ||
| args.vcpkg_root_dir = std::make_unique<std::string>(entry->string(VCPKG_LINE_INFO).to_string()); | ||
| auto as_sv = entry->string(VCPKG_LINE_INFO); | ||
| args.vcpkg_root_dir_env.emplace(as_sv.data(), as_sv.size()); | ||
| } |
There was a problem hiding this comment.
These should probably be consolidated into a single, non-optional field -- the outer vcpkg instance will have already warned about any mismatching.
| if (auto vcpkg_root_dir_arg = args.vcpkg_root_dir_arg.get()) | ||
| { | ||
| obj.insert(VCPKG_ROOT_ARG_NAME, Json::Value::string(*vcpkg_root_dir_arg)); | ||
| } | ||
|
|
||
| if (auto vcpkg_root_dir_env = args.vcpkg_root_dir_env.get()) | ||
| { | ||
| obj.insert(VCPKG_ROOT_DIR_ENV, Json::Value::string(*args.vcpkg_root_dir.get())); | ||
| obj.insert(VCPKG_ROOT_ENV_NAME, Json::Value::string(*vcpkg_root_dir_env)); | ||
| } |
There was a problem hiding this comment.
These should probably be consolidated.
There was a problem hiding this comment.
The problem is that the decision of what to use depends on information that isn't known until later, in VcpkgPaths.
There was a problem hiding this comment.
(To clarify: Yes, the right thing to do is to determine what the correct thing is, and set that. I went with this more hacky solution so that I could get this out quickly without needing to substantially alter the relationship between VcpkgCmdArguments and VcpkgPaths.
* Add documentation of microsoft/vcpkg-tool#725 * Fix @Neumann-A's feedback
This resolves https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1634907
In actions/runner-images#6196 we have yet another CI system that is trying to helpfully set VCPKG_ROOT for customers to find their vcpkg instance which yet again breaks classic-mode-as-submodule customers. This change mitigates the issue by ignoring the environment variable when we detect a valid VCPKG_ROOT, which the user can override by using the command line option.