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. |
dralley
force-pushed
the
constrains-itself
branch
2 times, most recently
from
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 |
dralley
force-pushed
the
constrains-itself
branch
from
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. |
dralley
force-pushed
the
constrains-itself
branch
2 times, most recently
from
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. |
dralley
force-pushed
the
constrains-itself
branch
from
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. |
dralley
force-pushed
the
constrains-itself
branch
from
bab305a to
2d1099c
Compare
dralley
force-pushed
the
constrains-itself
branch
3 times, most recently
from
fac083d to
bbcfd13
Compare
|
Changed: Added a This allows |
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
dralley
force-pushed
the
constrains-itself
branch
from
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 |
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