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

avoid removing empty list of packages #1554

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lexming
Copy link

@lexming lexming commented Sep 12, 2022

Fixes #1553

disclaimer: I'm not an expert in Perl, so better solutions may exist.

@@ -907,7 +907,9 @@ sub update_pkgs
$to_rm = $self->packages_to_remove($wanted);
defined($to_rm) or return 0;
Copy link
Member

Choose a reason for hiding this comment

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

i would do here

(defined($to_rm) && $to_rm) or return 0;

() is for readability; and untested ofcourse ;)

Copy link
Author

Choose a reason for hiding this comment

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

An empty $to_rm does not evaluate to false, so (defined($to_rm) && $to_rm) or return 0; continues execution.

Copy link
Author

@lexming lexming Sep 12, 2022

Choose a reason for hiding this comment

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

Probably because an empty $to_rm is () and that is something evaluated as a string (just a guess)

Copy link
Member

Choose a reason for hiding this comment

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

weird, i would have expected an empty Set::Scalar looking at the code. packages_to_remove shouldn't return an undef, so i'm probably missng something here why there is a defined in the first place...

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the defined($to_rm) is intended as error handling in the specific case that the function returned undef, there's no reason for the rest of the code not to run if there is nothing to remove?

Copy link
Member

Choose a reason for hiding this comment

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

Looking at this very briefly, it looks like for some reason $to_rm is not a Set::Scalar here and it should be.

ncm-spma/src/main/perl/spma/yum.pm Outdated Show resolved Hide resolved
@lexming
Copy link
Author

lexming commented Sep 12, 2022

I found the actual underlying cause of this issue in our system. The output of the LEAF_PACKAGES command contains some extra warning messages at the top with blank lines:

$ package-cleanup -c /etc/yum.conf --leaves --all --qf "%{NAME};%{ARCH}"
Loaded plugins: priorities, product-id, subscription-manager, versionlock

This system is not registered with an entitlement server. You can use subscription-manager to register.

SINDES-ca-certificate-cerberus;noarch
SOAPpy;noarch
[...]

The regex in packages_to_remove() does not handle blank lines:

my $leaves = Set::Scalar->new(grep($_ !~ m{\s}, split(/\n/, $out)));

Each blank line becomes an element of $to_rm with an empty string in it.

Fix for the regex in f0c309b

ned21
ned21 previously approved these changes Sep 13, 2022
@ned21 ned21 dismissed their stale review September 13, 2022 07:09

First change has been made, but I have a new comment

@ned21
Copy link
Contributor

ned21 commented Sep 13, 2022

Thanks for finding the root cause, @lexming ! I think it's still worth leaving in the length check in addition the regex fix, but would you mind using git rebase -i to squash b537693 into 45a4cc6? That will keep the history "clean" without confusing things with the previous version was never merged.

@lexming
Copy link
Author

lexming commented Sep 13, 2022

@ned21 sure, commits squashed 👍

@ned21 ned21 added this to the 22.4 milestone Sep 13, 2022
@jrha
Copy link
Member

jrha commented Oct 6, 2022

Looks like the tests are failing on:

2022-09-13T21:09:56.2659621Z Can't use string ("1") as an ARRAY ref while "strict refs" in use at /__w/configuration-modules-core/configuration-modules-core/ncm-spma/target/lib/perl/NCM/Component/spma/yum.pm line 922.
2022-09-13T21:09:56.2666127Z # Tests were run but no plan was declared and done_testing() was not seen.
2022-09-13T21:09:56.2735212Z src/test/perl/yum-update-pkgs.t ................. 

@jrha jrha modified the milestones: 23.6, 23.next Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

ncm-spma: update_pkgs in yum plugin tries to remove empty list of packages
4 participants