microsoft / vcpkg

C++ Library Manager for Windows, Linux, and MacOS
MIT License
22.89k stars 6.31k forks source link

[vcpkg-tool] default-features are enabled for all dependencies even if not required. #24548

Closed daschuer closed 2 years ago

daschuer commented 2 years ago

I have create https://github.com/microsoft/vcpkg/pull/24539 to remove extra pointless dependencies form our target project. Unfortunately it works only half way round. Installing vamp-sdk only, still pulls in the default libsndfile with all dependencies into the project. The target project needs to ship pointless libraries just to satisfy the linker. In this case the vamp-host can only handle wav files but is now linked to libraries for all sort of files.

./vcpkg install vamp-sdk --dry-run
Computing installation plan...
The following packages will be built and installed:
  * libflac[core]:x64-linux -> 1.3.4
  * libogg[core]:x64-linux -> 1.3.5
  * libsndfile[core,external-libs,mpeg]:x64-linux -> 1.1.0
  * libvorbis[core]:x64-linux -> 1.3.7#2
  * mp3lame[core]:x64-linux -> 3.100#7
  * mpg123[core]:x64-linux -> 1.29.2#3
  * opus[core]:x64-linux -> 1.3.1#6
    vamp-sdk[core]:x64-linux -> 2.10#3
Additional packages (*) will be modified to complete this operation.

however, explicitly installing the libsndfile[core] does work:

./vcpkg install libsndfile[core] vamp-sdk --dry-run
Computing installation plan...
The following packages will be built and installed:
    libsndfile[core]:x64-linux -> 1.1.0
    vamp-sdk[core]:x64-linux -> 2.10#3

Is this an vcpkg-tool regression?

Environment All

To Reproduce See above

Expected behavior My expectation would be that I install the default features when installing the library explicit and installing the bare minimum when it is pulled in as dependency.

Neumann-A commented 2 years ago

Is this an vcpkg-tool regression?

No that is a design decisions. You need to be explicit about every dependency from which you don't want to have the defaults.

daschuer commented 2 years ago

I think I am explicit with ./vcpkg install vamp-sdk. I want to have the vamp-sdk, with all default features, but not 6 additional libraries, that are default when I ask explicit for libsndfile, but are useless here.

As vcpkg user I have not the knowledge to set all dependencies to just "core" to get the expected result. But If I need a feature of a dependent library directly, I will be able can ask for it explicit.

This is also interesting in vase of updates. Imagine a library does not longer depend on a library. It is just removed, including all its dependencies. If build script has to list all of them explicit, they are still installed without any use.

Since we don't know the requirement of the target software installing the default for named libraries is reasonable, but it is a wast of resources for dependent libraries where we definitely know that a certain feature is not required.

Do you have an example where this assumption is wrong? Is there a thread where this issue has already been discussed?

Neumann-A commented 2 years ago

Is there a thread where this issue has already been discussed?

11602

daschuer commented 2 years ago

I have read trough the whole discussion of #11602, but there was not a single argument why the current behavior is favored over the proposed, except that "it is a design decision".

I think the given real live example shows impressive that we should revise the design decision. Here is another one: https://github.com/microsoft/vcpkg/issues/12216

Do you have a real live counter example?

autoantwort commented 2 years ago

You are maybe interested in https://github.com/microsoft/vcpkg/issues/19173 or a similar behavior

daschuer commented 2 years ago

Yes, but I see no reason why such "--minimal-dependencies" feature is not the default. Do you?

daschuer commented 2 years ago

I have also added a comment to your https://github.com/microsoft/vcpkg-tool/pull/177

dg0yt commented 2 years ago

IMO there are competing perspectives and goals, e.g.

You cannot achieve them all equally well. I'm not enthusiastic about the current approach, but at least I don't have to care too much when adding a feature interface to existing ports (e.g. gui and icu for qt5-base), and I enjoy less rebuilds of installed packages when the next installed packages requests a new feature in a remote dependency.

JackBoosY commented 2 years ago

For the dependencies:

