From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jonatan Schlag To: development@lists.ipfire.org Subject: Re: [PATCH 2/3] pakfire: add 'info ' option Date: Thu, 13 May 2021 10:02:55 +0200 Message-ID: <05dc65fec884222d76bec2cbe22fcbcf@ipfire.org> In-Reply-To: <20210423161534.32738-3-robin.roevens@disroot.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0540892242957756669==" List-Id: --===============0540892242957756669== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Hi, had a look at your code and found some more generic possibilities to improve it :-) Am 2021-04-23 18:15, schrieb Robin Roevens: > Add an 'info ' option to pakfire to display pakfile metadata > and current state of the pak on the system. > > Signed-off-by: Robin Roevens > --- > src/pakfire/lib/functions.pl | 124 ++++++++++++++++++++++++++++++++++- > src/pakfire/pakfire | 21 ++++++ > 2 files changed, 144 insertions(+), 1 deletion(-) > > diff --git a/src/pakfire/lib/functions.pl > b/src/pakfire/lib/functions.pl > index 4d5c6219a..46da06a88 100644 > --- a/src/pakfire/lib/functions.pl > +++ b/src/pakfire/lib/functions.pl > @@ -110,7 +110,8 @@ sub usage { > &Pakfire::message("Usage: pakfire [options] > "); > &Pakfire::message(" - Contacts the servers > for new lists of paks."); > &Pakfire::message(" - Installs the latest > version of all paks."); > - &Pakfire::message(" - Outputs a short list > with all available paks."); > + &Pakfire::message(" [installed/notinstalled] - > Outputs a list with all, installed or available paks."); > + &Pakfire::message(" - Output pak > metadata."); > &Pakfire::message(" - Outputs a summary > about available core upgrades, updates and a required reboot"); > &Pakfire::message(""); > &Pakfire::message(" Global options:"); > @@ -538,6 +539,127 @@ sub dblist { > } > } > > +sub pakinfo { > + ### This subroutine shows available info for a package > + # Pass package name and type of info as argument: > &Pakfire::pakinfo(package, "latest") > + # Type can be "latest" or "installed" > + # Usage is always with two argument. > + my $pak = shift; > + my $info_type = shift; > + > + my $found = 0; > + my @templine; > + my ($available_version, $available_release); > + > + if ("$info_type" eq "latest") { > + ### Make sure that the info is not outdated. > + &Pakfire::dbgetlist("noforce"); > + > + ### Check if package is in packages_list and get latest available > version > + open(FILE, "<$Conf::dbdir/lists/packages_list.db"); > + my @db = ; > + close(FILE); > + > + foreach (@db) { > + @templine = split(/;/,$_); > + if ("$templine[0]" eq "$pak" ) { > + $found = 1; > + $available_version = $templine[1]; > + $available_release = $templine[2]; > + break; > + } > + } > + } > + We have somehow similar code in dblist(). A very good example of why UI code and logic should be split up. Just want to mention it. It is definitely not your fault, and I do not want to blame you for the errors others made. But maybe it would be worth it to think about a function which either returns something like that: { (package-name, status in {installed, needs-upgrade, notinstalled })} or that: [ installed: {set of packages which are installed} needs upgrade: {self explanatory } not installed: {self explanatory } ] [] is a dictionary so in perl somehthing like %data = (); () is a tupel so in perl an array {} is a set so in perl an array and use this function here and in dblist. > + ### Parse latest package metadata > + my $installed = !&isinstalled("$pak"); > + my @file; > + > + if ($found && "$info_type" eq "latest") { > + getmetafile("$pak"); > + > + open(FILE, "<$Conf::dbdir/meta/meta-$pak"); > + @file = ; > + close(FILE); > + } elsif ($installed) { > + open(FILE, "<$Conf::dbdir/installed/meta-$pak"); > + @file = ; > + close(FILE); You could open the file at 1 > + } else { > + message("PAKFIRE WARN: Pak '$pak' not found."); > + return 1; > + } Here is 1 and deduplicate the code. > + > + my ($name, $summary, $size, $dependencies, $pakfile, $initscripts); > + foreach $line (@file) { > + @templine = split(/\: /,$line); > + if ("$templine[0]" eq "Name") { > + $name = $templine[1]; > + chomp($name); > + } elsif ("$templine[0]" eq "Summary") { > + $summary = $templine[1]; > + chomp($summary); > + } elsif ("$templine[0]" eq "Size") { > + $size = &beautifysize("$templine[1]"); > + chomp($size); > + } elsif ("$templine[0]" eq "Dependencies") { > + $dependencies = $templine[1]; > + chomp($dependencies); > + } elsif ("$templine[0]" eq "File") { > + $pakfile = $templine[1]; > + chomp($pakfile); > + } elsif ("$templine[0]" eq "InitScripts") { > + $initscripts = $templine[1]; > + chomp($initscripts); > + } > + } And here I somehow do not know what to say :-). The problem is that both blocks are code duplication. But we already have this kind of code duplication at a lot of places (I counted 5). So it is ok to do it that way, but it would be better to have a function which just returns a hash, and you could do something like that: metadata = get_metadate("$pak") metdata["installed_version"] Both functions would be also highly testable as we could lay down an example file and test if this file is read correctly. If now someone wonders why I wrote this down, it may help to learn new things, as I do when I read the list. I am unsure what advice a should give you @Robin: it is not your fault that the code looks like it looks, but on the other hand, making things "wrong" only because others did it before is also a dead end. So I hope others have an idea, what to do. Greetings Jonatan > + > + ### Get installed version information > + my ($installed_version, $installed_release); > + > + if ($installed) { > + open(FILE, "<$Conf::dbdir/installed/meta-$pak"); > + @file = ; > + close(FILE); > + > + foreach $line (@file) { > + @templine = split(/\: /,$line); > + if ("$templine[0]" eq "ProgVersion") { > + $installed_version = $templine[1]; > + chomp($installed_version); > + } elsif ("$templine[0]" eq "Release") { > + $installed_release = $templine[1]; > + chomp($installed_release); > + } > + } > + } > + > + print "Name: $name\n"; > + if ("$info_type" eq "latest") { > + print "Version: $available_version-$available_release\n"; > + } else { > + print "Version: $installed_version-$installed_release\n"; > + } > + print "Summary: $summary\nSize: $size\nDependencies: > $dependencies\nPakfile: $pakfile\nInitScripts: $initscripts\n"; > + if ($installed) { > + print "Installed: Yes\n"; > + } else { > + print "Installed: No\n"; > + } > + if ("$info_type" eq "latest") { > + if (!$found) { > + print "Status: obsolete (version > $installed_version-$installed_release is installed)\n"; > + } elsif ($installed && "$installed_release" < "$available_release") > { > + print "Status: outdated (version > $installed_version-$installed_release is installed)\n"; > + } elsif ($installed) { > + print "Status: up-to-date\n"; > + } else { > + print "Status: not installed\n"; > + } > + } > + print "\n"; > +} > + > sub resolvedeps_one { > my $pak = shift; > > diff --git a/src/pakfire/pakfire b/src/pakfire/pakfire > index c69a8d3ad..5507c8e92 100644 > --- a/src/pakfire/pakfire > +++ b/src/pakfire/pakfire > @@ -303,6 +303,27 @@ > &Pakfire::message("PAKFIRE WARN: Not a known option $ARGV[1]") if > ($ARGV[1]); > &Pakfire::dblist("all", "noweb"); > } > + > + } elsif ("$ARGV[0]" eq "info") { > + shift; > + > + my @paks; > + my $pak; > + foreach $pak (@ARGV) { > + unless ("$pak" =~ "^-") { > + push(@paks,$pak); > + } > + } > + > + unless ("@paks") { > + &Pakfire::message("PAKFIRE ERROR: missing package name"); > + &Pakfire::usage; > + exit 1; > + } > + > + foreach $pak (@paks) { > + &Pakfire::pakinfo("$pak", "latest"); > + } > > } elsif ("$ARGV[0]" eq "resolvedeps") { > foreach (@ARGV) { > -- > 2.31.1 --===============0540892242957756669==--