[vcpkg] Extract common paths settings to make_cmake_cmd... #11842
Conversation
BillyONeal
added
category:vcpkg-bug
|
See also https:/microsoft/vcpkg/pull/11654/files#diff-7c78ff08e66ca1729315028943b36e19R401, which is similar to this |
| local_variables.emplace_back("PACKAGES_DIR", paths.packages); | ||
| local_variables.emplace_back("BUILDTREES_DIR", paths.buildtrees); | ||
| local_variables.emplace_back("_VCPKG_INSTALLED_DIR", paths.installed); | ||
| local_variables.emplace_back("DOWNLOADS", paths.downloads); |
There was a problem hiding this comment.
This function is in the base library (vcpkg/base) and thus intends to model the more generic concept of "invoke a CMake script with given definitions". Because these arguments are specific to vcpkg, I think it would be better to provide a non-base function like void Build::add_vcpkg_cmake_path_args(const VcpkgPaths&, std::vector<CMakeVariable>& out) which would be called from All The Places.
Alternatively, this function could be moved out of base and into the core code.
There was a problem hiding this comment.
Hmmm the bit that's a bit strange about that is the get_modified_clean_environment just below is also very 'vcpkg specific', so I'm not sure if we're maintaining that layering.
Is the objection to naming VcpkgPaths? (I think there's still some improvement here from centralizing the get_tool_exe call even if I factor out those variables...)
There was a problem hiding this comment.
Offline @ras0219 confirmed that he didn't like either naming VcpkgPaths or the explicit list here, so I introduced a new layer buildenvironment.h / buildenvironment.cpp. Ideally the explicit environment list would move there too since that's also very 'vcpkg specific' but I'm not sure exactly how to design that (since the 'base' get_clean_environment() depends on get_modified_clean_environment()) and I don't want to block this regression fix on that.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Works for me now. Thanks for the fix! |
…e and build share those variables. Resolves microsoft#11784
|
Robert confirmed this was OK offline. |
…so that create and build share those variables.
Resolves #11784