Re: [PATCH 2/3] pakfire: add 'info <pak(s)>' option

[Jonatan Schlag <jonatan.schlag@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 7728 bytes --]

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(a)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