Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

default-features of port always used in another port dependency #20925

Closed
Azrod-afk opened this issue Oct 22, 2021 · 16 comments
Closed

default-features of port always used in another port dependency #20925

Azrod-afk opened this issue Oct 22, 2021 · 16 comments
Assignees
Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed

Comments

@Azrod-afk
Copy link

Azrod-afk commented Oct 22, 2021

Describe the bug

I have a port with default-features :

{
  "name": "foo",
  "version": "1.1.2.2",
  "description": "Foo lib",
  "dependencies": [],
  "default-features": ["json", "protobuf"]
  "features": {
    "json": {
      "description": "Activate json support",
      "dependencies": [
        "jsoncpp"
      ]
    },
    "protobuf": {
      "description": "Activate protobuf support",
      "dependencies": [
        "protobuf"
      ]
    }
  }
}

I have another port that will use this port but only a specific feature :

{
  "name": "bar",
  "version": "13.4.0.1",
  "description": "Bar library",
  "dependencies": [],
  "default-features": ["foo"],
  "features": {
    "foo": {
      "description": "Use Foo library to support json",
      "dependencies": [
        {
          "name": "foo",
          "default-features": off,
          "features": [
            "json"
          ]
        }
      ]
    }
  }
}

When i want to fetch bar library, vcpkg fetch foo library with default-features (json and protobuf).
It's not correct because i specified that i don't want default features of foo library.

{
  "name": "myapp",
  "version": "1.4.3.127",
  "description": "Amazing application",
  "dependencies": [
    "bar"
  ]
}

vcpkg output:

The following packages will be rebuilt:
jsoncpp[core]:x64-linux-dynamic -> XXX -- /home/azrod/.cache/XXX
protobuf[core]:x64-linux-dynamic -> XXX -- /home/azrod/.cache/XXX
foo[core, json, protobuf]:x64-linux-dynamic -> 1.1.2.2 -- /home/azrod/.cache/XXX
bar[core, foo]:x64-linux-dynamic -> 13.4.0.1 -- /home/azrod/.cache/XXX

Environment

  • OS: Centos 7.9 x64
  • Compiler: g++ (GCC) 9.3.1 20200408 (Red Hat 9.3.1-2)
  • CMake: cmake version 3.20.3

To Reproduce
Steps to reproduce the behavior:
With all ports defined in a local registry :

  1. ./vcpkg install bar

output:
protobuf[core]:x64-linux-dynamic -> XXX -- /home/azrod/.cache/XXX
foo[core, json, protobuf]:x64-linux-dynamic -> 1.1.2.2 -- /home/azrod/.cache/XXX

protobuf feature of foo library is activate but bar library explicitly set default-features: off when fetching foo port

Expected behavior
Only json feature of foo library is activated when fetching bar port

Failure logs
We don't have error logs but it fetch unused port

Additional context
I use vcpkg with Cmake integration so i use cmake toolchain to be plugged in directly in cmake

@NancyLi1013 NancyLi1013 self-assigned this Oct 22, 2021
@NancyLi1013 NancyLi1013 added the category:question This issue is a question label Oct 22, 2021
@dg0yt
Copy link
Contributor

dg0yt commented Oct 22, 2021

It is "default-features": false instead of off.
(No error reported?)

@Azrod-afk
Copy link
Author

Sorry, i made a typo error when creating example json to demonstrate my issue.
In my concrete vcpkg port, i well use "default-features" : false instead of off.

@autoantwort
Copy link
Contributor

autoantwort commented Oct 22, 2021

So you want something like #19173. Default features are always installed (because they are default)

@dg0yt
Copy link
Contributor

dg0yt commented Oct 22, 2021

Default features are always installed (because they are default)

Isn't this a contradiction to the explicit "default-features": false, as above?

@autoantwort
Copy link
Contributor

No, default-features: false is for end users. Als an end user you can say that you don't want default features. For ports in manifest mode default-features: false has no effect, that is the default. So the port only depends on core of the other port, but by default default features are instelled. You have to set in the users vcpkg.json default-features: false to disable them really

@dg0yt
Copy link
Contributor

dg0yt commented Oct 22, 2021

TBH this is really irritating, even after reading twice #11602 (linked in #19173).

You have to set in the users vcpkg.json default-features: false to disable them really