"dependencies": [
    {
        "name": "port-name",
        "default-features": false
    }
]

We should use this method in order to avoid consuming resources as much as possible on the installation of unnecessary default features.

For install, installing default features without declaring feature core is by design:

daschuer commented 2 years ago

@JackBoosY Do you propose that the example vcpkg.json fragment above of a of port a shall automatically install only port-name[core] when it is installed by ./vcpkg install a? And ./vcpkg install port-name will install port-name[defualt-features]?

JackBoosY commented 2 years ago

@daschuer If we set "default-features": false in manifest file, when installing the port, its dependency will not install the default features. Such as:

{
    "name": "a",
    ...
    "dependencies": [
        {
            "name": "b",
            "default-features": false
        }
    ]
}

./vcpkg install a will install a[core, default-features] and b[core].

daschuer commented 2 years ago

This is currently not the case unfortunately and the core part of this issue. (See my initial post).

Translated to the abstract example we have: Current state: ./vcpkg install a will install a[core, default-features] and b[core, default-features] and ./vcpkg install b[core] a will install a[core, default-features] and b[core]

JackBoosY commented 2 years ago

I will double confirm this with my team members later.

daschuer commented 2 years ago

@dg0yt

IMO there are competing perspectives and goals, e.g.

  • Feature convenience (defaults) vs. feature control (minimal)

If we change to the proposed mode we have both. Those, don't care about features and just use the port name get the default features of the library, but also no known-unnecessary dependencies, saving HDD and CPU.

  • Maintainer perspective (implementing options) vs. user perspective (consuming options) I enjoy less rebuilds of installed packages when the next installed packages requests a new feature in a remote dependency.

That's a fair point, for the current behavior and applies only if the new required feature is part of the default feature set. It comes at a cost of HDD and CPU of the initial build and the hassle for every user to strip down the install folder for the end user.

  • Single, well-defined installation (CI) vs. incremental installation/removal (interactive)

Currently we sit in the middle. The CI does not reflect the users setup because they will install the core version of the packages, but we have also no single well-defined installation (CI) because the packages can depend on any feature of another package not only default.

Maybe we tweak the CI that all packages are installed as default + features others are depending on, this mode will also add convenience when changing package dependencies. On the other hand we do not detect issues early with the package dependencies. For instance if a package a depends on b[core] and b splits out a feature in a later version, building a depends on, it will pass the CI but fail on the user machine.

autoantwort commented 2 years ago

Maybe we tweak the CI that all packages are installed as default + features others are depending on

Afaik this is already the case.

Imho installing default features of transitive dependencies is a good thing, but maybe ports have default features that shouldn't be default features. For example if a have a lib A that is a http curl wrapper, the lib depends on curl[core]. If I now depend on A and want to do a https query it would fail with your proposal, because it would not install the ssl support of curl, which is imho a bad user experience.
On the other side, if I install a lib that depends on aubio, it will install some random aubio tools. But imho the problem here is that the tools feature of aubio should not be a default feature and not that default features of transitive dependencies are installed by default.
So default features should be used rarely and only be used when a user expects this functionality from a library (like ssl support for curl ).

daschuer commented 2 years ago

I think we have the issue of a missing definition of default-feature, a rule set for it.

For me the default features of a library is what upstream considers as default. They have anyway the power to add or remove dependencies without making them a feature.

Imho installing default features of transitive dependencies is a good thing.

Can you please clarify when this applies? Do you have a real live example when it has make your work easier?

JackBoosY commented 2 years ago

After some discussion, I confirm this behavor is by design for both install command and the dependency relationship.

JackBoosY commented 2 years ago

We hope your question was answered to your satisfaction; if it wasn't, you can reopen with more info.

daschuer commented 2 years ago

I have not yet understand what is the main argument for the current behavior that overrules all others. Since I was not part of your internal discussion, I am not informed about your reasoning and If my valid arguments are considered and why they are not taken into account.

JackBoosY commented 2 years ago

@daschuer See https://github.com/microsoft/vcpkg/issues/11602 for more details.