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

PackageQuery: Add filter_{latest,earliest}_evr_ignore_arch #1217

Conversation

evan-goode
Copy link
Member

Resolves #1111.

filter_latest_evr and filter_earliest_evr group packages by architecture and keep the latest package on each architecture. After filter_latest_evr, a PackageQuery might have anaconda-1-1.noarch as well as anaconda-2.2.x86_64.

This commit adds filter_latest_evr_ignore_arch and filter_earliest_evr_ignore_arch which filter for packages with the latest versions regardless of their architecture. Filtering the above anaconda example with filter_latest_evr_ignore_arch would result in just {anaconda-2.2.x86_64} since it is the latest version of the package for any arch.

This is an ABI-breaking change and thus is targeted at dnf5 5.2.

@evan-goode evan-goode self-assigned this Jan 30, 2024
@evan-goode evan-goode requested a review from j-mracek January 30, 2024 19:53
/// @param limit If `limit` > 0, keep `limit` number `evr`s in each group.
/// If `limit` < 0, keep all **but** `limit` last `evr`s in each group.
/// @since 5.2
void filter_latest_evr_ignore_arch(int limit = 1);
Copy link
Contributor

@j-mracek j-mracek Jan 31, 2024

Choose a reason for hiding this comment

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

I am thinking about naming.
Important information is Group packages by name or Group packages by name and arch. But how it fits with
ignore_arch?
latest and latest_evr - I think latest is always according evr.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I had filter_latest_any_arch in an earlier draft, do you think that's better than filter_latest_ignore_arch? I agree that ignore_arch is not ideal since ignorearch has an established meaning in our project.

I don't like simply filter_latest as much since filter_latest_unresolved_advisories also groups packages by arch; it seems that grouping by name+arch is common for the two existing filter_latest_* functions.

Copy link
Member

@jan-kolarik jan-kolarik Feb 1, 2024

Choose a reason for hiding this comment

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

filter_*_evr_any_arch sounds good to me. Especially when looking to the current state of sources in the package_query.hpp where the docstrings for filter_latest_evr are followed by the filter_latest_evr_any_arch that gives a clear and coherent feeling.

@@ -2471,7 +2505,8 @@ static void filter_first_sorted_by(
int i;
for (i = 0; i < samename.size(); ++i) {
Solvable * considered = pool.id2solvable(samename[i]);
if (!highest || highest->name != considered->name || highest->arch != considered->arch) {
if (!highest || highest->name != considered->name ||
(grouping == FilterGrouping::BY_NAME_ARCH && (highest->arch != considered->arch))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Only note with no significant impact on the code. It adds one boolean test which is fast, but for each query this test is performed for each name in sets (Fedora has something like 40 000 package names).

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, the only alternative I can think of is duplicating the function or at least the for-loop. It should be only one additional AND instruction though?

@j-mracek j-mracek self-assigned this Jan 31, 2024
@evan-goode evan-goode force-pushed the evan-goode/filter-evr-ignore-arch branch from 63795c6 to c6f7523 Compare February 1, 2024 02:44
@kontura
Copy link
Contributor

kontura commented Feb 5, 2024

I may be missing something but how is this breaking ABI? It is adding two new functions to a class that don't overload anything that should be fine no?

@evan-goode
Copy link
Member Author

Oh 🤦 you're right, I was under the impression that adding any new member would break ABI, but it's only virtual functions and member variables. I can retarget this to main then.

Resolves rpm-software-management#1111.

`filter_latest_evr` and `filter_earliest_evr` group packages by
architecture and keep the latest package on each architecture. After
`filter_latest_evr`, a PackageQuery might have anaconda-1-1.noarch as
well as anaconda-2.2.x86_64.

This commit adds `filter_latest_evr_ignore_arch` and
`filter_earliest_evr_ignore_arch` which filter for packages with the
latest versions regardless of their architecture. Filtering the above
anaconda example with `filter_latest_evr_ignore_arch` would result in
just {anaconda-2.2.x86_64} since it is the latest version of the package
for any arch.

This is an ABI-breaking change and thus is targeted at dnf5 5.2.
@evan-goode evan-goode force-pushed the evan-goode/filter-evr-ignore-arch branch from c6f7523 to 9182973 Compare February 5, 2024 14:54
@evan-goode evan-goode changed the base branch from dnf5-5.2.0.0 to main February 5, 2024 14:55
@evan-goode evan-goode requested a review from j-mracek February 6, 2024 18:01
@evan-goode evan-goode removed their assignment Feb 6, 2024
@j-mracek
Copy link
Contributor

j-mracek commented Feb 7, 2024

LGTM

@j-mracek j-mracek added this pull request to the merge queue Feb 7, 2024
Merged via the queue into rpm-software-management:main with commit ea3c452 Feb 7, 2024
5 of 9 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.

libdnf5 doesn't pick highest NEVRA when a package is available from 2 arches
4 participants