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

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

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
  

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
>
  

Hello,

All the mess is probably my fault. I was young and had no clue what I was doing. Now I am older… still not having a clue what I am doing.

I would say your solution looks clean, but as Jonatan suggested, a unique function to read the package metadata and return it as a hash would help. If you want to add that, I am happy to merge the patch.

Generally I am interested in seeing pakfire being tidied up a little bit, but that has to happen in small steps that are reviewable and testable. Anything that breaks might stop people from installing updates and us from shipping a fix.

-Michael

> On 13 May 2021, at 20:11, Robin Roevens <robin.roevens@disroot.org> wrote:
> 
> 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
>> 
> 
> 
> -- 
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.
>
  

Hi

Michael Tremer schreef op vr 14-05-2021 om 13:19 [+0100]:
> Hello,
> 
> All the mess is probably my fault. I was young and had no clue what I
> was doing. Now I am older… still not having a clue what I am doing.
> 

We all were young once.. and who really does have a clue of what he is
actually doing :-).. 

> I would say your solution looks clean, but as Jonatan suggested, a
> unique function to read the package metadata and return it as a hash
> would help. If you want to add that, I am happy to merge the patch.
> 

I will certainly look into that.

> Generally I am interested in seeing pakfire being tidied up a little
> bit, but that has to happen in small steps that are reviewable and
> testable. Anything that breaks might stop people from installing
> updates and us from shipping a fix.
> 

Indeed pakfire is quite essential to be as bug-free as possible, so we
will have to be very carefully with it. I'll see what I can do over
time as I'm currently still a rookie in Perl; but I have plenty of
experience in different kinds of languages and scripts, that can come
in handy.

Robin

> -Michael
> 
> > On 13 May 2021, at 20:11, Robin Roevens <robin.roevens@disroot.org>
> > wrote:
> > 
> > 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
> > > 
> > 
> > 
> > -- 
> > Dit bericht is gescanned op virussen en andere gevaarlijke
> > inhoud door MailScanner en lijkt schoon te zijn.
> > 
> 
>
  

Hello,

> On 14 May 2021, at 21:24, Robin Roevens <robin.roevens@disroot.org> wrote:
> 
> Hi
> 
> Michael Tremer schreef op vr 14-05-2021 om 13:19 [+0100]:
>> Hello,
>> 
>> All the mess is probably my fault. I was young and had no clue what I
>> was doing. Now I am older… still not having a clue what I am doing.
>> 
> 
> We all were young once.. and who really does have a clue of what he is
> actually doing :-).. 

I hope I learned a couple of things on the way and produce a little bit less bullshit.

But that is why we have this list, so that we can call each other out if we produce garbage :)

>> I would say your solution looks clean, but as Jonatan suggested, a
>> unique function to read the package metadata and return it as a hash
>> would help. If you want to add that, I am happy to merge the patch.
>> 
> 
> I will certainly look into that.
> 
>> Generally I am interested in seeing pakfire being tidied up a little
>> bit, but that has to happen in small steps that are reviewable and
>> testable. Anything that breaks might stop people from installing
>> updates and us from shipping a fix.
>> 
> 
> Indeed pakfire is quite essential to be as bug-free as possible, so we
> will have to be very carefully with it. I'll see what I can do over
> time as I'm currently still a rookie in Perl; but I have plenty of
> experience in different kinds of languages and scripts, that can come
> in handy.

Perl is always full of surprises. So many things happen implicitly and I constantly run into unexpected behaviour. It probably is documented somewhere, but it takes about 10 years of experience to get Perl right in the first place. It is also a write-only language.

I recently wrote those functions. It is very hard to understand what is actually happening:

  https://git.ipfire.org/?p=people/ms/ipfire-2.x.git;a=commitdiff;h=dc5a53feed5059c43bb0d40b9ca6d10e6c128990

-Michael

> 
> Robin
> 
>> -Michael
>> 
>>> On 13 May 2021, at 20:11, Robin Roevens <robin.roevens@disroot.org>
>>> wrote:
>>> 
>>> 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
>>>> 
>>> 
>>> 
>>> -- 
>>> Dit bericht is gescanned op virussen en andere gevaarlijke
>>> inhoud door MailScanner en lijkt schoon te zijn.
>>> 
>> 
>> 
> 
> 
> -- 
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.