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

[Michael Tremer <michael.tremer@ipfire.org>]

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

Hello,

> On 14 May 2021, at 21:24, Robin Roevens <robin.roevens(a)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(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.