microsoft / vcpkg

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

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

Closed Azrod-afk closed 2 years ago

Azrod-afk commented 2 years ago

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

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

dg0yt commented 2 years ago

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

Azrod-afk commented 2 years ago

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 commented 2 years ago

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

dg0yt commented 2 years ago

Default features are always installed (because they are default)

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

autoantwort commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

"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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/microsoft/vcpkg-tool/pull/295

klalumiere commented 2 years ago

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 :slightly_smiling_face: .

horenmar commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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. :disappointed: