From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michael Tremer <michael.tremer@ipfire.org>
To: development@lists.ipfire.org
Subject: Re: [PATCH 2/3] pakfire: add 'info <pak(s)>' option
Date: Fri, 14 May 2021 13:19:46 +0100
Message-ID: <DD252A9B-2A7D-4B9B-8B74-BD946F8293A3@ipfire.org>
In-Reply-To: <c5ae94572b4446714c286cf10af84657a7d7889e.camel@disroot.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============9140748774367894206=="
List-Id: <development.lists.ipfire.org>

--===============9140748774367894206==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Hello,

All the mess is probably my fault. I was young and had no clue what I was doi=
ng. Now I am older=E2=80=A6 still not having a clue what I am doing.

I would say your solution looks clean, but as Jonatan suggested, a unique fun=
ction 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:
>=20
> Hi Jonatan
>=20
> I completely agree with your remarks about the code duplication and the
> separation of GUI vs logic.. Actually even a bit relieved..=20
>=20
> 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 :-)..=20
>=20
> 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.
>=20
> 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.
>=20
> 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..
>=20
> Regards
> Robin
>=20
> Jonatan Schlag schreef op do 13-05-2021 om 10:02 [+0200]:
>> Hi,
>>=20
>> had a look at your code and found some more generic possibilities to=20
>> improve it :-)
>>=20
>> 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.
>>>=20
>>> 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(-)
>>>=20
>>> diff --git a/src/pakfire/lib/functions.pl=20
>>> 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]=20
>>> <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=20
>>> 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 {
>>>         }
>>>  }
>>>=20
>>> +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 =3D shift;
>>> +       my $info_type =3D shift;
>>> +
>>> +       my $found =3D 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=20
>>> version
>>> +               open(FILE, "<$Conf::dbdir/lists/packages_list.db");
>>> +               my @db =3D <FILE>;
>>> +               close(FILE);
>>> +
>>> +               foreach (@db) {
>>> +                       @templine =3D split(/;/,$_);
>>> +                       if ("$templine[0]" eq "$pak" ) {
>>> +                               $found =3D 1;
>>> +                               $available_version =3D $templine[1];
>>> +                               $available_release =3D $templine[2];
>>> +                               break;
>>> +                       }
>>> +               }
>>> +       }
>>> +
>> We have somehow similar code in dblist(). A very good example of why
>> UI=20
>> code and logic should be split up.
>> Just want to mention it. It is definitely not your fault, and I do
>> not=20
>> 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=20
>> like that:
>>=20
>> { (package-name, status in {installed, needs-upgrade, notinstalled
>> })}
>>=20
>> or that:
>>=20
>> [
>> installed: {set of packages which are installed}
>> needs upgrade: {self explanatory }
>> not installed: {self explanatory }
>> ]
>>=20
>> [] is a dictionary so in perl somehthing like %data =3D ();
>> () is a tupel so in perl an array
>> {} is a set so in perl an array
>>=20
>> and use this function here and in dblist.
>>=20
>>> +       ### Parse latest package metadata
>>> +       my $installed =3D !&isinstalled("$pak");
>>> +       my @file;
>>> +
>>> +       if ($found && "$info_type" eq "latest") {
>>> +               getmetafile("$pak");
>>> +
>>> +               open(FILE, "<$Conf::dbdir/meta/meta-$pak");
>>> +               @file =3D <FILE>;
>>> +               close(FILE);
>>> +       } elsif ($installed) {
>>> +               open(FILE, "<$Conf::dbdir/installed/meta-$pak");
>>> +               @file =3D <FILE>;
>>> +               close(FILE);
>> You could open the file at 1
>>> +       } else {
>>> +               message("PAKFIRE WARN: Pak '$pak' not found.");
>>> +               return 1;
>>> +       }
>>=20
>> Here is 1 and deduplicate the code.
>>> +
>>> +       my ($name, $summary, $size, $dependencies, $pakfile,
>>> $initscripts);
>>> +       foreach $line (@file) {
>>> +               @templine =3D split(/\: /,$line);
>>> +               if ("$templine[0]" eq "Name") {
>>> +                       $name =3D $templine[1];
>>> +                       chomp($name);
>>> +               } elsif ("$templine[0]" eq "Summary") {
>>> +                       $summary =3D $templine[1];
>>> +                       chomp($summary);
>>> +               } elsif ("$templine[0]" eq "Size") {
>>> +                       $size =3D &beautifysize("$templine[1]");
>>> +                       chomp($size);
>>> +               } elsif ("$templine[0]" eq "Dependencies") {
>>> +                       $dependencies =3D $templine[1];
>>> +                       chomp($dependencies);
>>> +               } elsif ("$templine[0]" eq "File") {
>>> +                       $pakfile =3D $templine[1];
>>> +                       chomp($pakfile);
>>> +               } elsif ("$templine[0]" eq "InitScripts") {
>>> +                       $initscripts =3D $templine[1];
>>> +                       chomp($initscripts);
>>> +               }
>>> +       }
>>=20
>> And here I somehow do not know what to say :-). The problem is that
>> both=20
>> blocks are code duplication. But we already have this kind of code=20
>> 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=20
>> which just returns a hash, and you could do something like that:
>>=20
>> metadata =3D get_metadate("$pak")
>>=20
>> metdata["installed_version"]
>>=20
>> Both functions would be also highly testable as we could lay down an=20
>> example file and test if this file is read correctly.
>>=20
>> If now someone wonders why I wrote this down, it may help to learn
>> new=20
>> things, as I do when I read the list. I am unsure what advice a
>> should=20
>> give you @Robin: it is not your fault that the code looks like it
>> looks,=20
>> but on the other hand, making things "wrong" only because others did
>> it=20
>> before is also a dead end. So I hope others have an idea, what to do.
>>=20
>> Greetings Jonatan
>>> +
>>> +       ### Get installed version information
>>> +       my ($installed_version, $installed_release);
>>> +
>>> +       if ($installed) {
>>> +               open(FILE, "<$Conf::dbdir/installed/meta-$pak");
>>> +               @file =3D <FILE>;
>>> +               close(FILE);
>>> +
>>> +               foreach $line (@file) {
>>> +                       @templine =3D split(/\: /,$line);
>>> +                       if ("$templine[0]" eq "ProgVersion") {
>>> +                               $installed_version =3D $templine[1];
>>> +                               chomp($installed_version);
>>> +                       } elsif ("$templine[0]" eq "Release") {
>>> +                               $installed_release =3D $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")=20
>>> {
>>> +                       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 =3D shift;
>>>=20
>>> 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" =3D~ "^-") {
>>> +                               push(@paks,$pak);
>>> +                       }
>>> +               }
>>> +
>>> +               unless ("@paks") {
>>> +                       &Pakfire::message("PAKFIRE ERROR: missing
>>> package name");
>>> +                       &Pakfire::usage;
>>> +                       exit 1;
>>> +               }
>>> +
>>> +               foreach $pak (@paks) {
>>> +                       &Pakfire::pakinfo("$pak", "latest");
>>> +               }
>>>=20
>>>         } elsif ("$ARGV[0]" eq "resolvedeps") {
>>>                 foreach (@ARGV) {
>>> --
>>> 2.31.1
>>=20
>=20
>=20
> --=20
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.
>=20


--===============9140748774367894206==--