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

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

Closed
daschuer opened this issue May 5, 2022 · 20 comments
Assignees
Labels
category:question This issue is a question

Comments

@daschuer
Copy link
Contributor

daschuer commented May 5, 2022

I have create #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
Copy link
Contributor

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
Copy link
Contributor Author

daschuer commented May 5, 2022

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
Copy link
Contributor

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

#11602

@daschuer
Copy link
Contributor Author

daschuer commented May 5, 2022

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: #12216

Do you have a real live counter example?

@autoantwort
Copy link
Contributor

You are maybe interested in #19173 or a similar behavior

@daschuer
Copy link
Contributor Author

daschuer commented May 5, 2022

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

@daschuer
Copy link
Contributor Author

daschuer commented May 5, 2022

I have also added a comment to your microsoft/vcpkg-tool#177

@JackBoosY JackBoosY added the category:question This issue is a question label May 6, 2022
@dg0yt
Copy link
Contributor

dg0yt commented May 6, 2022

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

  • Feature convenience (defaults) vs. feature control (minimal)
  • Maintainer perspective (implementing options) vs. user perspective (consuming options)
  • Single, well-defined installation (CI) vs. incremental installation/removal (interactive)

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
Copy link
Contributor

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:

  • This is also what "default feature" means.
  • This will significantly reduce the number of reinstallations of this port: other ports may rely on these default features.

@daschuer
Copy link
Contributor Author

daschuer commented May 6, 2022

@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
Copy link
Contributor

@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
Copy link
Contributor Author

daschuer commented May 6, 2022

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
Copy link
Contributor

I will double confirm this with my team members later.

@daschuer
Copy link
Contributor Author

daschuer commented May 6, 2022

@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
Copy link
Contributor

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
Copy link
Contributor Author

daschuer commented May 6, 2022

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
Copy link
Contributor

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

@JackBoosY
Copy link
Contributor

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

@daschuer
Copy link
Contributor Author

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
Copy link
Contributor

@daschuer See #11602 for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:question This issue is a question
Projects
None yet
Development

No branches or pull requests

5 participants