Skip to content

Fix a bunch of cases where default features were incorrectly included#2012

Open
cqundefine wants to merge 1 commit intomicrosoft:mainfrom
cqundefine:default_features_fixes
Open

Fix a bunch of cases where default features were incorrectly included#2012
cqundefine wants to merge 1 commit intomicrosoft:mainfrom
cqundefine:default_features_fixes

Conversation

@cqundefine
Copy link
Copy Markdown
Contributor

In some cases related to host dependencies, default features were being included even those they were disabled, found this when cross compiling qtbase with default features disabled.

Copilot AI review requested due to automatic review settings May 8, 2026 11:46
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes cases where default features were being incorrectly included, particularly around host dependencies (notably when cross-compiling with default features disabled). It updates dependency planning behavior and adds regression tests to ensure host/target default-feature metadata is handled as intended.

Changes:

  • Prevents adding a default-feature reinstall requirement for non-user-requested clusters when the package isn’t installed.
  • Adjusts versioned dependency graph handling so host dependencies don’t get implicit default features; default features for versioned plans are only emitted when enabled for the node.
  • Adds new unit tests covering host dependency default-feature suppression in both classic and versioned planning.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/vcpkg/dependencies.cpp Fixes default-feature inclusion logic in both classic cluster reinstall handling and versioned dependency planning (esp. host deps).
src/vcpkg-test/plan.cpp Adds tests asserting host-dependency default-feature metadata is suppressed appropriately in classic plans.
src/vcpkg-test/dependencies.cpp Adds a versioned-plan test ensuring host dependencies respect explicit no-defaults.

Comment thread src/vcpkg-test/plan.cpp Outdated
@cqundefine cqundefine force-pushed the default_features_fixes branch from f3f6d11 to f272979 Compare May 8, 2026 11:54
Copilot AI review requested due to automatic review settings May 8, 2026 12:04
@cqundefine cqundefine force-pushed the default_features_fixes branch from f272979 to c9b5533 Compare May 8, 2026 12:04
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comment thread src/vcpkg/dependencies.cpp
@cqundefine cqundefine force-pushed the default_features_fixes branch from c9b5533 to a07cfe0 Compare May 8, 2026 13:51
@dg0yt
Copy link
Copy Markdown
Contributor

dg0yt commented May 8, 2026

I would like to see the exact install command and problem before the change.

@dg0yt
Copy link
Copy Markdown
Contributor

dg0yt commented May 8, 2026

(For such cases, I prefer to push tests first, to demonstrate the error in CI.)

@cqundefine
Copy link
Copy Markdown
Contributor Author

cqundefine commented May 8, 2026

I would like to see the exact install command and problem before the change.

Right, so with this simple test package (just added it to the vcpkg repository for testing):

{
  "name": "testport",
  "version": "0.0.0",
  "description": "test",
  "dependencies": [
    {
      "name": "testport",
      "host": true,
      "default-features": false
    }
  ],
  "default-features": [
    "a"
  ],
  "features": {
    "a": {
      "description": "a"
    }
  }
}

Calling ./vcpkg install testport[core] --triplet=arm64-linux it incorrectly included the a feature for the host build (in my case x64-linux:

Computing installation plan...
The following packages will be built and installed:
  * testport[a,core]:[email protected]
    testport:[email protected]

Running the same command with a binary built with those fixes it correctly skips adding the a feature:

Computing installation plan...
The following packages will be built and installed:
  * testport:[email protected]
    testport:[email protected]

(For such cases, I prefer to push tests first, to demonstrate the error in CI.)

Do you think that is something I should do now, like revert the fix, let the CI run and then readd the fix?

@Osyotr
Copy link
Copy Markdown
Contributor

Osyotr commented May 8, 2026

Calling ./vcpkg install testport[core] --triplet=arm64-linux it incorrectly included the a feature for the host build (in my case x64-linux

This is not nice but expected, should've been ./vcpkg install testport[core] --triplet=arm64-linux testport[core]:x64-linux.
#177

@dg0yt
Copy link
Copy Markdown
Contributor

dg0yt commented May 8, 2026

Right, already works as intended by Microsoft. Cf. microsoft/vcpkg#51325 for a recent discussion.
TLDNR: If you don't want default-features for a particular package:triplet, be explicit in your install command.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants