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-tool] Allow overriding FETCHCONTENT_FULLY_DISCONNECTED #28489

Merged
merged 11 commits into from
Jan 18, 2023

Conversation

Thomas1664
Copy link
Contributor

@Thomas1664 Thomas1664 commented Dec 22, 2022

Describe the pull request

We can't simply remove FETCHCONTENT_FULLY_DISCONNECTED because this would lead to bugs for users that want to build on isolated machines. Therefore, we have to set this variable in vcpkg_configure_cmake() as well. Since using vcpkg_configure_cmake() is deprecated, it shouldn't break ports in private registries. If a user still needs FETCHCONTENT_FULLY_DISCONNECTED set to OFF, they can override it via OPTIONS.

github-actions[bot]
github-actions bot previously approved these changes Dec 22, 2022
@JonLiu1993 JonLiu1993 added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Dec 22, 2022
@ras0219-msft
Copy link
Contributor

While vcpkg_configure_cmake() is deprecated, that does not mean we can break existing ports. We must take #28483, which reverts the change only for vcpkg_configure_cmake().

However, we absolutely should support the override for vcpkg_cmake_configure() as you've done in this PR. I think @dg0yt's suggested improvement is best -- we need to move the vcpkg "preset" options above the user's supplied OPTIONS.

@Neumann-A
Copy link
Contributor

VCPKG_CMAKE_CONFIGURE_OPTIONS is designed to override all settings. So I don't get why this is even needed.

@dg0yt
Copy link
Contributor

dg0yt commented Dec 23, 2022

VCPKG_CMAKE_CONFIGURE_OPTIONS is designed to override all settings. So I don't get why this is even needed.

IMO it is generally bad to override anything which is already passed explicitly in OPTIONS. Because this is the primary interface for this purpose.
VCPKG_CMAKE_CONFIGURE_OPTIONS is nice to have for use in triplet files.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for vcpkg-cmake have changed but the version was not updated
version: 2022-12-22
old SHA: c36da1395054163a8caebd6b80ef464e1d33a451
new SHA: 0b89253931e4069bc34403386ce54d2a8f91bb26
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

@Thomas1664
Copy link
Contributor Author

We must take #28483, which reverts the change only for vcpkg_configure_cmake().

Why do we have to take #28483 before merging this PR? This would introduce a bug (see PR description above) until this PR gets merged.

github-actions[bot]
github-actions bot previously approved these changes Dec 30, 2022
@Cheney-W
Copy link
Contributor

@Thomas1664 Could you please handle the conflicts?

github-actions[bot]
github-actions bot previously approved these changes Jan 17, 2023
Cheney-W
Cheney-W previously approved these changes Jan 18, 2023
@Cheney-W Cheney-W added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jan 18, 2023
@BillyONeal BillyONeal merged commit e1934f4 into microsoft:master Jan 18, 2023
@BillyONeal
Copy link
Member

Thanks for your fix :). I merged Robert's change which had only baseline issues, merged that with this, and committed (as this has a green run).

@Thomas1664
Copy link
Contributor Author

Thanks for your fix :). I merged Robert's change which had only baseline issues, merged that with this, and committed (as this has a green run).

@BillyONeal Now FETCHCONTENT_FULLY_DISCONNECTED is not enabled in vcpkg_configure_cmake. What's wrong with setting it on this one as well? one can override it just like with vcpkg_cmake_configure.

@BillyONeal
Copy link
Member

What's wrong with setting it on this one as well?

It breaks backwards compatibility with old port versions.

@Thomas1664 Thomas1664 deleted the vcpkg-cmake branch January 20, 2023 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Forcing FETCHCONTENT_FULLY_DISCONNECTED is inappropriate and breaks projects using private registries
7 participants