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

Also check if CONTROL is a file #574

Closed
wants to merge 2 commits into from

Conversation

Thomas1664
Copy link
Contributor

@Thomas1664 Thomas1664 commented Jun 7, 2022

Fixes microsoft/vcpkg#22686

We also may want to have Filesystem::file_exists()/ Filesystem::directory_exists().

@Thomas1664 Thomas1664 marked this pull request as ready for review June 7, 2022 16:40
@Thomas1664
Copy link
Contributor Author

There might be other places where we check this improperly. Maybe we need an e2e test for this?

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.

I think the actual fix is that we need to differentiate between a vcpkg.json that is a user/consumer manifest, vs a vcpkg.json that is a port. I don't think we want directories named control in ports either.

src/vcpkg/paragraphs.cpp Outdated Show resolved Hide resolved
Co-authored-by: Billy O'Neal <bion@microsoft.com>
BillyONeal added a commit to BillyONeal/vcpkg-tool that referenced this pull request Jun 10, 2022
… a consumer manifest.

Today, manifest mode works by effectively injecting the manifest directory as an overlay and letting load_port handle the situation. However, this means that the consuming manifest needs to follow all normal port rules, such as not having a CONTROL file. microsoft#574 attempts to partially fix this by not failing of the CONTROL found is a directory rather than a file, but we really shouldn't be having opinions about the consuming location at all.

Changes:
* Delete namespace PortFileProvider.
* Fix OverlayProviderImpl::load_port to be consistent with OverlayProviderImpl::load_all_control_files (reversing the order)
* Add ManifestProvider and CombinedProvider which explicitly model the loaded manifest overlay without needing to treat the manifest directory as a port directory
* Fix plumbing in install.cpp to use the same overlay provider at all times.
BillyONeal added a commit to BillyONeal/vcpkg-tool that referenced this pull request Jun 10, 2022
… a consumer manifest.

Today, manifest mode works by effectively injecting the manifest directory as an overlay and letting load_port handle the situation. However, this means that the consuming manifest needs to follow all normal port rules, such as not having a CONTROL file. microsoft#574 attempts to partially fix this by not failing of the CONTROL found is a directory rather than a file, but we really shouldn't be having opinions about the consuming location at all.

Changes:
* Delete namespace PortFileProvider.
* Fix OverlayProviderImpl::load_port to be consistent with OverlayProviderImpl::load_all_control_files (reversing the order)
* Add ManifestProvider and CombinedProvider which explicitly model the loaded manifest overlay without needing to treat the manifest directory as a port directory
* Fix plumbing in install.cpp to use the same overlay provider at all times.
BillyONeal added a commit to BillyONeal/vcpkg-tool that referenced this pull request Jun 10, 2022
… a consumer manifest.

Today, manifest mode works by effectively injecting the manifest directory as an overlay and letting load_port handle the situation. However, this means that the consuming manifest needs to follow all normal port rules, such as not having a CONTROL file. microsoft#574 attempts to partially fix this by not failing of the CONTROL found is a directory rather than a file, but we really shouldn't be having opinions about the consuming location at all.

Changes:
* Add ManifestProvider and CombinedProvider which explicitly model the loaded manifest overlay without needing to treat the manifest directory as a port directory
* Fix plumbing in install.cpp to use the same overlay provider at all times.

Competing resolution of microsoft#574 / microsoft/vcpkg#22686
@BillyONeal
Copy link
Member

I believe #582 is the correct fix.

@Thomas1664 Thomas1664 closed this Jun 10, 2022
BillyONeal added a commit that referenced this pull request Jun 14, 2022
… a consumer manifest. (#582)

* Don't complain about CONTROL files or directories in the directory of a consumer manifest.

Today, manifest mode works by effectively injecting the manifest directory as an overlay and letting load_port handle the situation. However, this means that the consuming manifest needs to follow all normal port rules, such as not having a CONTROL file. #574 attempts to partially fix this by not failing of the CONTROL found is a directory rather than a file, but we really shouldn't be having opinions about the consuming location at all.

Changes:
* Add ManifestProvider and CombinedProvider which explicitly model the loaded manifest overlay without needing to treat the manifest directory as a port directory
* Fix plumbing in install.cpp to use the same overlay provider at all times.

Competing resolution of #574 / microsoft/vcpkg#22686

* Add e2e test and make the manifest file the primary overlay.

* Add missing overrides.

* Degeneralize IOverlayProvider
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Directory named 'control' is mistaken as a 'CONTROL' file failing manifest mode of vcpkg
2 participants