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] PR and CI system should be better about testing features #13119

Open
BillyONeal opened this issue Aug 25, 2020 · 11 comments · May be fixed by microsoft/vcpkg-tool#802
Open

[vcpkg] PR and CI system should be better about testing features #13119

BillyONeal opened this issue Aug 25, 2020 · 11 comments · May be fixed by microsoft/vcpkg-tool#802
Assignees
Labels
category:infrastructure Pertaining to the CI/Testing infrastrucutre

Comments

@BillyONeal
Copy link
Member

Currently our CI system tests the default features of each port. That is good for finding most failures that are likely to happen, but there is still a fairly high likelihood of bugs slipping into features of each port that our CI doesn't find, because we only test defaults.

We wouldn't want to change to trying to test all features, first because some features are platforms specific so we'd get false failures, and second because that just changes the set of bugs we find: today we miss optional-features-being-broken, that would make us miss default-features-being broken.

In general, we can't test every combination of features, but we might want to test more, or at least more combinations for ports in which we have seen such features create customer failures in the past.

@BillyONeal BillyONeal added the category:infrastructure Pertaining to the CI/Testing infrastrucutre label Aug 25, 2020
@JackBoosY JackBoosY self-assigned this Aug 25, 2020
@BillyONeal BillyONeal changed the title [vcpkg] We should be better about testing features [vcpkg] PR and CI system should be better about testing features Aug 25, 2020
@JackBoosY
Copy link
Contributor

One of the solutions: #12160.

@BillyONeal
Copy link
Member Author

One of the solutions: #12160.

I don't think that's really a solution: that changes the set of features we install to be "all of them" which means we catch a bug where a feature doesn't build, but we now we miss the bug in the other direction where a build succeeds only when a non-default feature is selected. Given that we expect most customers to be using the default features rather than all features I don't think that's an improvement. However, the infrastructure there to add Supports: on an individual feature basis seems like it would be part of any solution we would want.

@JackBoosY
Copy link
Contributor

@BillyONeal In any case, we need to ensure the quality of existing ports. I hope to solve through the following points:

  • Ensure that the generated library can be used normally (test with some example code)
  • Ensure that the built-in test feature passes
  • Ensure that the features can be built normally
  • Ensure that the generated cmake files can be used normally
  • Ensure that the build environment is consistent to avoid build failures caused by the environment

@BillyONeal
Copy link
Member Author

@dan-shaw @vicroms and I discussed this in person today and agree that it seems we should update the CI to do 2 passes:

  • 1 pass that installs all the default features (identical to today)
  • 1 pass that tries to install all the features (which @JackBoosY @LilyWangL @PhoebeHui and co. have been doing manually; the CI system should do that)

We can probably use @JackBoosY 's PR above as a starting point.

@BillyONeal BillyONeal self-assigned this Aug 27, 2020
@PhoebeHui PhoebeHui self-assigned this Aug 28, 2020
@PhoebeHui
Copy link
Contributor

PhoebeHui commented Aug 28, 2020

Many issues reported for features installing problems, we have encountered the following cases:

  1. Features is broken when they has been added first time.
  2. Features doesn't work on specific platform.
  3. Regressions: features is broken when upgrade the port, or other fixes cause the feature failed.

Related to #12377 #12568 #12896 #13014

@BillyONeal
Copy link
Member Author

@PhoebeHui Yes, I should clarify above. The idea is to automate the same testing you folks have been so helpfully providing, but without giving up the testing the existing CI system provides. We don't want to test just the default features, or just all features, we want to test both default and all features.

In general we can't test every possible combination because there are 2^N of those possible and we have ports like ffmpeg with more than 30 features, and it's possible that some combinations will be missed. The trick is to choose a set of combinations we can actually test, but with are likely to find the majority of bugs users encounter here.

@horenmar
Copy link
Contributor

horenmar commented Oct 5, 2021

Can I also suggest the "core"/no feature set to be tested? I've ran into trouble when trying to use the minimal feature ports, and it seems to be a good chance to discover incorrect assumptions about minimal dependencies and such.

@BillyONeal
Copy link
Member Author

Can I also suggest the "core"/no feature set to be tested? I've ran into trouble when trying to use the minimal feature ports, and it seems to be a good chance to discover incorrect assumptions about minimal dependencies and such.

We could do something like that but it isn't a design rule in general that a port needs to be useful with only core selected.

@autoantwort
Copy link
Contributor

I think the simplest solution is the following:
In a PR, get all changed ports that have features. For every port:

  • generate a vcpkg.json with "default-features": false, run vcpkg install
  • generate a vcpkg.json with all features selected, run vcpkg install

@autoantwort
Copy link
Contributor

autoantwort commented Jun 19, 2022

In a PR, get all changed ports

Or should we check all ports via the parent-hashes?

@dg0yt
Copy link
Contributor

dg0yt commented Jun 19, 2022

I think it is not enough to build. You must test actual usage, to see if dependencies are forwarded properly.

@Cheney-W Cheney-W assigned Cheney-W and unassigned JackBoosY Dec 6, 2022
@autoantwort autoantwort linked a pull request Feb 26, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:infrastructure Pertaining to the CI/Testing infrastrucutre
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants