Dont enable default features for host dependencies #177

autoantwort · 2021-08-29T14:16:18Z

Fixes microsoft/vcpkg#19781, microsoft/vcpkg#35906

include/vcpkg/base/util.h

strega-nil-ms · 2021-10-04T19:15:35Z

I think this is not the correct way to do this; the best way to do this is just ask for default-features: false.

autoantwort · 2021-10-04T23:07:35Z

the best way to do this is just ask for default-features: false.

So I should run vcpkg install --dry-run, take a list of all ports that are a host dependency and add them with default-features: false to my vcpkg.json?

The function enable_default_features was only called when "core" was not in dep.features, but the function only had an effect when default_features was false, but it is only false when "core" is in dep.features. So the function does nothing at all.

strega-nil-ms · 2021-10-06T19:39:24Z

@autoantwort no, I think the best way is to fix the underlying problem: in manifest mode, dependencies of dependencies which don't depend on the default features should not get the default features.

ras0219-msft · 2021-10-07T10:05:12Z

in manifest mode, dependencies of dependencies which don't depend on the default features should not get the default features.

It is reasonable for this to be some sort of opt in, such as a setting in the manifest file that says "pretend I depended on [core] of every package in my tree". Consider that the consequence of using this with opencv as a transitive dependency would mean that user applications wouldn't actually be able to load any image files.

I think a separate setting that specifically affects host dependencies actually makes a lot of sense because those host dependencies can't affect the runtime of the application and so the reasoning behind default features isn't applicable.

strega-nil-ms · 2021-10-07T19:15:48Z

@ras0219-msft hmm. Alright then. I do think I'm not the right person to look at this though; can you take a look at the code and say if it makes sense?

autoantwort · 2021-10-08T09:37:11Z

It is reasonable for this to be some sort of opt in, such as a setting in the manifest file that says "pretend I depended on [core] of every package in my tree". Consider that the consequence of using this with opencv as a transitive dependency would mean that user applications wouldn't actually be able to load any image files.

Yeah this is microsoft/vcpkg#19173, but this PR does not implement this.

I think a separate setting that specifically affects host dependencies actually makes a lot of sense because those host dependencies can't affect the runtime of the application and so the reasoning behind default features isn't applicable.

Exactly, this is what I proposed in microsoft/vcpkg#19781 and implemented in this PR. But I have not added an option to enable default-features because I don't see a use case where you want default-features for host dependencies. Host dependencies are usually only build tools and generators.

include/vcpkg/base/util.h

src/vcpkg/dependencies.cpp

strega-nil-ms · 2021-10-18T19:04:49Z

src/vcpkg/dependencies.cpp

 VersionSchemeInfo& emplace_node(Versions::Scheme scheme, const Versions::Version& ver);

- PackageNode() = default;
+ PackageNode(bool only_host_dependency) : only_host_dependency(only_host_dependency){};


Suggested change

PackageNode(bool only_host_dependency) : only_host_dependency(only_host_dependency){};

explicit PackageNode(bool only_host_dependency) : only_host_dependency(only_host_dependency){};

This should also use an enum class.

In general I agree, but this would make the implementation very verbose. For Example only_host_dependency && host_dependency would be ((only_host_dependency == OnlyHostDependency::Yes) && (host_dependency == OnlyHostDependency::Yes)) ? OnlyHostDependency::Yes : OnlyHostDependency::No which seems very verbose.

@strega-nil-ms Any comment?

Co-authored-by: nicole mazzuca <[email protected]>

…encies # Conflicts: # src/vcpkg/build.cpp # src/vcpkg/dependencies.cpp

dg0yt · 2022-03-21T08:13:11Z

Can this be moved forward?
Qt5 could be a use case with effect on CI: microsoft/vcpkg#23668

…encies # Conflicts: # include/vcpkg/packagespec.h # src/vcpkg-test/dependencies.cpp # src/vcpkg/dependencies.cpp

autoantwort · 2022-03-25T21:14:15Z

This PR is currently wrong according to #292 iiuc. If I remember correctly A[x] -> B[x] -> C[x] -> D[x], root has a host dependency on A and normal on C and x is always a default feature it currently installs A, B, C[x], D[x] instead of A[x], B[x], C[x], D[x]

dg0yt · 2022-03-26T04:42:27Z

Hm, the focus of #292 was on a port being requested at least twice, with different feature options. This shouldn't drop the default features if at least one port doesn't exclude them.
But what if there is only one request (or: if there are only requests) with default-features disabled?

autoantwort · 2022-03-26T13:03:53Z

But what if there is only one request (or: if there are only requests) with default-features disabled?

Then they should be not enabled if it is a host dependency. This does work.

autoantwort · 2022-03-27T12:32:45Z

This PR is currently wrong according to #292 iiuc. If I remember correctly A[x] -> B[x] -> C[x] -> D[x], root has a host dependency on A and normal on C and x is always a default feature it currently installs A, B, C[x], D[x] instead of A[x], B[x], C[x], D[x]

If now everything is a [core] dependency (except from root to C), it now installs A, B, C[x], D[x] as expected

daschuer · 2022-05-05T16:47:33Z

I am in doubt that this approach fixes the root issue. I consider that a library maintainer knows exactly the dependencies of its own library, that can be another library, a header file or just a single feature of a library. It does not make a difference if this is a host or a target dependency.

For me the purpose of the default feature set is that a library maintainer can propose a set of feature, that allows the majority of user to use the library it without dealing with features at all. There is no reason the the package maintainer is empowered to override users decision and enable unwanted feature via a default feature set by default.

@ras0219-msft

It is reasonable for this to be some sort of opt in, such as a setting in the manifest file that says "pretend I depended on [core] of every package in my tree".

That does not work, because that are normally two persons. The package maintainer can state "default-features": false but the application developer gets unnecessary dependency in its build, many of them will never notice that.

Consider that the consequence of using this with opencv as a transitive dependency would mean that user applications wouldn't actually be able to load any image files.

I think you have such tree in mind:
<user-app> - <magic-for-png> - <opencv[core,png]>
Is this correct?

For me it is still obvious that this does not rectify to add something like tiff support to opencv. If the user-app likes to use tiff files directly with openvc it has to state that:
<user-app> - <magic-for-png> - <opencv[core,png]>
<user-app> - <opencv[core,tiff]>
vcpkg will install <opencv[core,tiff,png]> to satisfy both.
or he may just use the default if this suits best, and he don't want to deal with features.

If later the maintainer of <magic-for-png> decides that it does no longer depend on <opencv[core,png]> he can remove the dependency, and the <user-app> developer can use the updated vcpkg tree without regressions.

I think a separate setting that specifically affects host dependencies actually makes a lot of sense because those host dependencies can't affect the runtime of the application and so the reasoning behind default features isn't applicable.

I don't see a different in reasoning, we want only required dependencies that is true in the same way for host and target. There are different reasonable defaults though.

dg0yt · 2022-05-05T19:10:17Z

@daschuer It seems your comment here is not related to the host dependencies aspect. Please move it to one of the other places.

daschuer · 2022-05-05T21:17:51Z

It is an answer to @ras0219-msft post above.

dg0yt · 2022-07-22T11:19:56Z

Ping for re-consideration. It is a significant difference for host utilities from qt5-tools which don't need GUI, cf. microsoft/vcpkg#23668. And gui really needs to remain a default feature, no matter if native or cross build.

dg0yt · 2022-07-23T12:22:56Z

It is a significant difference for host utilities from qt5-tools which don't need GUI

Data from x86-windows CI, host x64-windows:

Building qt5-base[core,gui]:x64-windows...
Elapsed time to handle qt5-base:x64-windows: 6.659 min
Building qt5-activeqt[core]:x64-windows...
Elapsed time to handle qt5-activeqt:x64-windows: 39.88 s
Building qt5-imageformats[core]:x64-windows...
Elapsed time to handle qt5-imageformats:x64-windows: 6.465 s
Building qt5-svg[core]:x64-windows...
Elapsed time to handle qt5-svg:x64-windows: 17.41 s
Building qt5-declarative[core,quick]:x64-windows...
Elapsed time to handle qt5-declarative:x64-windows: 8.544 min
Building qt5-tools[core,gui]:x64-windows...
Elapsed time to handle qt5-tools:x64-windows: 2.173 min

What we want:

Building qt5-base[core]:x64-windows...
Elapsed time to handle qt5-base:x64-windows: < 1 min
Building qt5-tools[core]:x64-windows...
Elapsed time to handle qt5-tools:x64-windows: < 1 min

…encies # Conflicts: # src/vcpkg-test/dependencies.cpp # src/vcpkg/dependencies.cpp

autoantwort · 2022-10-01T16:31:32Z

@BillyONeal Any news? :)

BillyONeal · 2022-10-03T11:54:55Z

I believe @ras0219-msft still had backcompat concerns about this but not positive...

autoantwort · 2022-10-03T12:29:28Z

Would be nice if he could write them down so that they can be discussed. I would say that the benefits (greatly reduced build times) overwhelms the disadvantages (maybe backcompat(?)).
Disabling default features for the host triplet is currently impossible if you want default features for the target triplet and sometimes default = target triplet.

BillyONeal · 2022-10-04T23:59:51Z

@ras0219-msft @JavierMatosD @valeriaconde @vicroms @AugP and I discussed this today and have the following observations. (Most of these were originally pointed out by @ras0219-msft )

We do not think we can accept this in this form because it breaks our "no manifest changes means no changes" contract with customers.
One option would be to add a vcpkg-minimum-required or similar which would let a manifest opt in to this. The problem is this carries legacy forward forever.
Another option would be to add a separate host-default-features field which would let new versions of ports make the right choice. The unfortunate thing here is that it should almost always be the empty set. Or it could be a bool that just says it's the empty set.
Combine (2) and (3) (the vcpkg-minimum-required goes in the port rather than the consuming manifest)
Another option would be to deprecate most uses of default features. (Maybe the cure is worse than the disease) @vicroms specifically points out that this will likely not be popular among less experienced users of vcpkg.

@vicroms (and others) do not want (5)

@ras0219-msft @vicroms and I really like (4)

Since most of us preferred that that's as a dev team what we would ask for, but we are now concerned that we need to sell this stuff internally since it's a meaningful product feature change.

(I'm not leaving you with action items because we didn't agree on any that we could conclusively tell you would be a path forward)

autoantwort · 2022-10-16T17:34:08Z

We do not think we can accept this in this form because it breaks our "no manifest changes means no changes" contract with customers.

You could declare it as a bug fix like #292. Joke aside I guess this is only relevant if you use a builtin-baseline, otherwise there are already changes without changes to the manifest when you change for example the vcpkg-repo commit.
So what about comparing the builtin-baseline against a commit from which the new policy applies instead of the current one. This would be the same behavior as doing (3) as an bulk action in one commit but without (2) so that we don't carries legacy forward forever.

dg0yt · 2022-10-16T17:55:55Z

Or you explicitly do a major step forward, bundling several major changes. Such a defaulting to x64 on x64 windows hosts.

(I'm really tired of all these qt*webengine:host rebuilds when we only need non-gui host qt*tools.)

autoantwort · 2023-04-06T11:45:07Z

(I'm really tired of all these qtwebengine:host rebuilds when we only need non-gui host qttools.)

I mean we could only enable this change for the ci command

autoantwort force-pushed the Dont-enable-default-features-for-host-dependencies branch from aff1f89 to 1cdabae Compare August 29, 2021 14:28

BillyONeal reviewed Sep 3, 2021

View reviewed changes

include/vcpkg/base/util.h Outdated Show resolved Hide resolved

autoantwort force-pushed the Dont-enable-default-features-for-host-dependencies branch 2 times, most recently from 16d5cb9 to 706104b Compare September 3, 2021 23:00

autoantwort added 4 commits October 5, 2021 15:42

Improve documentation and names of symbols

5d072ee

Manifest mode: Don't enable default features for host dependencies

2a0ca58

Fix build

0cdd0d9

autoantwort force-pushed the Dont-enable-default-features-for-host-dependencies branch from 706104b to 0cdd0d9 Compare October 5, 2021 14:17

strega-nil-ms suggested changes Oct 18, 2021

View reviewed changes

autoantwort and others added 4 commits October 19, 2021 19:11

Apply suggestions from code review

0b80126

Co-authored-by: nicole mazzuca <[email protected]>

fix build

77d128d

Merge branch 'main' into Dont-enable-default-features-for-host-depend…

2ffc0be

…encies # Conflicts: # src/vcpkg/build.cpp # src/vcpkg/dependencies.cpp

revert small changes

6051bf5

autoantwort mentioned this pull request Dec 7, 2021

Implement depend-defaults #295

Closed

dg0yt mentioned this pull request Mar 21, 2022

[qt5-base] Move 'gui' and 'icu' out of 'core' feature microsoft/vcpkg#23668

Closed

Merge branch 'main' into Dont-enable-default-features-for-host-depend…

bdb9a13

…encies # Conflicts: # include/vcpkg/packagespec.h # src/vcpkg-test/dependencies.cpp # src/vcpkg/dependencies.cpp

autoantwort force-pushed the Dont-enable-default-features-for-host-dependencies branch from 1b59f64 to bdb9a13 Compare March 25, 2022 20:35

Interpret missing "default-features" as "default-features": true

39d34e5

daschuer mentioned this pull request May 5, 2022

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

Closed

Merge branch 'main' into Dont-enable-default-features-for-host-depend…

ebe78d1

…encies # Conflicts: # src/vcpkg-test/dependencies.cpp # src/vcpkg/dependencies.cpp

dg0yt mentioned this pull request Jan 7, 2023

'"default-features": false' doesn't work with a dependency of a dependency(transitive dependency) microsoft/vcpkg#26664

Open

dg0yt mentioned this pull request Dec 28, 2023

[qtbase] Build time when cross-compiling microsoft/vcpkg#35906

Closed

JavierMatosD added the requires:vcpkg-team-review This PR or issue requires someone from the vcpkg team to take a further look. label Oct 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dont enable default features for host dependencies #177

Dont enable default features for host dependencies #177

autoantwort commented Aug 29, 2021 •

edited

Loading

strega-nil-ms commented Oct 4, 2021

autoantwort commented Oct 4, 2021

strega-nil-ms commented Oct 6, 2021

ras0219-msft commented Oct 7, 2021

strega-nil-ms commented Oct 7, 2021

autoantwort commented Oct 8, 2021

strega-nil-ms Oct 18, 2021

autoantwort Oct 19, 2021

autoantwort Nov 18, 2021

dg0yt commented Mar 21, 2022 •

edited

Loading

autoantwort commented Mar 25, 2022 •

edited

Loading

dg0yt commented Mar 26, 2022 •

edited

Loading

autoantwort commented Mar 26, 2022

autoantwort commented Mar 27, 2022

daschuer commented May 5, 2022

dg0yt commented May 5, 2022

daschuer commented May 5, 2022

dg0yt commented Jul 22, 2022

dg0yt commented Jul 23, 2022

autoantwort commented Oct 1, 2022

BillyONeal commented Oct 3, 2022

autoantwort commented Oct 3, 2022

BillyONeal commented Oct 4, 2022

autoantwort commented Oct 16, 2022

dg0yt commented Oct 16, 2022

autoantwort commented Apr 6, 2023

	PackageNode(bool only_host_dependency) : only_host_dependency(only_host_dependency){};
	explicit PackageNode(bool only_host_dependency) : only_host_dependency(only_host_dependency){};

Dont enable default features for host dependencies #177

Are you sure you want to change the base?

Dont enable default features for host dependencies #177

Conversation

autoantwort commented Aug 29, 2021 • edited Loading

strega-nil-ms commented Oct 4, 2021

autoantwort commented Oct 4, 2021

strega-nil-ms commented Oct 6, 2021

ras0219-msft commented Oct 7, 2021

strega-nil-ms commented Oct 7, 2021

autoantwort commented Oct 8, 2021

strega-nil-ms Oct 18, 2021

Choose a reason for hiding this comment

autoantwort Oct 19, 2021

Choose a reason for hiding this comment

autoantwort Nov 18, 2021

Choose a reason for hiding this comment

dg0yt commented Mar 21, 2022 • edited Loading

autoantwort commented Mar 25, 2022 • edited Loading

dg0yt commented Mar 26, 2022 • edited Loading

autoantwort commented Mar 26, 2022

autoantwort commented Mar 27, 2022

daschuer commented May 5, 2022

dg0yt commented May 5, 2022

daschuer commented May 5, 2022

dg0yt commented Jul 22, 2022

dg0yt commented Jul 23, 2022

autoantwort commented Oct 1, 2022

BillyONeal commented Oct 3, 2022

autoantwort commented Oct 3, 2022

BillyONeal commented Oct 4, 2022

autoantwort commented Oct 16, 2022

dg0yt commented Oct 16, 2022

autoantwort commented Apr 6, 2023

autoantwort commented Aug 29, 2021 •

edited

Loading

dg0yt commented Mar 21, 2022 •

edited

Loading

autoantwort commented Mar 25, 2022 •

edited

Loading

dg0yt commented Mar 26, 2022 •

edited

Loading