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

nevra: Fix cmp_* comparators #272

Merged
merged 1 commit into from
Feb 9, 2023
Merged

nevra: Fix cmp_* comparators #272

merged 1 commit into from
Feb 9, 2023

Conversation

m-blaha
Copy link
Member

@m-blaha m-blaha commented Feb 8, 2023

Current implementation used rpmvercmp() on the whole EVR. But unfortunatelly rpmvercmp does not do any NEVRA parsing, it just compares two strings, which led to incorrect results.

For example these packages are in the current code ordered wrong:

firefox-0:100.0-2.fc36.x86_64
firefox-0:109.0.1-1.fc36.x86_64

the reason is that rmpvercmp() correctly returns that '0:100.0-2.fc36' version string is higher than '0:109.0.1-1.fc36'. But what is true for strings is not true for EVR. We need to compare each part (epoch, version, and release) separately.
For this a new evrcmp() function is introduced.

Current implementation used rpmvercmp() on the whole EVR. But
unfortunatelly rpmvercmp does not do any NEVRA parsing, it just compares
two strings, which led to incorrect results.

For example these packages are in the current code ordered wrong:

firefox-0:100.0-2.fc36.x86_64
firefox-0:109.0.1-1.fc36.x86_64

the reason is that rmpvercmp() correctly returns that '0:100.0-2.fc36'
version string is higher than '0:109.0.1-1.fc36'. But what is true for
strings is not true for EVR. We need to compare each part (epoch,
version, and release) separately.
For this a new evrcmp() function is introduced.
@j-mracek
Copy link
Contributor

j-mracek commented Feb 8, 2023

@m-blaha I am just curious, where do you want to use the nevra_cmp?

@m-blaha
Copy link
Member Author

m-blaha commented Feb 8, 2023

list command and it's behaviour in case no switch was applied (in dnf4 it was --all).
I need to filter available packages and keep only those which are not installed and those with version greater than the installed one.
My first attempt was to use queries, but the performance was poor (still possible, that it can be done better without this new comparator). My current solution is this (uses evrcmp):

if (!showduplicates) {
available.filter_latest_evr();
// keep only those available packages that are either not installed or
// available EVR is higher than the installed one
libdnf::rpm::PackageQuery installed_latest(installed);
installed_latest.filter_latest_evr();
std::unordered_map<std::string, libdnf::rpm::Nevra> latest_installed_evr;
for (const auto & pkg : installed_latest) {
libdnf::rpm::Nevra nevra;
nevra.set_epoch(pkg.get_epoch());
nevra.set_version(pkg.get_version());
nevra.set_release(pkg.get_release());
latest_installed_evr.emplace(pkg.get_na(), std::move(nevra));
}
for (const auto & pkg : available) {
auto installed_version = latest_installed_evr.find(pkg.get_na());
if (installed_version != latest_installed_evr.end()) {
if (libdnf::rpm::evrcmp(installed_version->second, pkg) >= 0) {
available.remove(pkg);
}
}
}
}

@m-blaha
Copy link
Member Author

m-blaha commented Feb 8, 2023

And if you meant cmp_nevra, I do not have usage for it. It was just there and it was broken.
cmp_naevr on the other hand is used to sort packages for output.

@j-mracek
Copy link
Contributor

j-mracek commented Feb 8, 2023

The PR looks fine

@j-mracek j-mracek self-assigned this Feb 9, 2023
@j-mracek j-mracek merged commit 836d5db into main Feb 9, 2023
@j-mracek j-mracek deleted the mblaha/fix_nevra_cmp branch February 9, 2023 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants