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.

Sign up for GitHub

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

Jump to bottom

PackageQuery: Add filter_extras #264

Merged
merged 2 commits into from
Feb 6, 2023
Merged

PackageQuery: Add filter_extras #264

merged 2 commits into from
Feb 6, 2023

Conversation

m-blaha
Copy link
Member

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

Filter installed "extra" packages - packages which are installed, but not available in any repository.

@m-blaha
PackageQuery: Add filter_extras
9976b98 
Filter installed "extra" packages - packages which are installed, but
not available in any repository.
@j-mracek
Copy link
Contributor

j-mracek commented Feb 6, 2023

LGTM, but there is one request for a different implementation of extras filter. Bug 1800910 - dnf list extras stopped to shows some packages that should.
I think we should also provide that functionality, but I have no idea, how to delivery it for commandline.

@j-mracek j-mracek self-assigned this Feb 6, 2023
@m-blaha
Copy link
Member Author

m-blaha commented Feb 6, 2023

Hm, yeah. The implementation in the patch uses the same approach as current dnf4 - extras are detected using name.arch.
Maybe adding new switch (and then also a filter), e.g. --extras-versions which can use algorithm that takes also package versions into account? What do you think?

@m-blaha
Copy link
Member Author

m-blaha commented Feb 6, 2023

I've added a commit which extends filter_extras() with optional with_versions parameter. If this is set to true, the whole NEVRA is used to detect extra packages.

j-mracek
j-mracek reviewed Feb 6, 2023
View reviewed changes
include/libdnf/rpm/package_query.hpp Outdated
void filter_extras();
/// Filter packages, which are installed but not available in any enabled repository.
/// @param with_versions If false (default) extras calculation is based only on `name.arch`. That means package is not in extras if any version of the package exists in the repository. If true, filter_extras is more strict and returns each package which exact NEVRA is not present in any repository.
void filter_extras(const bool with_versions=false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, technically it is not a version but EVR.

Just a suggestion. What about to name the variable as exact_evr or exact_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.

Naming, the hardest part of the coding...
I agree, your suggested name for parameter is clearer.

@m-blaha m-blaha force-pushed the mblaha/filter_extras branch from 674835b to be2428b Compare February 6, 2023 09:21
j-mracek
j-mracek reviewed Feb 6, 2023
View reviewed changes
include/libdnf/rpm/package_query.hpp Outdated
@@ -587,6 +587,10 @@ class PackageQuery : public PackageSet {
// TODO(dmach): consider removing the installed packages during the filtering
void filter_priority();

/// Filter packages, which are installed but not available in any enabled repository.
/// @param with_versions If false (default) extras calculation is based only on `name.arch`. That means package is not in extras if any version of the package exists in any of the enabled repositories. If true, filter_extras is more strict and returns each package which exact NEVRA is not present in any enabled repository.
Copy link
Contributor

Choose a reason for hiding this comment

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

What about to replace and returns each package which exact NEVRA is not present in any repository. by and keeps in query installed packages which exact NEVRA is not present in any repository.

include/libdnf/rpm/package_query.hpp Outdated
@@ -587,6 +587,10 @@ class PackageQuery : public PackageSet {
// TODO(dmach): consider removing the installed packages during the filtering
void filter_priority();

/// Filter packages, which are installed but not available in any enabled repository.
Copy link
Contributor

Choose a reason for hiding this comment

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

What about to replace Filter packages by Keep packages?
May I also ask you to describe the behavior with regular excluded packages?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. Done.

@m-blaha m-blaha force-pushed the mblaha/filter_extras branch 3 times, most recently from 99aa6bb to c543674 Compare February 6, 2023 09:37
j-mracek
j-mracek reviewed Feb 6, 2023
View reviewed changes
include/libdnf/rpm/package_query.hpp Outdated
@@ -587,6 +587,16 @@ class PackageQuery : public PackageSet {
// TODO(dmach): consider removing the installed packages during the filtering
void filter_priority();

/// Keep in query only packages that are installed but not available in any enabled
/// repository. Also installed packages that are only part of non-active modules
/// are considered extras.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice improvement, What about to considered extras change to considered as extras?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Sorry for all those mistakes in my English.

j-mracek
j-mracek reviewed Feb 6, 2023
View reviewed changes
libdnf/rpm/package_query.cpp
filter_nevra(available, sack::QueryCmp::NEQ);
} else {
filter_name_arch(available, sack::QueryCmp::NEQ);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am really exited how easily you were able to implemented the filter.

@m-blaha m-blaha force-pushed the mblaha/filter_extras branch 2 times, most recently from e111912 to c74e733 Compare February 6, 2023 11:06
j-mracek
j-mracek reviewed Feb 6, 2023
View reviewed changes
include/libdnf/rpm/package_query.hpp Outdated
@@ -587,6 +587,16 @@ class PackageQuery : public PackageSet {
// TODO(dmach): consider removing the installed packages during the filtering
void filter_priority();

/// Keep in query only packages that are installed but not available in any enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

May I ask you to add a note that filter ignores regular excludes?

Copy link
Member Author

Choose a reason for hiding this comment

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

oops. Sorry, I missed this comment. Should be fixed now.

@m-blaha
PackageQuery: filter_extras can filter by NEVRA
e2fd563 
By default the `filter_extras()` detects extra packages only by name and
arch. That means package is not considered as extra if any version of the
package exists in any of enabled repositories.
With new `exact_evr` parameter the filter is more strict and extra
is any package which is installed and which NEVRA does not exist in any
of enabled repositories.
@m-blaha m-blaha force-pushed the mblaha/filter_extras branch from c74e733 to e2fd563 Compare February 6, 2023 12:23
@j-mracek
Copy link
Contributor

j-mracek commented Feb 6, 2023

LGTM, feel free to merge it when tests pass.

@m-blaha m-blaha merged commit eda6a9b into main Feb 6, 2023
@m-blaha m-blaha deleted the mblaha/filter_extras branch February 6, 2023 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@j-mracek j-mracek j-mracek approved these changes

Assignees

@j-mracek j-mracek

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@m-blaha @j-mracek