Completely refactor the way orphan packages are handled (and add tests) #4969
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rjbou
added
the
PR: QUEUED
kit-ty-kate
approved these changes
Dec 16, 2021
rjbou
force-pushed
the
simpler-orphans
branch
from
ae22bc8
to
847b78f
Compare
AltGr
force-pushed
the
simpler-orphans
branch
from
847b78f
to
4ba7831
Compare
AltGr
changed the title
|
Note: this is now based on top of #4973, which includes most of the tests, and should be rebased once that is merged. |
|
2 opam-rt tests are failing:
|
AltGr
force-pushed
the
simpler-orphans
branch
from
4ba7831
to
e1eb9d4
Compare
|
Ok, now all should be ok:
|
AltGr
force-pushed
the
simpler-orphans
branch
from
e1eb9d4
to
ac42c99
Compare
|
Ok, only remaining failures should be fixed by the PR on opam-rt; apparently the trick that used the same-named branch of opam-rt for tests is no longer here, so we should probably merge #4973, then ocaml-opam/opam-rt#76, then re-run CI for this PR. |
The plan is to remove the pre-processing as much as possible and let the solver handle them as much as possible. Then some post-processing could be done to remove the irrelevant parts of the solution.
Accounting for the more restrictive handling of orphans, and working around it with `pin --current`.
- fixed removal of orphan packages (the sanitization of the request was too
strict)
- pass the list of relevant packages explicitely in the request. Previously this
was inferred from the wish_install/upgrade/remove fields, assuming no
filtering if that was empty, but:
* `opam upgrade -a foo` would wrongly restrict to `foo`
* the post-action on unpinning had to explicitely be an upgrade or remove,
while now we can just let the solver choose and still properly trim the
solution.
Can happen when changing the pinned-to version or unpinning. Also includes a small rewrite of avoid-version handling in the package universe. Now 'upgrade' requests are split for the solver: for pinned packages, it's sufficient to file an 'install' request since only one version is expected to be available.
kit-ty-kate
reviewed
Jan 4, 2022
| (** Returns [true] when the package has the [avoid-version] flag and there isn't | ||
| already a version with that flag installed (which disables the | ||
| constraint) *) | ||
| val avoid_version : 'a switch_state -> package -> bool |
There was a problem hiding this comment.
Since this PR also changes the code for the handling of the avoid-version flag, i just noticed there wasn’t any tests for this flag. Could you add some if you have some time?
There was a problem hiding this comment.
I moved some code around and replaced a Set.fold+exists by a filter for efficiency, but that should be functionnally equivalent; indeed there is no fundamental reason for this to be in this PR, I just stumbled upon it while I was writing the PR.
You're right about tests — though I'd prefer not to stall this and add them in another PR.
AltGr
added a commit
to AltGr/opam
that referenced
this pull request
Jan 14, 2022
Follows from ocaml#4969 and fixes ocaml#4727
AltGr
added a commit
to AltGr/opam
that referenced
this pull request
Jan 17, 2022
Follows from ocaml#4969 and fixes ocaml#4727
AltGr
added a commit
to AltGr/opam
that referenced
this pull request
Jan 17, 2022
Follows from ocaml#4969 and fixes ocaml#4727
kit-ty-kate
pushed a commit
to AltGr/opam
that referenced
this pull request
Jan 21, 2022
Follows from ocaml#4969 and fixes ocaml#4727
rjbou
pushed a commit
to AltGr/opam
that referenced
this pull request
Jan 21, 2022
Follows from ocaml#4969 and fixes ocaml#4727
rjbou
removed
the
PR: QUEUED
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Orphan packages are packages which are installed within a switch, but no longer available. Without special handling, they become invisible to the solver, which has quite a few drawbacks:
For these reasons, they need special care. The current approach is complex code in OpamClient that analyses them, their relationships with the user request, and lies about the universe so they can be kept when unrelated, or if they don't matter.
That code was tricky, complex, computing (reverse) dependency cones, intersections thereof and whatnot, costly — requiring a few preliminary conversions of the opam state to a solver universe, and, I found out, still incomplete and incorrect around the edges.
This PR implements a much simpler solution, that supplies the whole universe to the solver with a minimum of lies (orphans are included, but with their unavailability encoded as a fake dependency). Then, there is post-processing on the action graph after the solution is computed for removing sub-graphs that are disjoint from the requested actions, to avoid unrelated actions from being triggered.
The additional tests show a good behaviour in the considered cases (including some where the old system was incorrect).
IMPORTANT: major change compared to the previous behaviour
RELATED THINGS TO CONSIDER:
opam pin --current <pkg>that pins the package to its current state. Allows manual bypassing of an upstream removed packages, and would also offer a more consistent workaround for skipping pending reinstallsopam upgrade --fixuphas been simplified because it used to consider the specific status of orphan packages. Maybe consider re-implementing it with something likeopam upgrade --criteria=fixup --best-effort --update-invariant?