Deduplicate overlay-ports handling and avoid repeated querying of the filesystem for 'port' vs 'subdirectories'

[BillyONeal]

This work originally added an "overlay-port-dirs" setting to allow a user to explicitly request the "the subdirectory contains port subdirectories" interpretation of an overlay-ports value rather than the "this is the overlay-port itself" interpretation.

After feedback from contributors, we re-evaluated adding this feature and decided to not do that. However, the infrastructure work that was done in support of the feature still has some value, so I've removed the user-facing part of the feature from this PR but kept most of the internal machinery.

Other notable behavior changes:

• Attempting to set overlay-ports to the provably same directory as a top-level manifest now generates an error, with a special error for "." in the configuration file. Note that overlay-triplets are not affected.
• Previously, the load_all_control_files family on OverlayPortProviderImpl would load all ports in the lowest priority --overlay-ports, then overwrite any of those with 'higher' values. This meant that malformed ports which vcpkg would never consider would be forced to be loaded, thus potentially triggering irrelevant errors.
• Previously, some paths that considered overlay ports in the "subdirectories of named overlay ports" would enforce that their contained CONTROL/vcpkg.json declared a matching name, and some paths did not. Now, all paths consider ports with mismatched names an error rather than silently using the name in vcpkg.json.
• Previously, there were additional fs.exists checks in order to determine if the indicated directory should contain the port or subdirectories that are ports; now, we try_read_contents and consider file not found as nonexistent, saving a disk operation.
• commands.install.cpp no longer gets entirely broken in the 'add the builtin ports directory as an overlay' cases if there is a vcpkg.json in %VCPKG_ROOT%/ports.

Related:

• Clarify the behavior of overlay-ports. vcpkg-docs#422 (Documentation update clarifying overlay-ports)

Resolves:

• https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1979597

[BillyONeal]

I think stopping at the first error rather than trying to return all of them would also be reasonable behavior, but that broke the ci e2e test so I kept this join behavior. (I did not keep it for different values of --overlay-ports....)

[BillyONeal]

I think we should get rid of this is_directory check but that would be product change I didn't need to make so I left it.

[BillyONeal]

The other vcpkgpaths.h things try very hard to only forward declare things. I did this because:

1. portfileprovider.h itself looks fairly lightweight.
2. This avoids needing to edit most users of VcpkgPaths::overlay_ports.

[BillyONeal]

I considered caching this but decided that the number of times try_load_all_ports is called in the same running vcpkg.exe is functionally 0 or 1 and thus it wouldn't be worth it.

[Osyotr]

I'd prefer overlay-ports to be fixed even if it means someone would break, instead of introducing a new option that has a similar name. IMHO

[Neumann-A]

I'd prefer overlay-ports to be fixed even if it means someone would break, instead of introducing a new option that has a similar name. IMHO

I think there is nothing to fix. Its a documentation issue at best.

who had a vcpkg.json containing their manifest, where they expected subdirectories to be treated as overlay ports

This is just a misunderstanding of the option.

[BillyONeal]

I'd prefer overlay-ports to be fixed even if it means someone would break, instead of introducing a new option that has a similar name. IMHO

What do you mean by 'fixed'? As far as I can tell the feature's behavior matches its intended behavior. That you can point overlay-ports at the port directory itself is not an accident, and it is documented behavior.

who had a vcpkg.json containing their manifest, where they expected subdirectories to be treated as overlay ports

This is just a misunderstanding of the option.

Perhaps. The problem is, without this feature, there was no way to configure what the user wanted.

[Neumann-A]

The problem is, without this feature, there was no way to configure what the user wanted.

Just assume if there is subdirectories with vcpkg.json in it that the whole directory is a collection of ports and otherwise directory is considered to be a directly a port?

As far as I understand the problem is:

ports
| - vcpkg.json (project? manifest)
| - port A
     | - vcpkg.json (port manifest)
| - port B
     | - vcpkg.json (port manifest)

In my opinion this directory structure does not make a lot of sense but you can decided to either error here or ignore the root vcpkg.json.

[BillyONeal]

In my opinion this directory structure does not make a lot of sense but you can decided to either error here or ignore the root vcpkg.json.

We have at least 2 independent reports of users trying to do exactly this.

[Neumann-A]

We have at least 2 independent reports of users trying to do exactly this.

Have you asked why they are doing that?
For overlay reuse it does not make sense to have projects manifest at the root of the overlay.

Ideally you would want something like this:

vcpkg-stuff
| - manifest (or without subfolder)
  | - vcpkg.json
  | - portfile.cmake.in (for auto generating the projects portfile and exporting it.)
  | - vcpkg-configuration.json
| - overlay-triplets (here it would be fun to look for triplets in same named subfolders 
  | - triplet1.cmake
  | - triplet2.cmake
  | - triplet3-folder (this is not allowed yet but would be nice to have. if triplet3.cmake is not found at the root. 
    | - triplet3.cmake
| - overlay-ports
  | - port A
    | - vcpkg.json (port manifest)
  | - port B
    | - vcpkg.json (port manifest)

but you can also go option b) and simply warn on the layout or ignore it. I don't see a reason to add another option even though that is the "safe" way.

[Neumann-A]

I mean you are adding a new option just for people with a strange file layout? Seems more like a case to tell them to adjust their layout to match what vcpkg would expect or come up with a default layout instead of adding another very similar option which makes learning about overlays more complicated.

[BillyONeal]

Have you asked why they are doing that?
For overlay reuse it does not make sense to have projects manifest at the root of the overlay.

'I want all the vcpkg things to be in the same directory' is a reasonable ask.

[Neumann-A]

'I want all the vcpkg things to be in the same directory' is a reasonable ask.

Then either:

• assume intention and ignore the top level manifest
• or force a layout for 'vcpkg things should be in the same directory'

For me the later option is the better route since it won't potentially break anybody.

(or add a switch (--ignore-top-level-manifest-in-overlay-ports) instead of basically the same option with slightly different behavior)

[Osyotr]

As far as I can tell the feature's behavior matches its intended behavior. That you can point overlay-ports at the port directory itself is not an accident, and it is documented behavior.

'I want all the vcpkg things to be in the same directory' is a reasonable ask.

Maybe it's better to add a note to the docs about this behavior and suggest to use a subdirectory for overlay-ports.

[autoantwort]

'I want all the vcpkg things to be in the same directory' is a reasonable ask.

Then either:

• assume intention and ignore the top level manifest

I would vote for that. Or is there a reasonable motivation for depending on the same manifest as port with a portfile?
It could print a warning that the ports should be ideally moved in a sub dir (for example called overlay-ports).

[BillyONeal]

assume intention and ignore the top level manifest

Unfortunately I believe one of the reports didn't use the same manifest as the one that happened to be reachable from the configured overlay-ports..... but at least warning about this seems like a good idea nonetheless.

[BillyONeal]

@ras0219-msft would like to add an explicit error if overlay-ports is provably the top-level manifest directory.

[BillyONeal]

For the record, I am in favor of allowing overlay-port-dirs as the directory with the top level manifest.

[BillyONeal]

@AugP @JavierMatosD @ras0219-msft @vicroms and I discussed this today. Based on internal feedback plus feedback from the contributors above, we have decided to not ship this feature.

I will retarget this PR to include the structural improvements to not have that be a wasted effort, and the docs PR that clarifies how overlay-ports works better.

[autoantwort]

I would expect that if Paragraphs::try_load_port is successful it loaded a port...

Also I find it not so nice that m_kind is initialized in the constructor (but not for all cases) and here again. Imho it would be nicer to determine that beforehand and maybe even have distinct types.

[BillyONeal]

I would expect that if Paragraphs::try_load_port is successful it loaded a port...

You want try_load_port_required for that. "There was no error but there is also no port of that name" is a valid success condition:

vcpkg-tool/include/vcpkg/paragraphs.h

Lines 38 to 41 in d176cbe

	// If an error occurs, the Expected will be in the error state.
	// Otherwise, if the port is known, the maybe_scfl.get()->source_control_file contains the loaded port information.
	// Otherwise, maybe_scfl.get()->source_control_file is nullptr.
	PortLoadResult try_load_port(const ReadOnlyFilesystem& fs, const PortLocation& port_location);

Also I find it not so nice that m_kind is initialized in the constructor (but not for all cases) and here again. Imho it would be nicer to determine that beforehand and maybe even have distinct types.

The problem is that determining this answer requires hitting the disk and we have no reason to do that if we don't happen to consider this overlay-ports entry in the first place.