goal: Better handling of obsoleted pkg installation #354

m-blaha · 2023-03-15T09:46:48Z

In situation where a package exists in multiple versions and some older version is being obsoleted, any of obsoleters was considered a valid solution.

The result could be really misleading. For example let's have this package set:

systemd-udev-1.0

systemd-udev-2.0
Obsoletes: systemd-udev < 2

systemd-boot-unsigned-2.0
Obsoletes: systemd-udev < 2

In this case dnf install systemd-udev may lead to installation of systemd-boot-unsigned which is probably not what the user expected. The reason is the split in the upgrade-path created by obsolete and both branches - systemd-udev-2.0 and systemd-boot-unsigned-2.0 are considered valid.

With this patch install command takes into account only obsoleters of the best version of the package so the dnf install systemd-udev results in correct installation of systemd-udev-2.0 package.

Resolves: #339
Requires tests adjustment: rpm-software-management/ci-dnf-stack#1236

m-blaha · 2023-03-15T09:51:02Z

Test rpm-software-management/ci-dnf-stack#1235

m-blaha · 2023-03-15T11:11:48Z

Oh, the test for this issue is already present so I'll only change it according to the changed behavior.

j-mracek · 2023-03-20T07:07:53Z

libdnf/base/goal.cpp

+    libdnf::rpm::PackageQuery data_query(base_query.get_base(), libdnf::sack::ExcludeFlags::APPLY_EXCLUDES, true);
+    data_query.update(data);
+    data_query.filter_priority();
+    data_query.filter_latest_evr();


I am thinking about two edge cases where we might have a problem. Both cases are related to situation when package name is installed.

When vendor change is not allowed (configuration allow_vendor_change)

We will have a different set of candidates in comparison to upgrade request with the same argument

I have a suggestion - if data contains installed package than use the original approach. What do you think?

I think we should have test scenarios for these situations. I'll try to prepare them - it will definitely help me wrap my head around it.

j-mracek · 2023-03-21T07:04:22Z

libdnf/base/goal.cpp

