-
Notifications
You must be signed in to change notification settings - Fork 140
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
MergedTransaction: Calculate RPM difference between two same versions as no-op #1644
MergedTransaction: Calculate RPM difference between two same versions as no-op #1644
Conversation
I am planning to include CI tests this week. EDIT: Done. |
The patch is OK, but it fixes a peace in an incorrect logic. The current logic uses pairs model. One object has one of two operation - install, remove or both (upgrade), but this model fail in several situations - standard - installonly package, more obsoletes; and non standard - duplicates from broken transaction. Rollback does not work correctly in this example Reproducer is more complicated to handle minimum dependencies:
Reproducer (install 2 kernels and rollback the transaction)
It removes only one version of kernel-core but it should remove both version. What I want to say - the logic of transaction merge is incorrect and fixing particular use case might be not the best option. I recommend to create a new logic in DNF5 and then back-port it to DNF if it is possible. |
It would be definitely better to backport the fresh new logic from DNF5 once it's ready and tested. However, I expect that this could involve an extensive amount of work, and it won't be delivered soon. If I understand correctly, the reproducer isn't functioning even with the existing code. Therefore, the fix here should address several use cases while leaving other non-working ones as they are, without breaking anything that was functional until now. I don't have a strong opinion on this, but it appears to be a step forward. We can create a follow-up issue to address the issue properly, as you've suggested. What about that? |
It is always difficult what make sense. I think that creating CI tests make sense for all scenario (upgrade + downgrade, install-remove, operation with installonly packages (more items with same name in transaction, several transaction that install + remove over limit kernel)) because DNF and DNF5 will benefit from them. When The patch improves behavior, but the code will be replace or significantly modified when problem with installonly packages is resolved because it will require to exchange Personally I don't like partial fixes, because they are more expensive then fixing all related problems at once. @jan-kolarik But the decision is on you. |
… as no-op If a package of a particular version is installed and would still be installed after a list of transactions, it's more user friendly to treat the whole situation as "do nothing".
bbf869d
to
bc829d4
Compare
Thank you for your feedback! Based on the agreement from today's meeting, we can proceed with merging this. A follow-up issue to cover the functionality that is not working correctly now is created. |
LGTM |
If a package of a particular version is installed and would still be installed after a list of transactions, it's more user friendly to treat the whole situation as "do nothing".
Tests: rpm-software-management/ci-dnf-stack#1461.
Fixes: https://issues.redhat.com/browse/RHEL-17494.
Closes: rpm-software-management/dnf#2031.