-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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.
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
[vcpkg] VCPKG_APPINSTALL_DEPS install dependencies on install #1653 #13011
Conversation
…rride. Inspired by the work on microsoft#1653
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test of some sort?
scripts/buildsystems/vcpkg.cmake
Outdated
powershell -noprofile -executionpolicy Bypass -file \"${_VCPKG_TOOLCHAIN_DIR}/msbuild/applocal.ps1\" | ||
-targetBinary ${__VCPKG_INSTALL_DESTINATION}/$<TARGET_FILE_NAME:${TARGET}> | ||
-installedDir \"${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}$<$<CONFIG:Debug>:/debug>/bin\" | ||
-OutVariable out)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using OutVariable
and ignoring the results you probably want to record this and print the output on failure. (using OUTPUT_VARIABLE
and RESULT_VARIABLE
)
scripts/buildsystems/vcpkg.cmake
Outdated
foreach(TARGET ${__VCPKG_INSTALL_TARGETS}) | ||
install(CODE "message(\"-- Installing app dependencies for ${TARGET}...\") | ||
execute_process(COMMAND | ||
powershell -noprofile -executionpolicy Bypass -file \"${_VCPKG_TOOLCHAIN_DIR}/msbuild/applocal.ps1\" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this powershell be the one vcpkg fetch
grabs somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better, but this is just some copy & paste from the code in add_executable
with minimal changes.
Quoth @ras0219 @ras0219-msft perhaps we should create a new function to do this rather than overriding |
Thanks for the PR! I think it would be best to first introduce this functionality as an explicit function users can call -- this does mean it won't "just work", but there are some additional considerations with the If it proves successful, then we can look at making it more automatic. |
Co-authored-by: Billy O'Neal <[email protected]>
Co-authored-by: Billy O'Neal <[email protected]>
I must confess that I have no experience with What do you think of the following name and usage for that: |
Ping @ras0219 |
That would be fine, though please name it I'd also recommend that the list of targets is grouped into a "TARGETS" parameter |
Using TARGETS parameter as suggested. Renamed function and done some cleanup. Also made the optional config setting an experimental one with the x_ prefix. Wondering though if we should drop the setting as the command is now experimental anyway and no automatic install is happening as we're not overriding the default cmake |
Thanks for your contribution! |
This fixes the issue of #1653 - CMake: provide option to deploy DLLs on install() like VCPKG_APPLOCAL_DEPS
The option is VCPKG_APPINSTALL_DEPS only works on Windows platforms and takes the logic used in the add_executable / add_library part of the post build step to determine the libraries during install.