Message ID | 20210423161534.32738-3-robin.roevens@disroot.org |
---|---|
State | Superseded |
Headers |
Return-Path: <development-bounces@lists.ipfire.org> Received: from mail01.ipfire.org (mail01.haj.ipfire.org [172.28.1.202]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) client-signature ECDSA (P-384)) (Client CN "mail01.haj.ipfire.org", Issuer "R3" (verified OK)) by web04.haj.ipfire.org (Postfix) with ESMTPS id 4FRfYP1j84z44R4 for <patchwork@web04.haj.ipfire.org>; Fri, 23 Apr 2021 16:16:25 +0000 (UTC) Received: from mail02.haj.ipfire.org (mail02.haj.ipfire.org [172.28.1.201]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) client-signature ECDSA (P-384)) (Client CN "mail02.haj.ipfire.org", Issuer "R3" (verified OK)) by mail01.ipfire.org (Postfix) with ESMTPS id 4FRfYL4gRpz1Zb; Fri, 23 Apr 2021 16:16:22 +0000 (UTC) Received: from mail02.haj.ipfire.org (localhost [127.0.0.1]) by mail02.haj.ipfire.org (Postfix) with ESMTP id 4FRfYL13Mqz2yNS; Fri, 23 Apr 2021 16:16:22 +0000 (UTC) Received: from mail01.ipfire.org (mail01.haj.ipfire.org [172.28.1.202]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) client-signature ECDSA (P-384)) (Client CN "mail01.haj.ipfire.org", Issuer "R3" (verified OK)) by mail02.haj.ipfire.org (Postfix) with ESMTPS id 4FRfYK4kVNz2xmS for <development@lists.ipfire.org>; Fri, 23 Apr 2021 16:16:21 +0000 (UTC) Received: from knopi.disroot.org (knopi.disroot.org [178.21.23.139]) (using TLSv1.2 with cipher ECDHE-ECDSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mail01.ipfire.org (Postfix) with ESMTPS id 4FRfYD1lsBzBZ for <development@lists.ipfire.org>; Fri, 23 Apr 2021 16:16:16 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by disroot.org (Postfix) with ESMTP id E4C5252DD7 for <development@lists.ipfire.org>; Fri, 23 Apr 2021 18:16:15 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at disroot.org Received: from knopi.disroot.org ([127.0.0.1]) by localhost (disroot.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id RpL2b0PE7pLJ for <development@lists.ipfire.org>; Fri, 23 Apr 2021 18:16:14 +0200 (CEST) Received: from amaterasu.sicho.home ([192.168.0.1] helo=chojin.sicho.home) by filekeeper.sicho.home with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84_2) (envelope-from <robin.roevens@disroot.org>) id 1lZyTF-0007uY-An; Fri, 23 Apr 2021 18:15:57 +0200 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=disroot.org; s=mail; t=1619194573; bh=JZWJ34kNbtCUvY70OmEJe7qOtQPl3k4muAD54k4inoM=; h=From:To:Cc:Subject:Date:In-Reply-To:References; b=WjYuMdmAKlHhsvyUUu36UArJm/2XA58mYp06muAUAYzrMyYLbjwCHRJurPUUQlp9n ERhCo1arMe2X0517L0H1Zeu4qm3nlUdY1gkILmq0IPsE/md1xPfOUrFiXni/Y1wI+T jjPkpdkA0k6uIbPD/oQAvHmjibAeI1B7VEaIBhU3XS7sGGJSR7hjdrOhkkaXpQXOJt ZTqwncQDmLlgVb6seUWBhaDnPZqL02bHRFDjWFN0WhCx9/25HaR6WJqa2lZSrZ2YbN sGNimIExBxuw1G2NZlPRepR4vkG/gg1vGQzBEYo4ozNUVkSG6XTUvCKFUc1roAp5BJ h7SIXsdldn+mQ== From: Robin Roevens <robin.roevens@disroot.org> To: development@lists.ipfire.org Subject: [PATCH 2/3] pakfire: add 'info <pak(s)>' option Date: Fri, 23 Apr 2021 18:15:33 +0200 Message-Id: <20210423161534.32738-3-robin.roevens@disroot.org> In-Reply-To: <20210423161534.32738-1-robin.roevens@disroot.org> References: <20210423161534.32738-1-robin.roevens@disroot.org> Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-filekeeper-MailScanner-ID: 1lZyTF-0007uY-An X-filekeeper-MailScanner: Found to be clean X-filekeeper-MailScanner-From: robin.roevens@disroot.org X-filekeeper-MailScanner-Watermark: 1619799358.60258@iDO7p3OfJsNy4GRl41wCOQ ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=lists.ipfire.org; s=202003rsa; t=1619194576; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=+F0UDBdWPZv8+7T0EZg9r8/1qGG+Mme6YgJY1eFbhvg=; b=lrnunU2LwBCR9s5z0CID9rVHFaTZ1Eh5w/0ObFo6vWD+HxPBb/y1gQzJCk3u068IPsvRbN Sx3HdsYwnhXj6UOmO0Lv4//WHaZ1qzBZ+S4QS48uZDocWXL0PwE8l73eA3Wg2DWjHxAGXJ yJEd77Quy0Ywd5zwDSirwfXdmh4Z7hwixkYGWKb0iTxRCMts6Dh4/WOo377P0m7gVIGRSv odCaggg5hRW7QBVtW2Oexogh/KxhJSxP2qjxdmkXOA/AxE0y9L8I1aHT2y1ok72sdazdsG k88680IdvZVjBBWD7JRvYKqIx9rGT7m1cwguMg4vQ81WLWeFccq9yU6xxBQ9hQ== ARC-Authentication-Results: i=1; mail01.ipfire.org; dkim=pass header.d=disroot.org header.s=mail header.b=WjYuMdmA; dmarc=pass (policy=quarantine) header.from=disroot.org; spf=pass (mail01.ipfire.org: domain of robin.roevens@disroot.org designates 178.21.23.139 as permitted sender) smtp.mailfrom=robin.roevens@disroot.org ARC-Seal: i=1; s=202003rsa; d=lists.ipfire.org; t=1619194576; a=rsa-sha256; cv=none; b=bTMyxHYEVSjVA0K+kmDLYik0Vgl2C4OGz7ji4JGdczF0klGk9D0Mx7klIosl3pfS0zE2Ag 1obM1z9XMDr9I2076YIJW+Qj3eG95WbjrcUI8AUd2RfLcpaHfmi9en3AptLuwQYMJRX3eC 6LyZ6yjZ/Duq1g6HY+imhJ+LxKbmGIvQcIwCenJ5CMfMyJFfVS4mpC12AmtR0SWanbgleK LDp0pjvwNOD2xqFevmW3Q8pRVyNoXUIC00CYI8VsX5DZa7ymr20algEDOLgGaqZae3oa2B DOzqpgiUHuPEw/AzgtwxJir2d3dFabcRLAePqZyKDfvraY69P6G4PmqXJ3ryIQ== X-Rspamd-Server: mail01.haj.ipfire.org X-Spamd-Result: default: False [-1.90 / 11.00]; ARC_NA(0.00)[]; R_DKIM_ALLOW(-0.20)[disroot.org:s=mail]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; R_SPF_ALLOW(-0.20)[+a:c]; MV_CASE(0.50)[]; MIME_GOOD(-0.10)[text/plain]; PREVIOUSLY_DELIVERED(0.00)[development@lists.ipfire.org]; BROKEN_CONTENT_TYPE(1.50)[]; R_MISSING_CHARSET(2.50)[]; RCVD_COUNT_THREE(0.00)[4]; TO_MATCH_ENVRCPT_SOME(0.00)[]; IP_REPUTATION_HAM(-2.40)[asn: 50673(-0.34), country: NL(-0.01), ip: 178.21.23.139(-0.85)]; DKIM_TRACE(0.00)[disroot.org:+]; RCPT_COUNT_TWO(0.00)[2]; MID_CONTAINS_FROM(1.00)[]; BLOCKLISTDE_FAIL(0.00)[178.21.23.139:server fail]; DMARC_POLICY_ALLOW(-0.50)[disroot.org,quarantine]; NEURAL_HAM(-1.00)[-1.000]; ARC_SIGNED(0.00)[lists.ipfire.org:s=202003rsa:i=1]; FROM_EQ_ENVFROM(0.00)[]; MIME_TRACE(0.00)[0:+]; RCVD_TLS_LAST(0.00)[]; ASN(0.00)[asn:50673, ipnet:178.21.23.0/24, country:NL]; BAYES_HAM(-3.00)[99.99%] X-Rspamd-Queue-Id: 4FRfYD1lsBzBZ Authentication-Results: mail01.ipfire.org; dkim=pass header.d=disroot.org header.s=mail header.b=WjYuMdmA; dmarc=pass (policy=quarantine) header.from=disroot.org; spf=pass (mail01.ipfire.org: domain of robin.roevens@disroot.org designates 178.21.23.139 as permitted sender) smtp.mailfrom=robin.roevens@disroot.org X-BeenThere: development@lists.ipfire.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: IPFire development talk <development.lists.ipfire.org> List-Unsubscribe: <https://lists.ipfire.org/mailman/options/development>, <mailto:development-request@lists.ipfire.org?subject=unsubscribe> List-Archive: <http://lists.ipfire.org/pipermail/development/> List-Post: <mailto:development@lists.ipfire.org> List-Help: <mailto:development-request@lists.ipfire.org?subject=help> List-Subscribe: <https://lists.ipfire.org/mailman/listinfo/development>, <mailto:development-request@lists.ipfire.org?subject=subscribe> Errors-To: development-bounces@lists.ipfire.org Sender: "Development" <development-bounces@lists.ipfire.org> |
Series |
Pakfile metadata enhancements
|
|
Commit Message
Robin Roevens
April 23, 2021, 4:15 p.m. UTC
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@disroot.org>
---
src/pakfire/lib/functions.pl | 124 ++++++++++++++++++++++++++++++++++-
src/pakfire/pakfire | 21 ++++++
2 files changed, 144 insertions(+), 1 deletion(-)
Comments
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@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
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@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 >
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. 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. 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@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@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. >
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@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@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. > > > >
Hello, > On 14 May 2021, at 21:24, Robin Roevens <robin.roevens@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@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@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.
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; + } + } + } + + ### 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); + } else { + message("PAKFIRE WARN: Pak '$pak' not found."); + return 1; + } + + 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); + } + } + + ### 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) {