@@ -45,8 +45,26 @@ along with libdnf.  If not, see <https://www.gnu.org/licenses/>.
 namespace {

 void add_obsoletes_to_data(const libdnf::rpm::PackageQuery & base_query, libdnf::rpm::PackageSet & data) {
+    libdnf::rpm::PackageQuery data_query(base_query.get_base(), libdnf::sack::ExcludeFlags::APPLY_EXCLUDES, true);


This is not a problem of the PR - The creation of an empty query was cryptic to me.

What about to have query constructor with packageset and flags?

Yup. The reason is to by-pass the missing constructor.

j-mracek · 2023-03-21T07:13:33Z

It looks like that one test is failing

   @dnf5 @bz1902279
  Scenario: Install of obsoleting package, even though higher version than obsoleted present  # dnf/obsoletes.feature:34
    Given I use repository "dnf-ci-obsoletes"                                                 # dnf/steps/repo.py:182
    When I execute dnf with args "install PackageE"                                           # dnf/steps/cmd.py:106
    Then the exit code is 0                                                                   # common/output.py:57
    And Transaction is following                                                              # dnf/steps/transaction.py:200
      | Action  | Package                           |
      | install | PackageA-Obsoleter-0:1.0-1.x86_64 |
      Assertion Failed: [rpmdb] Package PackageA-Obsoleter-0:1.0-1.x86_64 not 'install'; Possible candidates: PackageE-0:3.0-1.x86_64
      Captured stdout:

I looked at the bug and it looks like that test is wrong (keeping wrong behavior) and your patch fix it. May I ask you to modify the test?

It's a shortcut instead of creating an empty PackageQuery and then updating it with the content of PackageSet.

In situation where a package exists in multiple versions and some older version is being obsoleted, any of obsoleters was considered a valid solution. The result could be really misleading. For example let's have this package set: systemd-udev-1.0 systemd-udev-2.0 Obsoletes: systemd-udev < 2 systemd-boot-unsigned-2.0 Obsoletes: systemd-udev < 2 In this case `dnf install systemd-udev` may lead to installation of systemd-boot-unsigned which is probably not what the user expected. The reason is the split in the upgrade-path created by obsolete and both branches - systemd-udev-2.0 and systemd-boot-unsigned-2.0 are considered valid. With this patch install command takes into account only obsoleters of the best version of the package so the `dnf install systemd-udev` results in correct installation of systemd-udev-2.0 package.

j-mracek · 2023-03-21T12:30:19Z

libdnf/rpm/package_query.cpp

@@ -267,6 +267,11 @@ PackageQuery::PackageQuery(const BaseWeakPtr & base, ExcludeFlags flags, bool em
 PackageQuery::PackageQuery(libdnf::Base & base, ExcludeFlags flags, bool empty)
    : PackageQuery(base.get_weak_ptr(), flags, empty) {}

+PackageQuery::PackageQuery(const PackageSet & pkgset, ExcludeFlags flags)
+    : PackageQuery(pkgset.get_base(), flags, true) {
+    update(pkgset);


This is functional but it looks like a strange implementation. I have to check internals

The constructor could look like following. But there is one small difference - creation of empty Map nad and operator vs copy of Map. And it is more code.

PackageQuery::PackageQuery(const PackageSet & pkgset, ExcludeFlags flags) : PackageSet(src), p_pq_impl(new PQImpl) { p_pq_impl->flags = flags; auto & pool = get_rpm_pool(base); switch (flags) { case ExcludeFlags::APPLY_EXCLUDES: // Considered map in Pool uses APPLY_EXCLUDES. It can be used in this case. base->get_rpm_package_sack()->p_impl->recompute_considered_in_pool(); break; case ExcludeFlags::IGNORE_EXCLUDES: break; default: { auto considered = base->get_rpm_package_sack()->p_impl->compute_considered_map(flags); if (considered) { p_pq_impl->considered_cache = std::move(considered); } break; } } }

Please the decision is up to you.

On the other hand your solution introduces code duplicity (it's basically a copy of current PackageQuery ctor).

I don't know, the difference in performance is one SolvMap | operation. Maybe if this constructor is massively used, then even this small performance increase may make a difference. But now there is only one place where this constructor is actually used.

Or would it be better from your point of view to use directly something like *p_impl |= *pkgset.p_impl instead of update()?

There is no performance difference between both solutions. Creation of Map and copy is cheap. And I agree that there is an overlap with other code.

j-mracek · 2023-03-21T14:13:41Z

LGTM

m-blaha mentioned this pull request Mar 15, 2023

Test for package split rpm-software-management/ci-dnf-stack#1235

Closed

m-blaha mentioned this pull request Mar 15, 2023

Adjust obsoletes test to new dnf5 behavior. rpm-software-management/ci-dnf-stack#1236

Merged

j-mracek self-assigned this Mar 20, 2023

j-mracek reviewed Mar 20, 2023

View reviewed changes

m-blaha force-pushed the mblaha/split_install branch from e3b2fe1 to d50c2bc Compare March 20, 2023 12:38

j-mracek reviewed Mar 21, 2023

View reviewed changes

m-blaha added 2 commits March 21, 2023 12:31

PackageQuery: New ctor based on the PackageSet instance

077b1cc

It's a shortcut instead of creating an empty PackageQuery and then updating it with the content of PackageSet.

m-blaha force-pushed the mblaha/split_install branch from d50c2bc to 28ad621 Compare March 21, 2023 11:32

j-mracek reviewed Mar 21, 2023

View reviewed changes

j-mracek merged commit fef4d28 into main Mar 21, 2023

j-mracek deleted the mblaha/split_install branch March 21, 2023 14:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

goal: Better handling of obsoleted pkg installation #354

goal: Better handling of obsoleted pkg installation #354

m-blaha commented Mar 15, 2023 •

edited

Loading

m-blaha commented Mar 15, 2023

m-blaha commented Mar 15, 2023

j-mracek Mar 20, 2023

m-blaha Mar 20, 2023

j-mracek Mar 21, 2023

j-mracek Mar 21, 2023

m-blaha Mar 21, 2023

j-mracek commented Mar 21, 2023 •

edited

Loading

j-mracek Mar 21, 2023

j-mracek Mar 21, 2023 •

edited

Loading

j-mracek Mar 21, 2023

m-blaha Mar 21, 2023

m-blaha Mar 21, 2023

j-mracek Mar 21, 2023

j-mracek commented Mar 21, 2023

goal: Better handling of obsoleted pkg installation #354

goal: Better handling of obsoleted pkg installation #354

Conversation

m-blaha commented Mar 15, 2023 • edited Loading

m-blaha commented Mar 15, 2023

m-blaha commented Mar 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-mracek commented Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

j-mracek Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-mracek commented Mar 21, 2023

m-blaha commented Mar 15, 2023 •

edited

Loading

j-mracek commented Mar 21, 2023 •

edited

Loading

j-mracek Mar 21, 2023 •

edited

Loading