Raise errors for malformed overlay port manifests

[dg0yt]

For microsoft/vcpkg#39276.

This PR will make vcpkg no longer tolerate broken overlay port manifests. Initially meant for vcpkg ci, the change also affects commands like install and search. This is acceptable: As long as broken ports are ignored, installation plans and results of "search" must be considered invalid.

Errors are raised for:

• malformed JSON (broken-manifests/malformed)
• missing name (broken-manifests/broken-no-name)
• missing version (broken-manifests/broken-no-version)

Note that errors were already raised before this change when --overlay-ports pointed directly to a single broken port.

No errors are raised for the remaining cases in ci/* (unchanged), including the test for missing dependencies of inactive features (new: ci/feature-missing-dep).

[Neumann-A]

This PR will make vcpkg no longer tolerate broken overlay port manifests.

What exactly is the behavior here? It shouldn't break on vcpkg install overlayA if overlayB is not used but broken.

[dg0yt]

This PR will make vcpkg no longer tolerate broken overlay port manifests.

What exactly is the behavior here? It shouldn't break on vcpkg install overlayA if overlayB is not used but broken.

vcpkg ci must fail if overlayB is broken. overlayB is vcpkg-ci-openimageio.

The effect on vcpkg install overlayA is debatable.

[dg0yt]

The effect on vcpkg install overlayA is debatable.

In particular, you cannot determine correct dependencies of overlayA if overlayB would overlay one of the dependencies.

[Neumann-A]

vcpkg ci must fail

I fully agree with that.

In particular, you cannot determine correct dependencies of overlayA if overlayB would overlay one of the dependencies.

that would mean overlayB is used. I am only interested in the case that overlayB is not used by anything and just exists. Overlays are for me working directories and erroring on incomplete and yet unused work is just annoying. A warning would be ok. Just don't straight out error for stuff which isn't required.

[dg0yt]

Overlays are for me working directories and erroring on incomplete and yet unused work is just annoying. A warning would be ok. Just don't straight out error for stuff which isn't required.

I understand the WIP nature of overlay port directories. However, silently ignoring (or just issueing a warning for) a potential overlay port just because @Neumann-A forgot to add a required comma isn't acceptable even for vcpkg install foo IMO. vcpkg must know the available ports to determine the installation plan.
But the only hard requirement is to be able to parse the manifest for the port name and version. In a more strict sense, only the port name is needed, and it is defined by the directory name [if the directory has a manifest].

[BillyONeal]

I think the correct behavior is that "if a port in an overlay directory would be considered, then errors therein need to be surfaced". vcpkg install foo --overlay-ports=broken\bar shouldn't explode because bar is malformed, unless it's named as a dependency of foo.

The ci command effectively names everything reachable, so the intended effect of exploding in the registry repo CI runs would still be achieved with this framing.

[dg0yt]

Hm, I don't think the problems exposed by the vcpkg-ci-openimageio fiasko are sufficiently understood.

The ci command effectively names everything reachable, so the intended effect of exploding in the registry repo CI runs would still be achieved with this framing.

vcpkg-ci-openimageio isn't used as a dependency, so its breakage went unnoticed.

Unnoticed even in presence of =pass in the baseline. Do we need to see this as a depedency? It is not unreasonable, but it wouldn't make the fix much easier. In particular if vcpkg eventually wants to move to "no baseline entry means =pass".

vcpkg install foo --overlay-ports=broken\bar shouldn't explode because bar is malformed, unless it's named as a dependency of foo.

The point is: ATM it will never explode "because bar is malformed". It will explode only when bar is missing.

Imagine it wasn't vcpkg-ci-openimageio but an openssl hotfix. vcpkg would silently use the main repo's vulnerable openssl because the author forgot one comma in the manifest. Is this acceptable?

[Neumann-A]

vcpkg would silently use the main repo's vulnerable openssl because the author forgot one comma in the manifest.

AS far as I know it is not silent since vcpkg prints the location where something is taken from. If it is from an external registry you would have to run x-add-version which would check the format. So the problem probably only really applies to overlays.
The problem is that changing the behavior to always break independent of actual requirements/dependencies breaks potentially existing users unnecessary. So the fix needs to only break if it is asked for, meaning you have a broken openssl in the overlay and you a requesting openssl to be build, then it should stop and yell at the user that openssl is broken.
Implementing this probably requires keeping a list of broken ports around and compare that against the requested stuff. The interesting cases then will be how to handle multiple overlays with different broken stuff, e.g.:
--overlay-ports=broken\foo --overlay-ports=broken\bar --overlay-ports=broken\abc

[dg0yt]

AS far as I know it is not silent since vcpkg prints the location where something is taken from.

IMO any overlay port modification which introduces a syntax error must lead to an immediate full error. Anything else, such as using a regular registry port, counts as "silent".

Yes, this change can cause interruption. That's why I tried to explicitly mention the effects in the top post.
But the only "bad" consequence is that users must fix (or remove) broken manifests.
It will not cause picking overlay ports which weren't pick before the change.
It might even turn some users' attentition to the fact that desired overlay ports where not used.

I still don't see the real benefit of broken manifest being silent ignored which justifies keeping that behavior. That's like keeping bugs unfixed because the it was always broken.

[BillyONeal]

Hm, I don't think the problems exposed by the vcpkg-ci-openimageio fiasko are sufficiently understood.

The ci command effectively names everything reachable, so the intended effect of exploding in the registry repo CI runs would still be achieved with this framing.

vcpkg-ci-openimageio isn't used as a dependency, so its breakage went unnoticed.

To ci, everything is referenced. Or, at least, should be.

Unnoticed even in presence of =pass in the baseline. Do we need to see this as a depedency? It is not unreasonable, but it wouldn't make the fix much easier. In particular if vcpkg eventually wants to move to "no baseline entry means =pass".

IMO this is a different bug but should also be fixed. If there's =pass but, for instance, the --overlay-ports part was missing in the command line, that should still fail, and this change wouldn't fix that.

vcpkg install foo --overlay-ports=broken\bar shouldn't explode because bar is malformed, unless it's named as a dependency of foo.

The point is: ATM it will never explode "because bar is malformed". It will explode only when bar is missing.

Do I misunderstand what this PR does then? It looks like this PR will make that explode even if the contents of that overlay-ports directory never participate in the plan whatsoever.

Imagine it wasn't vcpkg-ci-openimageio but an openssl hotfix. vcpkg would silently use the main repo's vulnerable openssl because the author forgot one comma in the manifest. Is this acceptable?

In that case, the name openssl participated in the plan, so a malformed openssl in an overlay-ports directory should explode.

My point is that names which never participate in the plan at all should not result in failures. For ci, all names in all overlay-ports directories participate in the plan, so they should indeed all result in failures.

[BillyONeal]

In the interest of making sure that this horse is dead, what I mean is that, given the following:

• broken-overlays/openssl
• overlays/openssl
• uniquely-broken-overlays/vcpkg-ci-openimageio

vcpkg install --overlay-ports=overlays curl[openssl] # should succeed

# should fail because a broken openssl overlay-port exists; I believe your PR fixes this
vcpkg install --overlay-ports=overlays --overlay-ports=broken-overlays curl[openssl]
vcpkg install --overlay-ports=broken-overlays curl[openssl]

# should succeed, there's no reason to ever have loaded the openssl overlay-port
# I believe your PR breaks this
vcpkg install --overlay-ports=broken-overlays vcpkg-cmake

# should fail because a broken openssl overlay-port was referenced
vcpkg ci --overlay-ports=broken-overlays

# should fail because a broken vcpkg-ci-openimageio overlay-port was referenced
# I believe your PR fixes this
vcpkg ci --overlay-ports=uniquely-broken-overlays

[BillyONeal]

I still don't see the real benefit of broken manifest being silent ignored which justifies keeping that behavior. That's like keeping bugs unfixed because the it was always broken.

I think it's reasonable to have directories in a directory passed to overlay-ports which aren't intended to be ports in the first place

[dg0yt]

I think it's reasonable to have directories in a directory passed to overlay-ports which aren't intended to be ports in the first place

That's still possible. They are just not allowed to have malformed vcpkg.json or CONTROL.

My point is that names which never participate in the plan at all should not result in failures.

My point is that vcpkg doesn't know if it participates in the plan at the time it loads the manifests.

[BillyONeal]

My point is that vcpkg doesn't know if it participates in the plan at the time it loads the manifests.

I believe that it does know that; the dependency planner doesn't read and parse every vcpkg.json or CONTROL in the repo before doing an install. Maybe this change is fine and that tests needed to change suggests some other bug. I'm taking a look...

[BillyONeal]

Indeed I tested this and load_overlay_ports never gets called while running install; this change might actually already do the thing Neumann-A and I are asking for.

[BillyONeal]

I tested and indeed this already works. I added an explicit test that this didn't break. Thanks for the fix!