Skip to content
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

When limiting installonly pkg handle each provider just once #1261

Merged

Conversation

kontura
Copy link
Contributor

@kontura kontura commented Feb 20, 2024

First gather all unique providers of installonly provides and then
handle each set of packages (providers) with the same name.

Previously the handling was done per provide, this could result in repeated
handling of a single package if it provided multiple installonly
provides. Such as kernel and installonlypkg(kernel).
This could also lead to problems when multiple versions of installonly
pkgs didn't have the same installonly provides.

https://bugzilla.redhat.com/show_bug.cgi?id=2263675

Test: rpm-software-management/ci-dnf-stack#1454

@kontura kontura changed the title When limiting installonly pkg handle each provides just once When limiting installonly pkg handle each provide just once Feb 21, 2024
@j-mracek j-mracek self-assigned this Feb 21, 2024
@kontura kontura changed the title When limiting installonly pkg handle each provide just once When limiting installonly pkg handle each provider just once Feb 21, 2024
@kontura kontura marked this pull request as ready for review February 22, 2024 11:55
// if some unused provider is also available we need to mark it ERASE as well.
auto low = std::lower_bound(
available_unused_providers.begin(), available_unused_providers.end(), name, name_solvable_id_name_cmp_key);
while (low != available_unused_providers.end() && (*low)->name == name) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that there is a long standing problem, because we always calculate the order according to name, but may be we should add architecture. Think about x86_64 and i686. Do you think whether it would be worth to add an architecture in account?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might sound strange, but handling of available_unused_providers by name is OK (or more secure), but handling of marked_for_install_providers by only name might be an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be another change in behavior so I would rather leave that for a different PR.

But I would be interested to know what kind of issues it might cause, perhaps a new github issue to track that would be nice. 👍

static void same_name_subqueue(libdnf5::solv::RpmPool & pool, Queue * in, Queue * out) {

/// @brief It takes an `in` Queue which contains packages sorted by name (see `installonly_cmp`).
/// It selects the first name and upon return the `out` Queue is populated with packages
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be I do not understand it correctly. I don't see that it states that it moves all packages (Ids) with the name that is returned from in to out Queue. Or did I overlooked something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully its better now.

It should have identical results but packages are only not marked
for ERASE multiple times.
First gather all unique providers of installonly provides and then
handle each set of packages (providers) with the same name.

Previously the handling was done per provide, this could result repeated
handling of a single package if it provided multiple installonly
provides. Such as `kernel` and `installonlypkg(kernel)`.
This could also lead to problems when multiple versions of installonly
pkgs didn't have the same installonly provides.

https://bugzilla.redhat.com/show_bug.cgi?id=2263675
@kontura kontura force-pushed the custom-overlimit-kernel branch from fc7550c to 1dcd416 Compare February 28, 2024 13:32
Copy link
Contributor

@j-mracek j-mracek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, ready to merge after all test finish.

@j-mracek j-mracek added this pull request to the merge queue Mar 1, 2024
Merged via the queue into rpm-software-management:main with commit a31446b Mar 1, 2024
14 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants