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

[Robin Roevens <robin.roevens@disroot.org>]

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

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.