-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Accept -Q / -K in show-build-deps, sort-dependencies, xbps-cycles.py #47888
Conversation
We need to ignore |
This only affects order, in the sense that For example:
even if OTOH,in
it is irrelevant for non-testing-build in which order these two packages are built, so it should not be a problem. The only case where this would make a difference is if there is a build-check cycle. Then this will print a warning, and IMO it should be considered a bug. Example:
Note this is only a cycle because of
|
Also, note that there are several situations of non-testing-build where checkdepends will still be required (see #46207). |
Pushed a new version. Now This means As is this won't fix CI, since It's also not 100% correct since there are a number of situations where The idea is: read the pkg, determine if checks would be run for this pkg (based on This also means it's easy to use Proposal (again): consider build-check cycles with |
New version:
Now running |
…tep() This function contains the logic that determines whether the check step will be skipped for the current pkg, taking into account `make_check`. Use this new function in `install_pkg_deps()` so that it uses a more accurate condition to skip installing check dependencies. For instance, check dependencies for a pkg with `make_check=extended` will no longer be installed when using `-Q`. Similar for `make_check=ci-skip`. Replaces: void-linux#46207
Due to this change, `./xbps-src sort-dependencies` will take checkdepends into account when using -Q or -K. Before this commit, if `pkgA` checkdepends on `pkgB`, sort-dependencies could still print `pkgA` before `pkgB`. This causes CI to build `pkgB` twice: first when building `pkgA`, which forces implicit build of pkgB; second when building `pkgB` (explicit, so it will ignore the package is already built). The implementation uses `skip_check_step()` from previous commit, for consistency, so checkdepends are only taken into account if the check step would be enabled. In particular, nothing is changed unless -Q or -K flag is passed. EXAMPLE: Before: ``` $ ./xbps-src -Q sort-dependencies python3-process-tests python3-pytest-cov python3-pytest-cov python3-process-tests ``` After: ``` $ ./xbps-src -Q sort-dependencies python3-process-tests python3-pytest-cov python3-process-tests python3-pytest-cov ```
This ensures that checkdepends will be taken into account in the build order whenever test is enabled.
Please review. In this version I moved fixing other pkgs to #47910 to make this PR simpler. Summary of changes:
@sgn does this address your concerns? |
ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
This also fixes The following is ok, since neither package depends or builddepends on the other:
The following is NOT ok, since coverage checkdepends on flaky
This is NOT ok, since With this branch, the no-check version does the same as before, and the check version does the proper thing.
|
@sgn: this scratches an itch and I think it's in good shape. I use it sometimes by merging into my current branch (useful on jupyter branch which often has several updates with a number of check dependencies). By fixing sort-dependencies when used with |
As it is now, if
pkgA
checkdepends onpkgB
, sort-dependencies will still printpkgA
beforepkgB
. This causes CI to buildpkgB
twice: first buildpkgA
which forces implicit build ofpkgB
; then build ofpkgB
(explicit, so it will ignore the package is already built).A concrete example:
The example above causes
python-process-tests
to be built twice, as shown in:https://github.com/void-linux/void-packages/actions/runs/7107346278/job/19348602412?pr=47610
After this commit one can do
The CI issue is fixed by pasing -Q to sort-dependencies when we are doing a test build.
Jump to #47888 (comment) to skip comments about earlier versions of the PR.
Testing the changes