Skip to content
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

Merged
merged 9 commits into from
Oct 13, 2020

Conversation

sandercox
Copy link
Contributor

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.

@sandercox sandercox marked this pull request as draft August 19, 2020 20:41
@sandercox sandercox marked this pull request as ready for review August 19, 2020 21:12
@BillyONeal
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@BillyONeal BillyONeal left a 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 Show resolved Hide resolved
scripts/buildsystems/vcpkg.cmake Outdated Show resolved Hide resolved
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)")
Copy link
Member

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)

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\"
Copy link
Member

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?

Copy link
Contributor Author

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.

@BillyONeal
Copy link
Member

Quoth @ras0219 @ras0219-msft perhaps we should create a new function to do this rather than overriding install.

@BillyONeal BillyONeal added category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed and removed requires:discussion labels Aug 20, 2020
@ras0219
Copy link
Contributor

ras0219 commented Aug 20, 2020

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 install command that don't apply to our existing build-only behavior (e.g. how does it interact with cpack?).

If it proves successful, then we can look at making it more automatic.

sandercox and others added 2 commits August 30, 2020 23:17
Co-authored-by: Billy O'Neal <bion@microsoft.com>
Co-authored-by: Billy O'Neal <bion@microsoft.com>
@sandercox
Copy link
Contributor Author

sandercox commented Aug 30, 2020

I must confess that I have no experience with CPack myself so I have no idea if this might interfere. I'm ok with a separate command (but that would probably just mean I'd override install in my own projects CMakeLists).

What do you think of the following name and usage for that:
vcpkg_instal_local_dependencies(<list of targets> [DESTINATION path])
So you give it a list of targets (like install is taking and have the option to specify the DESTINATION path for these targets. We can then use the built-in cmake parser for arguments to it would clean up the code quite a bit.

@BillyONeal
Copy link
Member

Ping @ras0219

@ras0219
Copy link
Contributor

ras0219 commented Sep 22, 2020

That would be fine, though please name it x_vcpkg_install_local_dependencies() to clarify that it is an experimental community component.

I'd also recommend that the list of targets is grouped into a "TARGETS" parameter

@sandercox
Copy link
Contributor Author

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 install function anymore.

@PhoebeHui PhoebeHui added the info:reviewed Pull Request changes follow basic guidelines label Oct 13, 2020
@BillyONeal BillyONeal merged commit 0a41fb2 into microsoft:master Oct 13, 2020
@BillyONeal
Copy link
Member

Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants