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

Test features in ci #802

Open
wants to merge 105 commits into
base: main
Choose a base branch
from

Conversation

autoantwort
Copy link
Contributor

@autoantwort autoantwort commented Nov 9, 2022

Introduces a new command x-test-features. It can be passed a list of port names or --all to specify for which ports the tests should be run.
The following tests are performed:

  • The core feature gets installed with default featured disabled (or --no-feature-core-test)
  • Every feature gets installed with default featured disabled (or --no-features-separated-tests)
  • All features combined gets installed (or --no-features-combined-test)

Feature which are unsupported or marked as cascade for the target triplet are skipped.
For every test only the necessary dependencies are installed (like vcpkg install in manifest mode).

The idea is to run the feature test for all ports that are changed by a PR.

Ports that cascade must be explicitly marked as cascade.

vcpkg format-feature-baseline scripts/ci.feature.baseline.txt is a command to format the feature baseline to enforce that all entries in a "block" are sorted alphabetically.

Docs: microsoft/vcpkg-docs#131

Fixes microsoft/vcpkg#13119

#908 reduce the needed ci time, this PR increases it again. So the actual change is not likely to be very large.

src/vcpkg/commands.ci.cpp Outdated Show resolved Hide resolved
# Conflicts:
#	include/vcpkg/base/strings.h
# Conflicts:
#	include/vcpkg/base/message-data.inc.h
#	locales/messages.json
#	src/vcpkg/commands.ci.cpp
#	src/vcpkg/commands.cpp
#	src/vcpkg/commands.help.cpp
@vicroms
Copy link
Member

vicroms commented Sep 12, 2023

@AugP @BillyONeal @dan-shaw @ras0219-msft

Hi @autoantwort

We discussed this command and have the following asks:

  • We want this feature
  • Make it an experimental command: x-test-features
  • Option names in vcpkg use the --no-<toggle> pattern instead of --dont-<toggle>

We haven't made a review of the implementation yet, but will probably have some more comments later.

@dg0yt
Copy link
Contributor

dg0yt commented Sep 17, 2023

The commands removes all subdirs in buildtrees/<port>/src. Can this be avoided, or limited to .clean dirs? I would like to test ports without loosing temporary git repos, or editable dirs.

@dg0yt
Copy link
Contributor

dg0yt commented Sep 17, 2023

BTW could it test an unregistered modification, maybe via overlay ports?

@autoantwort
Copy link
Contributor Author

So you want a flag like --editable?

@dg0yt
Copy link
Contributor

dg0yt commented Sep 17, 2023

I tested this PR while working locally on a vcpkg PR (zeroc-ice). It is quite helpful but at the moment,

  • I loose my git dirs for editing patches, without warning.
  • I hardly notice that it doesn't test the current state of my ports dir.

So you want a flag like --editable?

My comments were independent and shouldn't be mixed.
My first comment was about not touching unrelated source dirs in buildtrees (first problem).
My second comment was about the state of the ports dir (second problem).

With vcpkg install <port>; --editable has a different meaning: It is about not cleaning the actual source dir in buildtrees. This might be another option, but seems less important to me. I can transfer changes from editable state to proper patches before running test-features. And it is fragile: features may cause source dir modifications.

So I don't ask for a flag but for a general modification.

@autoantwort
Copy link
Contributor Author

I hardly notice that it doesn't test the current state of my ports dir.

It does that.

I loose my git dirs for editing patches, without warning.

The behavior is the same as for x-set-installed, so --clean-buildtrees-after-build is on by default. I can change that. But I would personally prefer an inverted switch like --keep-buildtrees-after-build.

@dg0yt
Copy link
Contributor

dg0yt commented Sep 17, 2023

It does that.

Indeed, you are right.

I stumbled over another behaviour now: Post-build check errors of dependencies cause cascades which prevent the test of the port of interest. Understandable from a CI point of view, but less convenient for interactive testing. (In my case: readline-unix creates empty bin directories for x64-linux-dynamic. Good to have this covered by post-build checks, but I don't want to be stopped by this....)

@autoantwort
Copy link
Contributor Author

Hm should I add a flag like --ignore-port-checks? As a workaround you could install the dependency outside of this command. Then it gets restored from the binary cache the next time, if you don't want to fix the check now :)

# Conflicts:
#	include/vcpkg/base/message-data.inc.h
#	locales/messages.json
# Conflicts:
#	include/vcpkg/base/message-data.inc.h
#	include/vcpkg/cmakevars.h
#	src/vcpkg/cmakevars.cpp
#	src/vcpkg/commands.build.cpp
#	src/vcpkg/packagespec.cpp
# Conflicts:
#	include/vcpkg/base/message-data.inc.h
#	src/vcpkg/commands.ci.cpp
@dg0yt
Copy link
Contributor

dg0yt commented May 2, 2024

We haven't made a review of the implementation yet, but will probably have some more comments later.

@vicroms This PR is waiting for MS.

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.

[vcpkg] PR and CI system should be better about testing features
4 participants