Hello, > On 14 May 2021, at 21:24, Robin Roevens 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 >>> 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 ' option to pakfire to display pakfile >>>>> metadata >>>>> and current state of the pak on the system. >>>>> >>>>> Signed-off-by: Robin Roevens >>>>> --- >>>>> 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 [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 >>>>> 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 { >>>>> } >>>>> } >>>>> >>>>> +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 = ; >>>>> + 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 = ; >>>>> + close(FILE); >>>>> + } elsif ($installed) { >>>>> + open(FILE, "<$Conf::dbdir/installed/meta- >>>>> $pak"); >>>>> + @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 = ; >>>>> + 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.