From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer To: development@lists.ipfire.org Subject: Re: [PATCH 2/3] pakfire: add 'info ' option Date: Tue, 25 May 2021 12:22:12 +0100 Message-ID: <54194C3C-B13E-47AE-8BA2-D54DDF8EDF29@ipfire.org> In-Reply-To: <4db47b94d6099c34bee4239b5fad7d3b27af8b5a.camel@disroot.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2847467949070123674==" List-Id: --===============2847467949070123674== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello, > On 14 May 2021, at 21:24, Robin Roevens wrote: >=20 > Hi >=20 > Michael Tremer schreef op vr 14-05-2021 om 13:19 [+0100]: >> Hello, >>=20 >> All the mess is probably my fault. I was young and had no clue what I >> was doing. Now I am older=E2=80=A6 still not having a clue what I am doing. >>=20 >=20 > We all were young once.. and who really does have a clue of what he is > actually doing :-)..=20 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 p= roduce 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. >>=20 >=20 > I will certainly look into that. >=20 >> 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. >>=20 >=20 > 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 cons= tantly run into unexpected behaviour. It probably is documented somewhere, bu= t 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 actua= lly happening: https://git.ipfire.org/?p=3Dpeople/ms/ipfire-2.x.git;a=3Dcommitdiff;h=3Ddc5= a53feed5059c43bb0d40b9ca6d10e6c128990 -Michael >=20 > Robin >=20 >> -Michael >>=20 >>> On 13 May 2021, at 20:11, Robin Roevens >>> 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 ' option to pakfire to display pakfile >>>>> metadata >>>>> and current state of the pak on the system. >>>>>=20 >>>>> Signed-off-by: Robin Roevens >>>>> --- >>>>> 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 [options] >>>>> "); >>>>> &Pakfire::message(" - Contacts the >>>>> servers >>>>> for new lists of paks."); >>>>> &Pakfire::message(" - Installs the >>>>> latest >>>>> version of all paks."); >>>>> - &Pakfire::message(" - Outputs a short >>>>> list >>>>> with all available paks."); >>>>> + &Pakfire::message(" >>>>> [installed/notinstalled] - >>>>> Outputs a list with all, installed or available paks."); >>>>> + &Pakfire::message(" - Output >>>>> pak=20 >>>>> metadata."); >>>>> &Pakfire::message(" - 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 ; >>>>> + 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 ; >>>>> + close(FILE); >>>>> + } elsif ($installed) { >>>>> + open(FILE, "<$Conf::dbdir/installed/meta- >>>>> $pak"); >>>>> + @file =3D ; >>>>> + 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 ; >>>>> + 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 >>=20 >>=20 >=20 >=20 > --=20 > Dit bericht is gescanned op virussen en andere gevaarlijke > inhoud door MailScanner en lijkt schoon te zijn. --===============2847467949070123674==--