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 availableversion
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