Hi Jonatan
I completely agree with your remarks about the code duplication and the separation of GUI vs logic.. Actually even a bit relieved..
I only continued this way since I didn't want to 'break' with the way code was currently written and as I'm both very new here in this group and I never actually coded in Perl before.. I more or less assumed this is the way it is done here and it was not (yet) my 'right' to question it :-)..
But I would be happy to introduce more reusable functions, less to no duplicate code and a more (m)vc-like approach to the code.
In the meantime I have also been working on a .cgi page for the zabbix agent configuration.. completely in the style currently all cgi pages are where duplicate code is everywhere and logic and view are mangled together.. even with 'goto's all around the place (in most languages where 'goto' exists, it is some kind of taboo to actually use it.. except for plain old BASIC or so.. And it seems to be accepted in the Perl community too.. but still I frowned upon it :-) Anyway, I was about to start thinking about giving up on it since I would not want to maintain such code in the long run.
But if it is ok for you guys to revamp the coding style to a more "modern" style, I'm willing to give that a try..
Regards Robin
Jonatan Schlag schreef op do 13-05-2021 om 10:02 [+0200]:
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 <pak(s)>' option to pakfire to display pakfile metadata and current state of the pak on the system.
Signed-off-by: Robin Roevens robin.roevens@disroot.org
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 <install|remove> [options] <pak(s)>"); &Pakfire::message(" <update> - Contacts the servers for new lists of paks."); &Pakfire::message(" <upgrade> - Installs the latest version of all paks."); - &Pakfire::message(" <list> - Outputs a short list with all available paks."); + &Pakfire::message(" <list> [installed/notinstalled] - Outputs a list with all, installed or available paks."); + &Pakfire::message(" <info> <pak(s)> - Output pak metadata."); &Pakfire::message(" <status> - 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 = <FILE>; + 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 = <FILE>; + close(FILE); + } elsif ($installed) { + open(FILE, "<$Conf::dbdir/installed/meta-$pak"); + @file = <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 = <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