[vcpkg] Implement Default-Features

[Squareys]

Hi @ras0219-msft !

As discussed on #2687, I am giving the implementation of default features a shot.

I have been playing around with a solution for the default features, trying to make it "feel clean". To make communication about the code easier, here's my WIP.

All feedback is greatly apprechiated. In case the "default" feature is to intrusive, I will start over based on your suggestion (already had this implemented at the time of the suggestion though, so might as well see what you think of this.)

Thanks!
Jonathan Hale

[Squareys]

@ras0219-msft This and the following location are they key diffs that make me believe this could be a valid solution.

[ras0219-msft]

From #2687 (comment):

>vcpkg install z0 // z0 depends on x0[core]
// should install defaults of x0 and z0 <- this the hard part

>vcpkg install x1[core,y]
>vcpkg install z1 // z1 depends on x1[core]
// should not install defaults of x1, but should install defaults of z1

>vcpkg install z2 x2[core,y] // z2 depends on x2[core]
// should not install defaults of x2, but should install defaults of z2

>vcpkg install x3[core,y]
>vcpkg upgrade
// should not install defaults of x3

>vcpkg install z4 x4 // z4 depends on x4[core]
// should install defaults of both x4 and z4

>vcpkg install z6[core,w] // z6 depends on x6[core]
// should not install defaults of z6 but should install defaults of x6

>vcpkg install x7[core,y]
>vcpkg install x7
// should install defaults of x7

The "idea" behind defaults is to represent what an uninformed user would think of the library. For example, let's take libarchive. You can actually build libarchive with essentially no compression algorithms (and it's pretty useless like that). But every compression algorithm should arguably listed as a separate feature.

Then, if my library depends on libarchive, but I don't need any particular compression algorithm, I should arguably depend on libarchive[core].

In the above configuration, if a user runs vcpkg install mylibrary, she might be missing some significant functionality out of the box but has no idea what libarchive is or why she should care. She just wants to write her program and saw that my library could do X, Y, and Z.

This might be combatted by duplicating every feature from libarchive into my package and making a suitable set be default, but that seems like a pretty terrible approach at scale.

Alternatively, we can recognize libarchive as a library that the user is "uninformed" of and install the defaults in the pursuit of producing a capable result by default. This is the approach I believe we should take.

Another example of where we apply this "informed" vs "uninformed" dynamic is when passing --head. Only the libraries listed on the install line are built using the latest upstream sources; the transitive dependencies are built using the latest stable as usual, because a failure in some random dependency 5 levels deep that you've never heard of is a poor experience ;p

[Squareys]

@ras0219-msft Thanks for putting so much effort into clarifying that!

One idea regarding that:
@mosra was able to convince me, that this is indeed sufficient and allowing package maintainers to specifiy default packages for its dependent packages is not really that useful.

I'm leaving the below, but it is not relevant anymore.

---

Could it possibly be even better to let you, the developer of the library (call it "archive++") that depends on libarchive, decide on what features the user of your library would expect?
This would split a feature package dependency into "Minimally required dependencies" and "default dependencies".

To illustrate what I mean, this could be an example CONTROL for archive++:

# archive++/CONTROL
Source: archive++
Version: 0.1.0
Build-Depends: libarchive[core]
Build-Depends-Default: libarchive[gzip,lzma]
Description: One up on the libarchive library
Default-Features: quantum-compression, zero-byte-compression

Feature: quantum-compression
Build-Depends: qpp[core]
Build-Depends-Default: qpp[openmp]
Description: Compress a file onto a Qubyte.

[...]

This would still "hide" the dependency from an uninformed user and make it useable, but give the control over that to the package maintainer rather than the maintainer of the dependent package. And as an "inexperienced vcpkg user", I would not expect a dependency foo[core] to be installed as foo.
In cargo this seems to be handled with a flag to disable default features, which is done in vcpkg by explicitly specifying "core". But now this explicit specification would be overwritten by installing the default after all. Assume you actually do just depend on "core", there would be no way of specifying that currently, which is why this doesn't feel quite right.

I am happy to implement as proposed anyway, if you considered the above but don't find that problematic. It would probably make it even more complicated to implement, anyway.

[Squareys]

@ras0219-msft If possible, some (very brief) feedback on the commits I just added would be apprechiated:

I implemented a first test in 26395fb0c4bd1dc55f1d515ff35e200c7421ad2f (and added documentation to everything I came across to aid my understanding). The test actually runs fine, I am usure why, as I didn't find default features implemented in any way yet, but maybe there is something already?

I am having slight difficulties understanding the remove_plan_check part. I guess that "remove" action is used to force a rebuild of the package when the features change?

Thanks in advance!

[ras0219-msft]

I am having slight difficulties understanding the remove_plan_check part. I guess that "remove" action is used to force a rebuild of the package when the features change?

Yep! When we detect a package needs to be rebuilt, it manifests as a RemovePlanAction followed later by an InstallPlanAction. In your test, you had added an already-installed-package (that's what the statusparagraphs tracks) as well as a new package definition (that's what the PackageSpecMap tracks).

I didn't look too closely, but create_feature_install_plan determined that you needed a package rebuilt in that context, so it was reinstalling the same features that already were installed (1 and core).

I've taken the liberty to reduce the test down to what I think you wanted originally -- now there are no installed packages and a single portfile with Default-Features: 1. This results in only a single step in the "plan": an InstallPlanAction which currently doesn't have feature 1 selected (but it should!). Please feel free to revert the commit if it's not helpful :).

and added documentation to everything I came across to aid my understanding

Those are great, thank you very much 👍 !

[Squareys]

@ras0219-msft That is super helpful, thank you very much!

[Squareys]

@ras0219-msft The implemented tests now run successfully.

How should the following two cases be handled:

> vcpkg install x1
// x1 adds a feature to its "default features" list
> vcpkg upgrade
// Currently no way of telling that the user originally requested the default features
// Should this install the added default feature?

> vcpkg install a1[core] // depends on b1[core]
// b1 default features are installed
// b1 adds a feature to their default list
> vcpkg upgrade
// Should this install the added default feature of b1?

I believe tested that the latter would install the new default package with the current implementation, as it only adds features of user-requested packages to the "don't install defaults" list.

The former could be solved with a RequestType::USER_REQUESTED_DEFAULTS or something, in case that is supposed to install the new feature.

[Squareys]

@ras0219-msft Pinging, just in case you missed the above message, no hurry.

[ras0219-msft]

I suppose an interesting question would be:

> vcpkg install a1[core]
// a1 adds a default feature
> vcpkg upgrade
// should this install the new default feature of a1?

I almost feel like it should, since this scenario is most likely mapping to "We're making something optional that was not optional before, so you should act like you've had it installed all along.".

Therefore, by the same logic, I think both of your above scenarios should also install new default packages.

This is a really good point, thanks for bringing it up!

---

Just to possibly clarify more: I think the logic should be "when rebuilding a package in a way that doesn't express explicit immediate desire to not install default features, any new default features should be added."

[Squareys]

@ras0219-msft Alright, that makes sense, but sounds tricky to implement. Is there a way to tell what changed in a CONTROL/SourceFile since the last upgrade?

[ras0219-msft]

Sorry for the delay!

The StatusParagraph/BinaryParagraph for the previously installed package should probably record the Default-Features, so that could be diff'd with the new list.

[Squareys]

@ras0219-msft Should be done!

If you need me to make any adjustments or changes/add some more tests, I'm happy to do so!

[ras0219-msft]

Thanks so much, these changes are awesome in every way (comments, tests, and functionality, oh my)!

[Squareys]

Thanks for merging! 👍 And thank you for all the feedback, help and clarifications.
It was fun working on this with you!

[ras0219-msft]

Just wanted to drop back by to say that Default-Features are working great in all my uses so far :)

https://github.com/Microsoft/vcpkg/blob/dev/roschuma/opencv-rework/ports/opencv/CONTROL

[Squareys]

Very happy to hear that, thanks! :D