So you are saying that setting "default-features": false in the user's manifest has a transitive effect on the "default-features": false statements in the ports suddenly taking effect?
(Or to complete the twist, the absence of the first has an effect on the latter?)
And there is no effect in classic mode?

It seems to be against the Principle of least surprise.

A key argument in #11602 is "to reduce the likelihood of needing to rebuild the world if someone adds dependencies incrementally". This argument does make sense from a vcpkg CI point of view (in particular given its current cache-drivenness). But vcpkg ci is a separate command which may choose a different behaviour. I don't think the argument justifies modifying the general behaviour ("for end users").
And it adds to the load of issues, for building undesired dependencies.

@Azrod-afk
Copy link
Author

I'm agree with @dg0yt, default features are activated by default when default-features is not explicitly set to false in user's manifest. But if user does not specify which features of dependent ports he want to use, he needs minimal features.

In this given example, myapp doesn't specify that it wants to use foo library with protobuf supports because it will not use it so why foo library has to be fetched with protobuf feature ?

I think it's more logical in this case that user has to activate more features than minimal if he need them.

@autoantwort
Copy link
Contributor

So you are saying that setting "default-features": false in the user's manifest has a transitive effect on the "default-features": false statements in the ports suddenly taking effect?

"default-features": false in ports and in the users vcpkg.json have a different meaning. In ports it means: The port bar does only depend on foo[core] (the default in manifest mode; in classic mode a dependency by default requires default features). In the end users vcpkg.json "default-features": false means don't install the default features.

image
The left vcpkg.json is the end users one. In classic mode it would install test-1[core] and test-2[test-feature] because of the above argument.

A key argument in #11602 is "to reduce the likelihood of needing to rebuild the world if someone adds dependencies incrementally"

That is also the argument for end user:

the reason is that in classic mode, we're trying to avoid as many installations as possible, and whenever you add features, we have to rebuild

I think it's more logical in this case that user has to activate more features than minimal if he need them.

No. If a normal end user normally don't want the default feature, then that shouldn't be a default feature, but default-features should be installed by default. For example curl has a default feature ssl. If I now use a package A that uses B that uses curl, I expect that A supports ssl because that is so common without enabling that manually for the curl dependency that is used somewhere down in the tree (I maybe don't even know that curl is used somewhere in my dependency tree because B is a http wrapper lib or something like that).

@dg0yt
Copy link
Contributor

dg0yt commented Oct 23, 2021

"default-features": false in ports and in the users vcpkg.json have a different meaning.

This is what I call surprising, in particular when you deal with both ports and projects.
Not to mention that there is another instance of default-features with yet another meaning (the list of default features).

It is also not very intuitive that the user needs to add direct dependencies on ports where we only wants trim features.

There seem to be some good arguments, but I don't think it is sufficiently explained in documentation.

@klalumiere
Copy link
Contributor

klalumiere commented Dec 14, 2021

From what I understand from the description, this issue is about "default-features": off, not being transitive. My colleague and I just stumble upon this behavior, and we were both surprise.

Just to give more context, I'll rewrite the description of the issue in a different way. Let's say we have a communication library:

{
    "name": "communication",
    "version": "1.0.0",
    "default-features": [ "curl" ],
    "features": {
        "curl": {
            "dependencies": [ "curl" ]
        }
    }
}

curl is a default feature because it is used a lot, and from the perspective of the author of the communication library, it makes sense. Now, let's say a different organisation owns the library

{
    "name": "fastinterprocesscommunication",
    "version": "2.0.0",
    "dependencies": [
        {
            "name": "communication",
            "default-features": false
        }
    ]
}

The authors of fastinterprocesscommunication (fast interprocess communication) know that they don't need curl in their particular case, so they disable it. Now, let's say I have an app

{
    "name": "myapp",
    "version": "3.0.0",
    "dependencies": [ "fastinterprocesscommunication" ]
}

When I run cmake .., this will install curl, since "default-features": false is not transitive. This is problematic, since I don't need it, and fastinterprocesscommunication don't need it either. Actually, I don't even know about the library communication: If fastinterprocesscommunication is "well" designed (they could for instance use pimpl idiom and link statically), I could be completely isolated from that dependency.

Of course, I can dig. One day, I could see in my cmake log that curl is being installed. Perhaps even it is the slowest dependency to install, or the biggest. Then I'd need to check why does fastinterprocesscommunication need it, only to realize that it doesn't. Then I'd have to dig to communication, and add an explicit dependency on communication to disable curl. This seems simple enough, but if you have dozens of dependencies and deep transitive dependencies, this can quickly become hard to manage.

There's also a stability to change problem. What if I add a dependency to communication just to add the "default-features": false that allows me to skip curl, and then, some day, fastinterprocesscommunication drop the dependency on communication. I may not know (or want to know) about this implementation detail that changed. However, in that case, I'd still pull on communication although nothing uses it. Perhaps worse, what if fastinterprocesscommunication starts to need curl from communication. Then, fastinterprocesscommunication would start to fail in myapp until I remove the explicit dependency on communication.

Regarding these two significant problems, I wonder: What are the advantages of making "default-features": false not transitive?

@autoantwort
Copy link
Contributor

curl is a default feature because it is used a lot, and from the perspective of the author of the communication library, it makes sense.

I think in this example curl should simply not a default feature. A better example is the png default feature of qtbase. One expect that qtbase is able to load png images, but libs that use qtbase don't depends on this functionality. But the end user expects that qtbase is able to load png images.

But you are probably interested in microsoft/vcpkg-tool#295

@klalumiere
Copy link
Contributor

In my example, the author of myapp wouldn't know that the library fastinterprocesscommunication depends on qtbase, so they would have no expectation about png 🙂 .

@horenmar
Copy link
Contributor

horenmar commented Dec 15, 2021

Found out about this today, when I noticed that my test package build takes 3x as long as building it's only dependency directly, due to the default-features fiasco.

I have multiple issues with this

  1. vcpkg is completely silent about this issue, even thought it should be trivially diagnosable: just warn me that my dependency is using default-features: false and that it is ignored
  2. I can maybe, sorta, possibly, see the reasoning for legacy mode, but in manifest mode it makes no sense to me. People shouldn't rely on transitive dependencies being overbuilt any more, then they should depend on transitive includes being present.
  3. It causes massive overbuilding and is not friendly to our caching when we develop both a library and its dependencies through vcpkg.
  4. IT BREAKS LICENCING. This is actually a massive problem. We use ffmpeg port, which with default features is covered by the GPL licence. Our only dependency that touches ffmpeg directly is careful to only enable features that leave the compiled dynamic library covered by LGPL, which we can adhere to. This "feature" makes it so that when the final customer facing product is built through vcpkg, it builds a GPLd version of ffmpeg. THIS IS A MASSIVE ISSUE

@klalumiere
Copy link
Contributor

I can maybe, sorta, possibly, see the reasoning for legacy mode, but in manifest mode it makes no sense to me. People shouldn't rely on transitive dependencies being overbuilt any more, then they should depend on transitive includes being present.

I'm still in the dark regarding this. In other words, the question I asked yesterday remains: What are the advantages of making "default-features": false not transitive?

I guess there is some advantages (even thought I can't think of them now). As I mentioned already, the only advantage I read in this discussion, "the user expect transitive dependencies to be built with their default features even if they've been disabled by another dependency in the chain", doesn't work. In a chain of dependency A -> B -> C -> D, the package A usually won't even know (or want to know) about D, except maybe when tools like Snyk find vulnerabilities (which is yet another downside: more default feature implies more attack surface). Hence, if C decide it doesn't need the default features of D, A will probably never know about this.

@horenmar
Copy link
Contributor

@klalumiere My understanding is that the behaviour in manifest mode comes from the behaviour in legacy mode, where it barely made some sense.

For context, legacy mode is doing vcpkg install some-port-or-another, and the underlying motivation was to avoid some recompilation by overcompiling dependencies in the first place. Because the full dependency tree was discovered incrementally, this was seen as worth it and nobody changed the underlying logic when manifest mode became a thing.

@dg0yt
Copy link
Contributor

dg0yt commented Dec 16, 2021

The situation is that I now need to explain reviewers why I add "defauĺt-features": false to dependencies on non-trivial ports. But this is the only way to allow user to opt-out from default features of the non-trivial port.
😞

@microsoft microsoft locked and limited conversation to collaborators Jan 13, 2022
@LilyWangLL LilyWangLL converted this issue into discussion #22522 Jan 13, 2022
@LilyWangLL LilyWangLL added category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed and removed category:question This issue is a question labels Jan 13, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed
Projects
None yet
Development

No branches or pull requests

7 participants