Bug 1947958

Well, apologies for the mess. This was changed here: https://github.com/rpm-software-management/dnf/commit/f54a1001eb4eefda952de522902c2dd18527023f

In an attempt to bring some sanity to the code, but unfortunately installation of both groups and environments goes through this wrapper which just eats the exception (and prints the warning): https://github.com/rpm-software-management/dnf/blob/395541fbf8f87f81cdca7567f22be1182e55bea7/dnf/comps.py#L94

It's a bad interface which we maintained for compatibility. To fix this we either need to break the interface or add a new one (or revert to the previous inconsistent behavior, which seems like the worst option).

(FTR I haven't yet managed to find where the MarkingError -> ValueError change happened)

Jaroslav, can you propose a solution? IIRC I wanted to not catch the exception, if we did that, at least now lorax would have something to catch, this way it's still not working correctly.

PR: https://github.com/rpm-software-management/dnf/pull/1784

With the above, dnf will start throwing CompsError when a group or environment is not found for the install methods.

Brian, we've discussed this in the team and concluded raising a CompsError (admittedly yet another exception class than what was thrown before) is the best way to move forward here. Not raising any exception is clearly a bug and ValueError is also not the right exception to raise.

Is this fine with you, assuming you'll have to add another except block for catching this one?

Right now this bug is slated for 8.5, but that now depends mainly on the API users being able to adapt to this API change for 8.5, so it may well need to be pushed to 8.6.

Again, apologies for the inconvenience...

(In reply to Lukáš Hrázký from comment #3)
> Brian, we've discussed this in the team and concluded raising a CompsError
> (admittedly yet another exception class than what was thrown before) is the
> best way to move forward here. Not raising any exception is clearly a bug
> and ValueError is also not the right exception to raise.
> 
> Is this fine with you, assuming you'll have to add another except block for
> catching this one?
> 
> Right now this bug is slated for 8.5, but that now depends mainly on the API
> users being able to adapt to this API change for 8.5, so it may well need to
> be pushed to 8.6.
> 
> Again, apologies for the inconvenience...

That sounds fine, I can catch multiple exceptions to cover all the cases if I need to :)

Ok, thanks Brian, this bug has been moved to 8.6. Can you add the catch for CompsError into lorax for 8.6, so that we can merge this upstream and schedule it for 8.6 release? Thanks!

(In reply to Lukáš Hrázký from comment #5)
> Ok, thanks Brian, this bug has been moved to 8.6. Can you add the catch for
> CompsError into lorax for 8.6, so that we can merge this upstream and
> schedule it for 8.6 release? Thanks!

Sure, once this has acks I can push it as Related to this bug.

Brian, the acks are in, can you make the change (unless it's been done already)? We should move forward with this in a timely manner. Thanks!

I have a PR here - https://github.com/weldr/lorax/pull/1179

Note that this bug needs to have ITR set before it will turn on release+ and allow commits.

Acceptation Criteria:
The Base.group_install() and Base.environment_install() methods raise an exception of type dnf.exceptions.CompsError in case the group or environment doesn't exist.

Ok, the release+ is there, Brian!

We are also planning to add a conflict on lorax versions lower than the one with this fix to dnf so that we force the upgrade and don't allow a broken combination of versions. Can you let me know the version in which the fix will land? Thank you.

(In reply to Lukáš Hrázký from comment #11)
> Ok, the release+ is there, Brian!
> 
> We are also planning to add a conflict on lorax versions lower than the one
> with this fix to dnf so that we force the upgrade and don't allow a broken
> combination of versions. Can you let me know the version in which the fix
> will land? Thank you.

That doesn't seem like the right thing to do, dnf should not be obsoleting any version of a package that uses it.

This new version of lorax will handle all the variations of this error, so I don't think there is anything for dnf to do.
Also, this change is in a disused corner of lorax, the chances of someone hitting it are very low at this point since they should all be using osbuild-composer anyway.

(In reply to Brian Lane from comment #12)
> That doesn't seem like the right thing to do, dnf should not be obsoleting
> any version of a package that uses it.
> 
> This new version of lorax will handle all the variations of this error, so I
> don't think there is anything for dnf to do.

No, not obsoleting, conflicting:

Assume dnf-1 didn't raise CompsError and dnf-2 does.
lorax-1 doesn't catch CompsError (it tracebacks) and lorax-2 does handle CompsError gracefully.

We would add in dnf.spec:
Conflicts: lorax < 2

So that you can't upgrade dnf to version 2 and keep lorax at 1, which would mean lorax now can crash on a non-existent group.


Tbf. I'm not a personal fan of this just because it adds a relation between dnf and lorax in the opposite direction (lorax depends on dnf, now dnf references lorax in its .spec and from dnf's standpoint it's a bit arbitrary), but it is the proper way to prevent the described breakage in the rpm packaging system it would seem.

> Also, this change is in a disused corner of lorax, the chances of someone
> hitting it are very low at this point since they should all be using
> osbuild-composer anyway.

That might be an argument to not add the conflict just because the case is so niche now.

(In reply to Lukáš Hrázký from comment #13)
> (In reply to Brian Lane from comment #12)
> > That doesn't seem like the right thing to do, dnf should not be obsoleting
> > any version of a package that uses it.
> > 
> > This new version of lorax will handle all the variations of this error, so I
> > don't think there is anything for dnf to do.
> 
> No, not obsoleting, conflicting:
> 
> Assume dnf-1 didn't raise CompsError and dnf-2 does.
> lorax-1 doesn't catch CompsError (it tracebacks) and lorax-2 does handle
> CompsError gracefully.
> 
> We would add in dnf.spec:
> Conflicts: lorax < 2
> 
> So that you can't upgrade dnf to version 2 and keep lorax at 1, which would
> mean lorax now can crash on a non-existent group.
> 
> 
> Tbf. I'm not a personal fan of this just because it adds a relation between
> dnf and lorax in the opposite direction (lorax depends on dnf, now dnf
> references lorax in its .spec and from dnf's standpoint it's a bit
> arbitrary), but it is the proper way to prevent the described breakage in
> the rpm packaging system it would seem.

Yeah, that just seems wrong to me. If anything lorax could add a requirement on the newer version of dnf after it is built.

> 
> > Also, this change is in a disused corner of lorax, the chances of someone
> > hitting it are very low at this point since they should all be using
> > osbuild-composer anyway.
> 
> That might be an argument to not add the conflict just because the case is
> so niche now.

I can agree to that :) It really is code that is only hit during testing, so I really don't want to see anything referring to lorax added to dnf.

Ok, we will not create a conflict.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (dnf bug fix and enhancement update), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2022:2051