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


-- 
Dit bericht is gescanned op virussen en andere gevaarlijke
inhoud door MailScanner en lijkt schoon te zijn.