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(a)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(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
>> 
> 
> 
> -- 
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.
>