Fix assertion failure when a package constrains itself - #230
Conversation
|
This is admittedly a strange thing to do, but here was the rationale: https://gitlab.com/redhat/centos-stream/rpms/centos-stream-release/-/commit/ce231c10b48cb4ddde860bfc9d6f79c76ba92e0a I believe the semantics here are basically "if this package is installed, you must not install another provider, and if another provider is installed, you cannot install this package". |
|
Let me know the ideal way to provide tests, unless you want to do it |
baszalmstra
left a comment
baszalmstra
left a comment
There was a problem hiding this comment.
The constraint introduces a tautology. I think a more technically correct approach would be to (not crash and) block the package from being installable using an assertion. This would be the same as when this is done through a transitive dependency (e.g. A depending on B, which constrains A). I don't think skipping the clause is the right approach. But maybe I misunderstand the use-case?
|
So, it's a package that is installed and can be updated (on my CentOS Stream dev container it's at release 21)
I haven't tracked down exactly what libsolv is doing to handle this scenario, but libsolv / DNF must be handling it somehow. Probably it's doing something similar. |
ab34b25 to
c16fc12
Compare
|
Ah, so here's some libsolv documentation: So, libsolv does indeed do basically the same thing as this patch, but it's behind a solver configuration flag, because the desired behavior varies by ecosystem. Would you want to do something similar (or is there already a mechanism for solver config options)? Unrelated: looks like the multiversion code is right there as well. And that might need a similar solution as well |
|
Is that then something we should not just solve in the dependency provider itself? Simply dont return self-referential constraints? We should still solve the crash of course, but I think we should backtrack in that case. |
|
How would that work exactly? The "conflicts" is on any provider of |
c16fc12 to
6b790f3
Compare
|
Conceptually the DependencyProvider provides the ecosystem implementation. Whether to allow self referential constraints or not seems like an ecosystem specific implementation, so I think this should be part of that. We can either model this as a seperate function to enable disable globally, we can allow this per nameid when returning candidates, or even per solvable when returning dependency information, or even per versionset with a new function. WDYT? |
You mean a trait method on If I can suggest a tweak on that, going w/ the And the (if that was the case I would probably drop that 2nd commit w/ the activity params change, it wouldn't make much sense anymore).
That could probably work. Do you prefer that over option 1 (+tweaks)? I could spit out both for comparison purposes.
I think that's probably overkill. At least at the moment I have no example of a versioned self-conflicts, only this example of a conflict over a global virtual provide. |
95a411e to
829c580
Compare
|
@baszalmstra Ok, this passes locally (the C++ side is broken of course). Once you give me a final design direction I'll clean it up. The multi-version change ended up being a very similar implementation so I include that commit as well, but I can split that off into a different PR if you want. |
|
I'm still a bit on the fence; please help me understand this better. I could not understand how self-referential constraints make any sense, because the constraint literally says that it cannot install itself; a tautology. But in combination with multi-version, I can see how it makes sense. But it's basically saying: you can have multiple versions of package A, but if you select version A@42 (the version that adds the self-referential constraints), that's the only possible version. Right? I am wondering if there is not a more elegant way of modeling that specifically. Resolvo takes a lot of care to try to minimize the number of clauses that it has to maintain. I wonder if it would take fewer clauses if we model this particular behavior specifically rather than just adding general constraint clauses. |
|
The two aren't directly related, I'm only thinking about both because I would eventually want to have both and because the approach you suggested was essentially the same as what I was working on for multi-version, not because there's a direct relationship. I agree with you that from a purely logical perspective it doesn't make a ton of sense. What I've been told (from Slack) is:
and then another response:
Specifically this package seems to include things like distro public GPG keys for package signing, secure boot certificates, and the list of distro repo configs. In other words it's a package that you don't want to be able to remove or replace easily lest the system become unusable, & kind of defines what the distro is in some sense (the root of trust and such). Fedora also has a similar pattern: https://src.fedoraproject.org/rpms/fedora-release/blob/f44/f/fedora-release.spec#_156 Otherwise all I know that it seems to be something DNF / libsolv explicitly supports. And really it seems to just be standard behavior across an entire solve and necessarily something that is per-package configurable. |
829c580 to
bab305a
Compare
|
If you want to / have time to play with it yourself, you can. Code is here: https://github.com/prefix-dev/resolvo-rpm/ Here's the repo metadata I'm using: https://drive.google.com/file/d/12e_lwXdLfykQn0r3fHL7qlX886wJXnF4/view?usp=sharing Extract the files and dump them in Example: I need to clean up this branch a bit though FYI. That example no longer reproduces the original issue exactly, it hits a related one first. |
bab305a to
2d1099c
Compare
fac083d to
bbcfd13
Compare
|
Changed: Added a This allows |
bbcfd13 to
fc53d87
Compare
| /// (i.e. it conflicts with itself) is marked uninstallable. When `false`, | ||
| /// self-conflicts are silently ignored. | ||
| pub forbid_self_conflicts: bool, | ||
| } |
There was a problem hiding this comment.
@baszalmstra Do you think this is reasonable?
Libsolv offers a couple such options (e.g. 1, 2), but I know your intention is to stay as generic as possible.
I also have a Candidates based implementation, but I don't think it actually ends up simpler and is probably less performant, since it is treated as a a global option so if you were to emulate it with a map per-nameid or per-solvable then it would require a lot of hashmap insertions and subsequent lookups where the answer will always be the same. The additional flexibility isn't needed, at least not by RPM.
| /// Creates a single threaded block solver, using the provided | ||
| /// [`DependencyProvider`]. | ||
| pub fn new(provider: D) -> Self { | ||
| let config = provider.solver_config(); |
There was a problem hiding this comment.
Uses the config provided by DependencyProvider by default, which implementors can optionally define (has default impl) but can be overridden manually on the solver (makes testing easier)
|
Im on a holiday this week, Ill have all the time to think this through properly. Im hesitant to merge this as is because it feels like genericly allowing self references just feels like a hack. It feels to me that the versionset is just wrong, I think it should exclude the solvable itself. But I can also see that this is harder to express. That said, does this still work now that #246 is merged? It breaks the assumption that a constraint with a certain id always matches the same solvables. |
|
Maybe we can combine this with your previous PR? If we detect a self reference but we allow multiple versions for the package that we then disable all other solvables? Thats what we are actually trying to express. |
|
I'm open to whatever you think is best, so long as it works.
It seems to still work. ( with With |
|
@dralley Back from holiday, not much closer to the perfect answer, but eager to get this in. What do you think of my last thought?
|
|
I'll give a proper response in a few days, I've also been taking a holiday the past week and returning home this weekend. |
|
Sorry for the lack of response, I got sick immediately after my vacation and have also had a lot of work to catch up on. I haven't forgotten. |
|
Still haven't forgotten. I'm getting caught up but I want to be able to focus on it (to the extent I can focus on a side project) for a couple of days. |
No rush! Take your time! |
In such a case, the constrains clause would try to forbid the package from coexisting with itself. This caused WatchedLiterals::constrains to panic with "both literals cannot be false" because both watched literals referred to the same variable. There exists a special case in certain packaging ecosystems such as RPM, where a package can both provide and conflict with the same capability (example: in CentOS Stream, the "centos-stream-release" provides "system-release" and also conflicts with "system-release"). This is explicitly permitted and supported by libsolv, which provides the configuration flag "forbidselfconflicts" which can be disabled. Provide an option to skip self-referential constrains entries, so that implementations can configure the resolver such that a package that is already in the solution cannot constrain itself out of it. Assisted-By: Claude Opus 4.6
fc53d87 to
de25564
Compare
…iders The shared aux-variable constrains encoding (used when a version set has >= CONSTRAINS_AUX_ENCODING_THRESHOLD forbidden candidates) allocates one aux variable per version set, meaning "some forbidden provider of this constraint is installed", shared across all constraining parents. The previous self-conflict handling skipped the self term while *building* that shared aux variable. Because the aux var is memoized per version set and built by whichever parent triggers it first, this left the aux definition incomplete for every other parent: the skipped candidate was no longer forbidden for anyone else, and every other self-conflicting provider that reused the aux became spuriously uninstallable. In the real CentOS case (five centos-stream-release versions each providing and conflicting with system-release) this produced a spurious UNSAT whenever the solver had to fall back off the aux-building provider. Build the shared aux variable over all candidates so its meaning is parent-independent, and handle a self-conflicting parent separately: since it is itself part of the aux definition (parent -> aux), the shared (parent -> not aux) clause would make it uninstallable, so encode it pairwise (forbidding the other providers but not itself), or, when forbid_self_conflicts is set, mark it uninstallable directly. Add regression tests covering multiple self-conflicting providers on both the shared-aux and pairwise paths, including the fallback case that previously went unsat. Assisted-By: Claude Opus 4.8 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@baszalmstra Ok, so I have a bit more free time to play with this now. I am convinced that the previous implementation is indeed buggy with the new encodings, and the test cases demonstrate that. I can wrap my head around the basic problem, but I am not sure if this latest patch (from claude) is the right direction to go. I'm marking it draft because this is still very speculative and also needs to be cleaned up a bit. But let me know if it's directionallly sane. As for
I'm not sure you can describe "all providers except me" as a version set, because we're talking virtual provides here, it's a capability which more than one package (and different versions of each of those packages) can provide. What we basically want is for solvables representing packages like this (with self-conflicts) to say "if I am the solution, no other provider can be", independent of multiversion (so only one such package can be installed even if multiversion is enabled). Because you could a package If that makes sense. I could well be missing something about version sets though. |
|
Independently I'm hitting this Because it seems to be assuming that every candidate of a version set is sharing one package name, which is false in this case. So maybe I am missing something, because it already creates a version set across different package names if that assert is being triggered. Anyway that will need its own fix. In any case, assuming a version set does cross package names via virtual provides, I think it still doesn't work because you can't express "all versions except me" as a version set without having the full list of candidates and knowing which "me" is. I think it can, technically, be made to work on the provider side, but it's very messy and requires duplicating some core logic from the solver. |
2 participants
When a package both provides and conflicts with the same capability (example: in CentOS Stream, "centos-stream-release" provides "system-release" and also conflicts with "system-release"), the constrains clause would try to forbid the package from coexisting with itself. This caused WatchedLiterals::constrains to panic with "both literals cannot be false" because both watched literals referred to the same variable.
Skip self-referential constrains entries: a package that is already in the solution obviously cannot constrain itself out of it.
Assisted-By: Claude Opus 4.6