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|toolchain] add VCPKG_OVERLAY_(PORTS|TRIPLETS) to the toolchain #13240

Conversation

Neumann-A
Copy link
Contributor

So people can add the overlays from cmake instead of modifying the environment

@JackBoosY JackBoosY added the category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed label Aug 31, 2020
@ras0219
Copy link
Contributor

ras0219 commented Sep 1, 2020

We hope to address this via settings in the manifest file; adding these settings via cmake is problematic because they won't be present when you run other commands like vcpkg search or vcpkg update.

@Neumann-A
Copy link
Contributor Author

We hope to address this via settings in the manifest file

this does not belong there ..... users should always be free to supply their own version of dependencies and have it not hardcoded! The same goes for toolchain/triplet settings.

via cmake is problematic because they won't be present when you run other commands

the manifest installed dir does not directly belong to the user but to the project and is put into ${CMAKE_BINARY_DIR}/vcpkg_installed as such it will not affect other commands.
Furthermore I can overwrite the corresponding env variable and get a similar behavior. Since messing with env variables within a cmake script is kind of messy I would rather like those to be true cmake variables.

@strega-nil
Copy link
Contributor

@Neumann-A the plan was to put it in the vcpkg-configuration.json file. This would allow overlay ports and triplets to be provided no matter what mode a person is using vcpkg in.

@Neumann-A
Copy link
Contributor Author

@strega-nil: As far as I understand vcpkg-configuration.json as well as vcpkg.json are meant for the project/library not for the user to mess with.
Putting more stuff into vcpkg.exe instead of cmake also seems wrong from my perspective. I want to have full control from the CMakeLists.txt not from some other location.

@strega-nil
Copy link
Contributor

@Neumann-A the vcpkg-configuration.json is absolutely meant for the user to mess with, we just don't have the capabilities for it yet because we haven't had the time to do the work just yet. Specifically, I'd like to add a vcpkg-configuration.user.json that would allow the user to change settings.

@Neumann-A
Copy link
Contributor Author

is absolutely meant for the user to mess with

In that case it does not belong into the CMAKE_SOURCE_DIR. Either way you probably will need a cmake variable to hold the path to a user supplied vcpkg-configuration.json in this case.

@strega-nil
Copy link
Contributor

fair enough, I guess.

@strega-nil
Copy link
Contributor

Hrm, I've changed my opinion on this after some cajoling from Billy. I think this feature should be merged.

Copy link
Contributor

@strega-nil strega-nil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the one minor suggestion (which doesn't need to be taken, I just think it might be a nicer design as a whole)

scripts/buildsystems/vcpkg.cmake Outdated Show resolved Hide resolved
@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Sep 10, 2020
@strega-nil strega-nil merged commit 59115f6 into microsoft:master Sep 11, 2020
@Neumann-A Neumann-A deleted the add_overlay_ports_triplets_to_cmake_manifest branch September 15, 2020 17:57
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

4 participants