goal: Fix group packages removal (RhBug:2173927) #329
Conversation
|
I've just realized that there still are cases where In case of |
c427b24 to
37b46ec
Compare
libdnf/base/goal.cpp
Outdated
| } | ||
| rpm_goal.add_remove(packages_to_remove_ids, cfg_main.clean_requirements_on_remove().get_value()); |
There was a problem hiding this comment.
I think this need to be updated: clean_requirements_on_remove -> get_clean_requirements_on_remove_option since #327 was merged. Sorry for the delay.
There was a problem hiding this comment.
Hmm this is interesting, I didn't think of it before. Because all tests are passing, everything is green, but I suppose after merging it as it is, it wouldn't be compilable. Can we improve that with like running the CI stack in the environment where the commit is already rebased and merged?
There was a problem hiding this comment.
I think that already happens, the problem is it ran before merging the incompatible changes. I believe if we re-run the CI now it would fail to compile.
There was a problem hiding this comment.
I have tried to re-run it just now to make sure.
There was a problem hiding this comment.
Allright, then just maybe automatic re-running workflows for pending pull requests? But that's probably expensive... At least there could be some warning that the checks are not valid anymore, it is confusing and probably dangerous 🙂 I created a follow-up issue to not forget about that.
libdnf/base/goal.cpp
Outdated
| @@ -1429,9 +1429,15 @@ void Goal::Impl::add_group_install_to_goal( | |||
| } | |||
|
|
|||
| void Goal::Impl::add_group_remove_to_goal( | |||
| base::Transaction & transaction, | |||
| [[maybe_unused]] base::Transaction & transaction, | |||
There was a problem hiding this comment.
Can I ask why are we keeping the unused parameter?
There was a problem hiding this comment.
Sure, I forgot to remove it :)
libdnf/base/goal.cpp
Outdated
| rpm_goal.add_reason_change(pkg, transaction::TransactionItemReason::DEPENDENCY, std::nullopt); | ||
| continue; | ||
| } | ||
| remove_candidates.emplace_back(std::move(pkg)); |
There was a problem hiding this comment.
I am curious are you using std::vector<rpm::Package> remove_candidates instead of PackageSet for efficiency gains? If yes I would like to note that I think you cannot std::move the pkg because it is const, AFAIK it is being copied.
If PackageSet was used it could then later also be added to the goal_tmp without the loop.
There was a problem hiding this comment.
Yeah, right. I've changed it to PackageSet.
This commit fixes a problem where group packages were not being removed
along with their respective groups. The issue was caused by the
algorithm for determining whether another installed package requires the
group package. Previously, it took into account all packages that were
installed, regardless of whether they were part of any group being
removed.
To address the issue, the algorithm now only takes into account packages
that are not part of any group currently being removed. For example, if
there are two groups installed on the system:
Grp1 with packages
Pkg1 which requires Dep1
Grp2 with packages
Dep1
The new algorithm will correctly remove both Pkg1 and Dep1 packages when
both Grp1 and Grp2 are being removed.
This situation can even occur with only one group being removed - in
case that the group package requires one of its own provides. These self
requirements are quite common in rpmdb and they also caused group
package not to be removed. The patch fixes also this issue.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2173927
37b46ec to
c46ccc4
Compare
This commit fixes a problem where group packages were not being removed along with their respective groups. The issue was caused by the algorithm for determining whether another installed package requires the group package. Previously, it took into account all packages that were installed, regardless of whether they were part of any group being removed.
To address the issue, the algorithm now only takes into account packages that are not part of any group currently being removed. For example, if there are two groups installed on the system:
The new algorithm will correctly remove both Pkg1 and Dep1 packages when both Grp1 and Grp2 are being removed.
This situation can even occur with only one group being removed - in case that the group package requires one of its own provides. These self requirements are quite common in rpmdb and they also caused group package not to be removed. The patch fixes also this issue.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2173927
Test: rpm-software-management/ci-dnf-stack#1230