diff mbox series

Fix for bug 12539

Message ID 20201207140136.3423-1-ahb.ipfire@gmail.com
State Accepted
Commit d978558809285afcee56b2120b10d9107b1b260b
Headers
Series Fix for bug 12539 |

Commit Message

Adolf Belka Dec. 7, 2020, 2:01 p.m. UTC 
  The installer recognises cups and cups-filters both as cups and puts
two instances of cups in the add-on services table.
Based on input from Michael Tremer this patch replaces the command
returning the second element between hyphens with one that takes
what comes after "meta-" using Perl code rather than a shell command.
The second find command was changed as per Michael's suggestion.

Tested in my ipfire test bed system and only results in one cups
entry.
Signed-off-by: Adolf Belka <ahb.ipfire@gmail.com>
---
 html/cgi-bin/services.cgi | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Michael Tremer Dec. 10, 2020, 12:48 p.m. UTC | #1 
Hello,

Great work. This looks like how it should be.

> On 7 Dec 2020, at 15:01, Adolf Belka <ahb.ipfire@gmail.com> wrote:
> 
> The installer recognises cups and cups-filters both as cups and puts
> two instances of cups in the add-on services table.
> Based on input from Michael Tremer this patch replaces the command
> returning the second element between hyphens with one that takes
> what comes after "meta-" using Perl code rather than a shell command.
> The second find command was changed as per Michael's suggestion.
> 
> Tested in my ipfire test bed system and only results in one cups
> entry.
> Signed-off-by: Adolf Belka <ahb.ipfire@gmail.com>
> ---
> html/cgi-bin/services.cgi | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/html/cgi-bin/services.cgi b/html/cgi-bin/services.cgi
> index 26ab4f314..36954ba70 100644
> --- a/html/cgi-bin/services.cgi
> +++ b/html/cgi-bin/services.cgi
> @@ -161,19 +161,20 @@ END
> 	my $lines=0; # Used to count the outputlines to make different bgcolor
> 
> 	# Generate list of installed addon pak's
> -	my @pak = `find /opt/pakfire/db/installed/meta-* 2>/dev/null | cut -d"-" -f2`;
> +	opendir (DIR, "/opt/pakfire/db/installed") || die "Cannot opendir /opt/pakfire/db/installed/: $!";
> +	my @pak = sort readdir DIR;
> 	foreach (@pak){
> 		chomp($_);
> +		next unless (m/^meta-/);
> +		s/^meta-//;

Although this is the least intuitive thing to do. I have no idea who designed Perl. I hope they are proud of all the chaos they have created :)

-Michael

> 
> 		# Check which of the paks are services
> -		my @svc = `find /etc/init.d/$_ 2>/dev/null | cut -d"/" -f4`;
> -		foreach (@svc){
> +		if (-e "/etc/init.d/$_") {
> 			# blacklist some packages
> 			#
> 			# alsa has trouble with the volume saving and was not really stopped
> 			# mdadm should not stopped with webif because this could crash the system
> 			#
> -			chomp($_);
> 			if ( $_ eq 'squid' ) {
> 				next;
> 			}
> -- 
> 2.29.2
>
Bernhard Bitsch Dec. 10, 2020, 1:13 p.m. UTC | #2 
Hello,

> Gesendet: Donnerstag, 10. Dezember 2020 um 13:48 Uhr
> Von: "Michael Tremer" <michael.tremer@ipfire.org>
> An: "Adolf Belka" <ahb.ipfire@gmail.com>
> Cc: development@lists.ipfire.org
> Betreff: Re: [PATCH] Fix for bug 12539
>
> Hello,
>
> Great work. This looks like how it should be.
>
> > On 7 Dec 2020, at 15:01, Adolf Belka <ahb.ipfire@gmail.com> wrote:
> >
> > The installer recognises cups and cups-filters both as cups and puts
> > two instances of cups in the add-on services table.
> > Based on input from Michael Tremer this patch replaces the command
> > returning the second element between hyphens with one that takes
> > what comes after "meta-" using Perl code rather than a shell command.
> > The second find command was changed as per Michael's suggestion.
> >
> > Tested in my ipfire test bed system and only results in one cups
> > entry.
> > Signed-off-by: Adolf Belka <ahb.ipfire@gmail.com>
> > ---
> > html/cgi-bin/services.cgi | 9 +++++----
> > 1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/html/cgi-bin/services.cgi b/html/cgi-bin/services.cgi
> > index 26ab4f314..36954ba70 100644
> > --- a/html/cgi-bin/services.cgi
> > +++ b/html/cgi-bin/services.cgi
> > @@ -161,19 +161,20 @@ END
> > 	my $lines=0; # Used to count the outputlines to make different bgcolor
> >
> > 	# Generate list of installed addon pak's
> > -	my @pak = `find /opt/pakfire/db/installed/meta-* 2>/dev/null | cut -d"-" -f2`;
> > +	opendir (DIR, "/opt/pakfire/db/installed") || die "Cannot opendir /opt/pakfire/db/installed/: $!";
> > +	my @pak = sort readdir DIR;
> > 	foreach (@pak){
> > 		chomp($_);
> > +		next unless (m/^meta-/);
> > +		s/^meta-//;
>
> Although this is the least intuitive thing to do. I have no idea who designed Perl. I hope they are proud of all the chaos they have created :)
>
> -Michael

Can't see where the solution is 'least intuitive'. As far as I can see from the patch, now the directory of installed addons is read with Perl functions ( not in a extra forked process ). The definition of filenames is used consequently ( 'meta-<addon name>', possible services are stored in /etc/init.d/<addon name> ).

- Bernhard


>
> >
> > 		# Check which of the paks are services
> > -		my @svc = `find /etc/init.d/$_ 2>/dev/null | cut -d"/" -f4`;
> > -		foreach (@svc){
> > +		if (-e "/etc/init.d/$_") {
> > 			# blacklist some packages
> > 			#
> > 			# alsa has trouble with the volume saving and was not really stopped
> > 			# mdadm should not stopped with webif because this could crash the system
> > 			#
> > -			chomp($_);
> > 			if ( $_ eq 'squid' ) {
> > 				next;
> > 			}
> > --
> > 2.29.2
> >
>
>
Michael Tremer Dec. 10, 2020, 1:29 p.m. UTC | #3 
Hi,

> On 10 Dec 2020, at 14:13, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
> 
> Hello,
> 
>> Gesendet: Donnerstag, 10. Dezember 2020 um 13:48 Uhr
>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
>> An: "Adolf Belka" <ahb.ipfire@gmail.com>
>> Cc: development@lists.ipfire.org
>> Betreff: Re: [PATCH] Fix for bug 12539
>> 
>> Hello,
>> 
>> Great work. This looks like how it should be.
>> 
>>> On 7 Dec 2020, at 15:01, Adolf Belka <ahb.ipfire@gmail.com> wrote:
>>> 
>>> The installer recognises cups and cups-filters both as cups and puts
>>> two instances of cups in the add-on services table.
>>> Based on input from Michael Tremer this patch replaces the command
>>> returning the second element between hyphens with one that takes
>>> what comes after "meta-" using Perl code rather than a shell command.
>>> The second find command was changed as per Michael's suggestion.
>>> 
>>> Tested in my ipfire test bed system and only results in one cups
>>> entry.
>>> Signed-off-by: Adolf Belka <ahb.ipfire@gmail.com>
>>> ---
>>> html/cgi-bin/services.cgi | 9 +++++----
>>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/html/cgi-bin/services.cgi b/html/cgi-bin/services.cgi
>>> index 26ab4f314..36954ba70 100644
>>> --- a/html/cgi-bin/services.cgi
>>> +++ b/html/cgi-bin/services.cgi
>>> @@ -161,19 +161,20 @@ END
>>> 	my $lines=0; # Used to count the outputlines to make different bgcolor
>>> 
>>> 	# Generate list of installed addon pak's
>>> -	my @pak = `find /opt/pakfire/db/installed/meta-* 2>/dev/null | cut -d"-" -f2`;
>>> +	opendir (DIR, "/opt/pakfire/db/installed") || die "Cannot opendir /opt/pakfire/db/installed/: $!";
>>> +	my @pak = sort readdir DIR;
>>> 	foreach (@pak){
>>> 		chomp($_);
>>> +		next unless (m/^meta-/);
>>> +		s/^meta-//;
>> 
>> Although this is the least intuitive thing to do. I have no idea who designed Perl. I hope they are proud of all the chaos they have created :)
>> 
>> -Michael
> 
> Can't see where the solution is 'least intuitive'. As far as I can see from the patch, now the directory of installed addons is read with Perl functions ( not in a extra forked process ). The definition of filenames is used consequently ( 'meta-<addon name>', possible services are stored in /etc/init.d/<addon name> ).

To be clear, I am complaining about the Perl language design instead of the code. In my view something like

  name = name.replace(“meta-“, “”)

is a thousand times easier to read.

Best,
-Michael

> 
> - Bernhard
> 
> 
>> 
>>> 
>>> 		# Check which of the paks are services
>>> -		my @svc = `find /etc/init.d/$_ 2>/dev/null | cut -d"/" -f4`;
>>> -		foreach (@svc){
>>> +		if (-e "/etc/init.d/$_") {
>>> 			# blacklist some packages
>>> 			#
>>> 			# alsa has trouble with the volume saving and was not really stopped
>>> 			# mdadm should not stopped with webif because this could crash the system
>>> 			#
>>> -			chomp($_);
>>> 			if ( $_ eq 'squid' ) {
>>> 				next;
>>> 			}
>>> --
>>> 2.29.2
Michael Tremer Dec. 10, 2020, 2:48 p.m. UTC | #4 
Hello,

> On 10 Dec 2020, at 15:35, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
> 
> Hi,
> 
>> Gesendet: Donnerstag, 10. Dezember 2020 um 14:29 Uhr
>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
>> An: "Bernhard Bitsch" <Bernhard.Bitsch@gmx.de>
>> Cc: "Adolf Belka" <ahb.ipfire@gmail.com>, development@lists.ipfire.org
>> Betreff: Re: [PATCH] Fix for bug 12539
>> 
>> Hi,
>> 
>>> On 10 Dec 2020, at 14:13, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
>>> 
>>> Hello,
>>> 
>>>> Gesendet: Donnerstag, 10. Dezember 2020 um 13:48 Uhr
>>>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
>>>> An: "Adolf Belka" <ahb.ipfire@gmail.com>
>>>> Cc: development@lists.ipfire.org
>>>> Betreff: Re: [PATCH] Fix for bug 12539
>>>> 
>>>> Hello,
>>>> 
>>>> Great work. This looks like how it should be.
>>>> 
>>>>> On 7 Dec 2020, at 15:01, Adolf Belka <ahb.ipfire@gmail.com> wrote:
>>>>> 
>>>>> The installer recognises cups and cups-filters both as cups and puts
>>>>> two instances of cups in the add-on services table.
>>>>> Based on input from Michael Tremer this patch replaces the command
>>>>> returning the second element between hyphens with one that takes
>>>>> what comes after "meta-" using Perl code rather than a shell command.
>>>>> The second find command was changed as per Michael's suggestion.
>>>>> 
>>>>> Tested in my ipfire test bed system and only results in one cups
>>>>> entry.
>>>>> Signed-off-by: Adolf Belka <ahb.ipfire@gmail.com>
>>>>> ---
>>>>> html/cgi-bin/services.cgi | 9 +++++----
>>>>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>>>> 
>>>>> diff --git a/html/cgi-bin/services.cgi b/html/cgi-bin/services.cgi
>>>>> index 26ab4f314..36954ba70 100644
>>>>> --- a/html/cgi-bin/services.cgi
>>>>> +++ b/html/cgi-bin/services.cgi
>>>>> @@ -161,19 +161,20 @@ END
>>>>> 	my $lines=0; # Used to count the outputlines to make different bgcolor
>>>>> 
>>>>> 	# Generate list of installed addon pak's
>>>>> -	my @pak = `find /opt/pakfire/db/installed/meta-* 2>/dev/null | cut -d"-" -f2`;
>>>>> +	opendir (DIR, "/opt/pakfire/db/installed") || die "Cannot opendir /opt/pakfire/db/installed/: $!";
>>>>> +	my @pak = sort readdir DIR;
>>>>> 	foreach (@pak){
>>>>> 		chomp($_);
>>>>> +		next unless (m/^meta-/);
>>>>> +		s/^meta-//;
>>>> 
>>>> Although this is the least intuitive thing to do. I have no idea who designed Perl. I hope they are proud of all the chaos they have created :)
>>>> 
>>>> -Michael
>>> 
>>> Can't see where the solution is 'least intuitive'. As far as I can see from the patch, now the directory of installed addons is read with Perl functions ( not in a extra forked process ). The definition of filenames is used consequently ( 'meta-<addon name>', possible services are stored in /etc/init.d/<addon name> ).
>> 
>> To be clear, I am complaining about the Perl language design instead of the code. In my view something like
>> 
>>  name = name.replace(“meta-“, “”)
>> 
>> is a thousand times easier to read.
>> 
>> Best,
>> -Michael
>> 
> 
> Okay, that makes it clearer. 
> Perl has its roots in pattern matching. So the base functions of matching and replacing are defined with minimal overhead in writing.
> Maybe the designer(s) of Perl had in mind the description of COBOL as "speaking english with your computer". This was in the beginnings of computing, which did not experience all of us. ;)
> With this single example you are right. Your formulation is much more readable ( besides the fact that name is a string object ). But if you have many such snippets in your code, the Perl way makes a shorter and thus more understandable program.

I disagree. Why would the usage of something that is hard to comprehend be better when it is being used more often? Getting used to the wrong pattern doesn’t make it right.

My main complaint about this is, that it is not clear what is being manipulated. It is better to use the variable name and then there is no confusion about it. The same is true for loads of other Perl-isms like shift, chomp, chop, etc.

Shorter code does not necessarily mean that it executes faster. Probably the opposite is true in this case, because you would have to compile a regular expression first and the Python version would simply perform a string search.

The point is, that code should be obvious. It avoids bugs and it makes it easier to understand for reviewers what is supposed to go on.

Best,
-Michael

> 
> Best,
> Bernhard
> 
>>> 
>>> - Bernhard
>>> 
>>> 
>>>> 
>>>>> 
>>>>> 		# Check which of the paks are services
>>>>> -		my @svc = `find /etc/init.d/$_ 2>/dev/null | cut -d"/" -f4`;
>>>>> -		foreach (@svc){
>>>>> +		if (-e "/etc/init.d/$_") {
>>>>> 			# blacklist some packages
>>>>> 			#
>>>>> 			# alsa has trouble with the volume saving and was not really stopped
>>>>> 			# mdadm should not stopped with webif because this could crash the system
>>>>> 			#
>>>>> -			chomp($_);
>>>>> 			if ( $_ eq 'squid' ) {
>>>>> 				next;
>>>>> 			}
>>>>> --
>>>>> 2.29.2
Bernhard Bitsch Dec. 10, 2020, 3:26 p.m. UTC | #5 
> Gesendet: Donnerstag, 10. Dezember 2020 um 15:48 Uhr
> Von: "Michael Tremer" <michael.tremer@ipfire.org>
> An: "Bernhard Bitsch" <Bernhard.Bitsch@gmx.de>
> Cc: "IPFire: Development-List" <development@lists.ipfire.org>
> Betreff: Re: [PATCH] Fix for bug 12539
>
> Hello,
> 
> > On 10 Dec 2020, at 15:35, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
> > 
> > Hi,
> > 
> >> Gesendet: Donnerstag, 10. Dezember 2020 um 14:29 Uhr
> >> Von: "Michael Tremer" <michael.tremer@ipfire.org>
> >> An: "Bernhard Bitsch" <Bernhard.Bitsch@gmx.de>
> >> Cc: "Adolf Belka" <ahb.ipfire@gmail.com>, development@lists.ipfire.org
> >> Betreff: Re: [PATCH] Fix for bug 12539
> >> 
> >> Hi,
> >> 
> >>> On 10 Dec 2020, at 14:13, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
> >>> 
> >>> Hello,
> >>> 
> >>>> Gesendet: Donnerstag, 10. Dezember 2020 um 13:48 Uhr
> >>>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
> >>>> An: "Adolf Belka" <ahb.ipfire@gmail.com>
> >>>> Cc: development@lists.ipfire.org
> >>>> Betreff: Re: [PATCH] Fix for bug 12539
> >>>> 
> >>>> Hello,
> >>>> 
> >>>> Great work. This looks like how it should be.
> >>>> 
> >>>>> On 7 Dec 2020, at 15:01, Adolf Belka <ahb.ipfire@gmail.com> wrote:
> >>>>> 
> >>>>> The installer recognises cups and cups-filters both as cups and puts
> >>>>> two instances of cups in the add-on services table.
> >>>>> Based on input from Michael Tremer this patch replaces the command
> >>>>> returning the second element between hyphens with one that takes
> >>>>> what comes after "meta-" using Perl code rather than a shell command.
> >>>>> The second find command was changed as per Michael's suggestion.
> >>>>> 
> >>>>> Tested in my ipfire test bed system and only results in one cups
> >>>>> entry.
> >>>>> Signed-off-by: Adolf Belka <ahb.ipfire@gmail.com>
> >>>>> ---
> >>>>> html/cgi-bin/services.cgi | 9 +++++----
> >>>>> 1 file changed, 5 insertions(+), 4 deletions(-)
> >>>>> 
> >>>>> diff --git a/html/cgi-bin/services.cgi b/html/cgi-bin/services.cgi
> >>>>> index 26ab4f314..36954ba70 100644
> >>>>> --- a/html/cgi-bin/services.cgi
> >>>>> +++ b/html/cgi-bin/services.cgi
> >>>>> @@ -161,19 +161,20 @@ END
> >>>>> 	my $lines=0; # Used to count the outputlines to make different bgcolor
> >>>>> 
> >>>>> 	# Generate list of installed addon pak's
> >>>>> -	my @pak = `find /opt/pakfire/db/installed/meta-* 2>/dev/null | cut -d"-" -f2`;
> >>>>> +	opendir (DIR, "/opt/pakfire/db/installed") || die "Cannot opendir /opt/pakfire/db/installed/: $!";
> >>>>> +	my @pak = sort readdir DIR;
> >>>>> 	foreach (@pak){
> >>>>> 		chomp($_);
> >>>>> +		next unless (m/^meta-/);
> >>>>> +		s/^meta-//;
> >>>> 
> >>>> Although this is the least intuitive thing to do. I have no idea who designed Perl. I hope they are proud of all the chaos they have created :)
> >>>> 
> >>>> -Michael
> >>> 
> >>> Can't see where the solution is 'least intuitive'. As far as I can see from the patch, now the directory of installed addons is read with Perl functions ( not in a extra forked process ). The definition of filenames is used consequently ( 'meta-<addon name>', possible services are stored in /etc/init.d/<addon name> ).
> >> 
> >> To be clear, I am complaining about the Perl language design instead of the code. In my view something like
> >> 
> >>  name = name.replace(“meta-“, “”)
> >> 
> >> is a thousand times easier to read.
> >> 
> >> Best,
> >> -Michael
> >> 
> > 
> > Okay, that makes it clearer. 
> > Perl has its roots in pattern matching. So the base functions of matching and replacing are defined with minimal overhead in writing.
> > Maybe the designer(s) of Perl had in mind the description of COBOL as "speaking english with your computer". This was in the beginnings of computing, which did not experience all of us. ;)
> > With this single example you are right. Your formulation is much more readable ( besides the fact that name is a string object ). But if you have many such snippets in your code, the Perl way makes a shorter and thus more understandable program.
> 
> I disagree. Why would the usage of something that is hard to comprehend be better when it is being used more often? Getting used to the wrong pattern doesn’t make it right.
> 
> My main complaint about this is, that it is not clear what is being manipulated. It is better to use the variable name and then there is no confusion about it. The same is true for loads of other Perl-isms like shift, chomp, chop, etc.
> 
> Shorter code does not necessarily mean that it executes faster. Probably the opposite is true in this case, because you would have to compile a regular expression first and the Python version would simply perform a string search.
> 
> The point is, that code should be obvious. It avoids bugs and it makes it easier to understand for reviewers what is supposed to go on.
> 

The main difference between the Perl and the Python approach is the basic language class. Python works on objects and with their methods ( functions ), Perl works on plain variables with operators. Types are determined by the operator and context. Each operator has also a result which isn't destroyed by a assigment or statement end. This result is hold in a special variable which can be manipulated directly. 
If I review code, I should know this basics. Thus it doesn't make a real difference in comprehension ( to me ) if I read a object-oriented Python code or a Perl code. Nevertheless is the Perl code shorter. In our special example Perl avoids the generation (in written code ) of an intermeditiate variable by use of the implicit $_.
But this is a case of habit. Some like to talk in English only, some like to talk in their native language to be more exact. ;)

Best,
Bernhard
> Best,
> -Michael
> 
> > 
> > Best,
> > Bernhard
> > 
> >>> 
> >>> - Bernhard
> >>> 
> >>> 
> >>>> 
> >>>>> 
> >>>>> 		# Check which of the paks are services
> >>>>> -		my @svc = `find /etc/init.d/$_ 2>/dev/null | cut -d"/" -f4`;
> >>>>> -		foreach (@svc){
> >>>>> +		if (-e "/etc/init.d/$_") {
> >>>>> 			# blacklist some packages
> >>>>> 			#
> >>>>> 			# alsa has trouble with the volume saving and was not really stopped
> >>>>> 			# mdadm should not stopped with webif because this could crash the system
> >>>>> 			#
> >>>>> -			chomp($_);
> >>>>> 			if ( $_ eq 'squid' ) {
> >>>>> 				next;
> >>>>> 			}
> >>>>> --
> >>>>> 2.29.2
> 
>
Michael Tremer Dec. 10, 2020, 3:46 p.m. UTC | #6 
Hi,

> On 10 Dec 2020, at 16:26, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
> 
> 
> 
>> Gesendet: Donnerstag, 10. Dezember 2020 um 15:48 Uhr
>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
>> An: "Bernhard Bitsch" <Bernhard.Bitsch@gmx.de>
>> Cc: "IPFire: Development-List" <development@lists.ipfire.org>
>> Betreff: Re: [PATCH] Fix for bug 12539
>> 
>> Hello,
>> 
>>> On 10 Dec 2020, at 15:35, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
>>> 
>>> Hi,
>>> 
>>>> Gesendet: Donnerstag, 10. Dezember 2020 um 14:29 Uhr
>>>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
>>>> An: "Bernhard Bitsch" <Bernhard.Bitsch@gmx.de>
>>>> Cc: "Adolf Belka" <ahb.ipfire@gmail.com>, development@lists.ipfire.org
>>>> Betreff: Re: [PATCH] Fix for bug 12539
>>>> 
>>>> Hi,
>>>> 
>>>>> On 10 Dec 2020, at 14:13, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
>>>>> 
>>>>> Hello,
>>>>> 
>>>>>> Gesendet: Donnerstag, 10. Dezember 2020 um 13:48 Uhr
>>>>>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
>>>>>> An: "Adolf Belka" <ahb.ipfire@gmail.com>
>>>>>> Cc: development@lists.ipfire.org
>>>>>> Betreff: Re: [PATCH] Fix for bug 12539
>>>>>> 
>>>>>> Hello,
>>>>>> 
>>>>>> Great work. This looks like how it should be.
>>>>>> 
>>>>>>> On 7 Dec 2020, at 15:01, Adolf Belka <ahb.ipfire@gmail.com> wrote:
>>>>>>> 
>>>>>>> The installer recognises cups and cups-filters both as cups and puts
>>>>>>> two instances of cups in the add-on services table.
>>>>>>> Based on input from Michael Tremer this patch replaces the command
>>>>>>> returning the second element between hyphens with one that takes
>>>>>>> what comes after "meta-" using Perl code rather than a shell command.
>>>>>>> The second find command was changed as per Michael's suggestion.
>>>>>>> 
>>>>>>> Tested in my ipfire test bed system and only results in one cups
>>>>>>> entry.
>>>>>>> Signed-off-by: Adolf Belka <ahb.ipfire@gmail.com>
>>>>>>> ---
>>>>>>> html/cgi-bin/services.cgi | 9 +++++----
>>>>>>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>>>>>> 
>>>>>>> diff --git a/html/cgi-bin/services.cgi b/html/cgi-bin/services.cgi
>>>>>>> index 26ab4f314..36954ba70 100644
>>>>>>> --- a/html/cgi-bin/services.cgi
>>>>>>> +++ b/html/cgi-bin/services.cgi
>>>>>>> @@ -161,19 +161,20 @@ END
>>>>>>> 	my $lines=0; # Used to count the outputlines to make different bgcolor
>>>>>>> 
>>>>>>> 	# Generate list of installed addon pak's
>>>>>>> -	my @pak = `find /opt/pakfire/db/installed/meta-* 2>/dev/null | cut -d"-" -f2`;
>>>>>>> +	opendir (DIR, "/opt/pakfire/db/installed") || die "Cannot opendir /opt/pakfire/db/installed/: $!";
>>>>>>> +	my @pak = sort readdir DIR;
>>>>>>> 	foreach (@pak){
>>>>>>> 		chomp($_);
>>>>>>> +		next unless (m/^meta-/);
>>>>>>> +		s/^meta-//;
>>>>>> 
>>>>>> Although this is the least intuitive thing to do. I have no idea who designed Perl. I hope they are proud of all the chaos they have created :)
>>>>>> 
>>>>>> -Michael
>>>>> 
>>>>> Can't see where the solution is 'least intuitive'. As far as I can see from the patch, now the directory of installed addons is read with Perl functions ( not in a extra forked process ). The definition of filenames is used consequently ( 'meta-<addon name>', possible services are stored in /etc/init.d/<addon name> ).
>>>> 
>>>> To be clear, I am complaining about the Perl language design instead of the code. In my view something like
>>>> 
>>>> name = name.replace(“meta-“, “”)
>>>> 
>>>> is a thousand times easier to read.
>>>> 
>>>> Best,
>>>> -Michael
>>>> 
>>> 
>>> Okay, that makes it clearer. 
>>> Perl has its roots in pattern matching. So the base functions of matching and replacing are defined with minimal overhead in writing.
>>> Maybe the designer(s) of Perl had in mind the description of COBOL as "speaking english with your computer". This was in the beginnings of computing, which did not experience all of us. ;)
>>> With this single example you are right. Your formulation is much more readable ( besides the fact that name is a string object ). But if you have many such snippets in your code, the Perl way makes a shorter and thus more understandable program.
>> 
>> I disagree. Why would the usage of something that is hard to comprehend be better when it is being used more often? Getting used to the wrong pattern doesn’t make it right.
>> 
>> My main complaint about this is, that it is not clear what is being manipulated. It is better to use the variable name and then there is no confusion about it. The same is true for loads of other Perl-isms like shift, chomp, chop, etc.
>> 
>> Shorter code does not necessarily mean that it executes faster. Probably the opposite is true in this case, because you would have to compile a regular expression first and the Python version would simply perform a string search.
>> 
>> The point is, that code should be obvious. It avoids bugs and it makes it easier to understand for reviewers what is supposed to go on.
>> 
> 
> The main difference between the Perl and the Python approach is the basic language class. Python works on objects and with their methods ( functions ), Perl works on plain variables with operators. Types are determined by the operator and context. Each operator has also a result which isn't destroyed by a assigment or statement end. This result is hold in a special variable which can be manipulated directly. 
> If I review code, I should know this basics. Thus it doesn't make a real difference in comprehension ( to me ) if I read a object-oriented Python code or a Perl code. Nevertheless is the Perl code shorter. In our special example Perl avoids the generation (in written code ) of an intermeditiate variable by use of the implicit $_.
> But this is a case of habit. Some like to talk in English only, some like to talk in their native language to be more exact. ;)

Yes, I understand that. But what do you do when you have more than one variable? Most of my code does.

> Best,
> Bernhard
>> Best,
>> -Michael
>> 
>>> 
>>> Best,
>>> Bernhard
>>> 
>>>>> 
>>>>> - Bernhard
>>>>> 
>>>>> 
>>>>>> 
>>>>>>> 
>>>>>>> 		# Check which of the paks are services
>>>>>>> -		my @svc = `find /etc/init.d/$_ 2>/dev/null | cut -d"/" -f4`;
>>>>>>> -		foreach (@svc){
>>>>>>> +		if (-e "/etc/init.d/$_") {
>>>>>>> 			# blacklist some packages
>>>>>>> 			#
>>>>>>> 			# alsa has trouble with the volume saving and was not really stopped
>>>>>>> 			# mdadm should not stopped with webif because this could crash the system
>>>>>>> 			#
>>>>>>> -			chomp($_);
>>>>>>> 			if ( $_ eq 'squid' ) {
>>>>>>> 				next;
>>>>>>> 			}
>>>>>>> --
>>>>>>> 2.29.2
Adolf Belka Dec. 10, 2020, 4:45 p.m. UTC | #7 
Hi,


On 10/12/2020 16:46, Michael Tremer wrote:
> Hi,
> 
>> On 10 Dec 2020, at 16:26, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
>>
>>
>>
>>> Gesendet: Donnerstag, 10. Dezember 2020 um 15:48 Uhr
>>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
>>> An: "Bernhard Bitsch" <Bernhard.Bitsch@gmx.de>
>>> Cc: "IPFire: Development-List" <development@lists.ipfire.org>
>>> Betreff: Re: [PATCH] Fix for bug 12539
>>>
>>> Hello,
>>>
>>>> On 10 Dec 2020, at 15:35, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
>>>>
>>>> Hi,
>>>>
>>>>> Gesendet: Donnerstag, 10. Dezember 2020 um 14:29 Uhr
>>>>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
>>>>> An: "Bernhard Bitsch" <Bernhard.Bitsch@gmx.de>
>>>>> Cc: "Adolf Belka" <ahb.ipfire@gmail.com>, development@lists.ipfire.org
>>>>> Betreff: Re: [PATCH] Fix for bug 12539
>>>>>
>>>>> Hi,
>>>>>
>>>>>> On 10 Dec 2020, at 14:13, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>>> Gesendet: Donnerstag, 10. Dezember 2020 um 13:48 Uhr
>>>>>>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
>>>>>>> An: "Adolf Belka" <ahb.ipfire@gmail.com>
>>>>>>> Cc: development@lists.ipfire.org
>>>>>>> Betreff: Re: [PATCH] Fix for bug 12539
>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> Great work. This looks like how it should be.
>>>>>>>
>>>>>>>> On 7 Dec 2020, at 15:01, Adolf Belka <ahb.ipfire@gmail.com> wrote:
>>>>>>>>
>>>>>>>> The installer recognises cups and cups-filters both as cups and puts
>>>>>>>> two instances of cups in the add-on services table.
>>>>>>>> Based on input from Michael Tremer this patch replaces the command
>>>>>>>> returning the second element between hyphens with one that takes
>>>>>>>> what comes after "meta-" using Perl code rather than a shell command.
>>>>>>>> The second find command was changed as per Michael's suggestion.
>>>>>>>>
>>>>>>>> Tested in my ipfire test bed system and only results in one cups
>>>>>>>> entry.
>>>>>>>> Signed-off-by: Adolf Belka <ahb.ipfire@gmail.com>
>>>>>>>> ---
>>>>>>>> html/cgi-bin/services.cgi | 9 +++++----
>>>>>>>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/html/cgi-bin/services.cgi b/html/cgi-bin/services.cgi
>>>>>>>> index 26ab4f314..36954ba70 100644
>>>>>>>> --- a/html/cgi-bin/services.cgi
>>>>>>>> +++ b/html/cgi-bin/services.cgi
>>>>>>>> @@ -161,19 +161,20 @@ END
>>>>>>>> 	my $lines=0; # Used to count the outputlines to make different bgcolor
>>>>>>>>
>>>>>>>> 	# Generate list of installed addon pak's
>>>>>>>> -	my @pak = `find /opt/pakfire/db/installed/meta-* 2>/dev/null | cut -d"-" -f2`;
>>>>>>>> +	opendir (DIR, "/opt/pakfire/db/installed") || die "Cannot opendir /opt/pakfire/db/installed/: $!";
>>>>>>>> +	my @pak = sort readdir DIR;
>>>>>>>> 	foreach (@pak){
>>>>>>>> 		chomp($_);
>>>>>>>> +		next unless (m/^meta-/);
>>>>>>>> +		s/^meta-//;
>>>>>>>
>>>>>>> Although this is the least intuitive thing to do. I have no idea who designed Perl. I hope they are proud of all the chaos they have created :)
>>>>>>>
>>>>>>> -Michael
>>>>>>
>>>>>> Can't see where the solution is 'least intuitive'. As far as I can see from the patch, now the directory of installed addons is read with Perl functions ( not in a extra forked process ). The definition of filenames is used consequently ( 'meta-<addon name>', possible services are stored in /etc/init.d/<addon name> ).
>>>>>
>>>>> To be clear, I am complaining about the Perl language design instead of the code. In my view something like
>>>>>
>>>>> name = name.replace(“meta-“, “”)
>>>>>
>>>>> is a thousand times easier to read.
>>>>>
>>>>> Best,
>>>>> -Michael
>>>>>
>>>>
>>>> Okay, that makes it clearer.
>>>> Perl has its roots in pattern matching. So the base functions of matching and replacing are defined with minimal overhead in writing.
>>>> Maybe the designer(s) of Perl had in mind the description of COBOL as "speaking english with your computer". This was in the beginnings of computing, which did not experience all of us. ;)
>>>> With this single example you are right. Your formulation is much more readable ( besides the fact that name is a string object ). But if you have many such snippets in your code, the Perl way makes a shorter and thus more understandable program.
>>>
>>> I disagree. Why would the usage of something that is hard to comprehend be better when it is being used more often? Getting used to the wrong pattern doesn’t make it right.
>>>
>>> My main complaint about this is, that it is not clear what is being manipulated. It is better to use the variable name and then there is no confusion about it. The same is true for loads of other Perl-isms like shift, chomp, chop, etc.
>>>
>>> Shorter code does not necessarily mean that it executes faster. Probably the opposite is true in this case, because you would have to compile a regular expression first and the Python version would simply perform a string search.
>>>
>>> The point is, that code should be obvious. It avoids bugs and it makes it easier to understand for reviewers what is supposed to go on.
>>>
>>
>> The main difference between the Perl and the Python approach is the basic language class. Python works on objects and with their methods ( functions ), Perl works on plain variables with operators. Types are determined by the operator and context. Each operator has also a result which isn't destroyed by a assigment or statement end. This result is hold in a special variable which can be manipulated directly.
>> If I review code, I should know this basics. Thus it doesn't make a real difference in comprehension ( to me ) if I read a object-oriented Python code or a Perl code. Nevertheless is the Perl code shorter. In our special example Perl avoids the generation (in written code ) of an intermeditiate variable by use of the implicit $_.
>> But this is a case of habit. Some like to talk in English only, some like to talk in their native language to be more exact. ;)
> 
> Yes, I understand that. But what do you do when you have more than one variable? Most of my code does.
> 

My Perl learning here included the existence of the implicit $_. I let it in place because the section covered by foreach (@pak) was using it.
However if the preference is to be explicit with variables, I have no problem in doing that in any future bug fixes etc that I do.

Regards,
Adolf.

>> Best,
>> Bernhard
>>> Best,
>>> -Michael
>>>
>>>>
>>>> Best,
>>>> Bernhard
>>>>
>>>>>>
>>>>>> - Bernhard
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> 		# Check which of the paks are services
>>>>>>>> -		my @svc = `find /etc/init.d/$_ 2>/dev/null | cut -d"/" -f4`;
>>>>>>>> -		foreach (@svc){
>>>>>>>> +		if (-e "/etc/init.d/$_") {
>>>>>>>> 			# blacklist some packages
>>>>>>>> 			#
>>>>>>>> 			# alsa has trouble with the volume saving and was not really stopped
>>>>>>>> 			# mdadm should not stopped with webif because this could crash the system
>>>>>>>> 			#
>>>>>>>> -			chomp($_);
>>>>>>>> 			if ( $_ eq 'squid' ) {
>>>>>>>> 				next;
>>>>>>>> 			}
>>>>>>>> --
>>>>>>>> 2.29.2
>
Bernhard Bitsch Dec. 10, 2020, 6:49 p.m. UTC | #8 
Hi,

> Gesendet: Donnerstag, 10. Dezember 2020 um 17:45 Uhr
> Von: "Adolf Belka" <ahb.ipfire@gmail.com>
> An: "Michael Tremer" <michael.tremer@ipfire.org>, "Bernhard Bitsch" <Bernhard.Bitsch@gmx.de>
> Cc: "IPFire: Development-List" <development@lists.ipfire.org>
> Betreff: Re: [PATCH] Fix for bug 12539
>
> Hi,
> 
> 
> On 10/12/2020 16:46, Michael Tremer wrote:
> > Hi,
> > 
> >> On 10 Dec 2020, at 16:26, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
> >>
> >>
> >>
> >>> Gesendet: Donnerstag, 10. Dezember 2020 um 15:48 Uhr
> >>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
> >>> An: "Bernhard Bitsch" <Bernhard.Bitsch@gmx.de>
> >>> Cc: "IPFire: Development-List" <development@lists.ipfire.org>
> >>> Betreff: Re: [PATCH] Fix for bug 12539
> >>>
> >>> Hello,
> >>>
> >>>> On 10 Dec 2020, at 15:35, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>>> Gesendet: Donnerstag, 10. Dezember 2020 um 14:29 Uhr
> >>>>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
> >>>>> An: "Bernhard Bitsch" <Bernhard.Bitsch@gmx.de>
> >>>>> Cc: "Adolf Belka" <ahb.ipfire@gmail.com>, development@lists.ipfire.org
> >>>>> Betreff: Re: [PATCH] Fix for bug 12539
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>>> On 10 Dec 2020, at 14:13, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
> >>>>>>
> >>>>>> Hello,
> >>>>>>
> >>>>>>> Gesendet: Donnerstag, 10. Dezember 2020 um 13:48 Uhr
> >>>>>>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
> >>>>>>> An: "Adolf Belka" <ahb.ipfire@gmail.com>
> >>>>>>> Cc: development@lists.ipfire.org
> >>>>>>> Betreff: Re: [PATCH] Fix for bug 12539
> >>>>>>>
> >>>>>>> Hello,
> >>>>>>>
> >>>>>>> Great work. This looks like how it should be.
> >>>>>>>
> >>>>>>>> On 7 Dec 2020, at 15:01, Adolf Belka <ahb.ipfire@gmail.com> wrote:
> >>>>>>>>
> >>>>>>>> The installer recognises cups and cups-filters both as cups and puts
> >>>>>>>> two instances of cups in the add-on services table.
> >>>>>>>> Based on input from Michael Tremer this patch replaces the command
> >>>>>>>> returning the second element between hyphens with one that takes
> >>>>>>>> what comes after "meta-" using Perl code rather than a shell command.
> >>>>>>>> The second find command was changed as per Michael's suggestion.
> >>>>>>>>
> >>>>>>>> Tested in my ipfire test bed system and only results in one cups
> >>>>>>>> entry.
> >>>>>>>> Signed-off-by: Adolf Belka <ahb.ipfire@gmail.com>
> >>>>>>>> ---
> >>>>>>>> html/cgi-bin/services.cgi | 9 +++++----
> >>>>>>>> 1 file changed, 5 insertions(+), 4 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/html/cgi-bin/services.cgi b/html/cgi-bin/services.cgi
> >>>>>>>> index 26ab4f314..36954ba70 100644
> >>>>>>>> --- a/html/cgi-bin/services.cgi
> >>>>>>>> +++ b/html/cgi-bin/services.cgi
> >>>>>>>> @@ -161,19 +161,20 @@ END
> >>>>>>>> 	my $lines=0; # Used to count the outputlines to make different bgcolor
> >>>>>>>>
> >>>>>>>> 	# Generate list of installed addon pak's
> >>>>>>>> -	my @pak = `find /opt/pakfire/db/installed/meta-* 2>/dev/null | cut -d"-" -f2`;
> >>>>>>>> +	opendir (DIR, "/opt/pakfire/db/installed") || die "Cannot opendir /opt/pakfire/db/installed/: $!";
> >>>>>>>> +	my @pak = sort readdir DIR;
> >>>>>>>> 	foreach (@pak){
> >>>>>>>> 		chomp($_);
> >>>>>>>> +		next unless (m/^meta-/);
> >>>>>>>> +		s/^meta-//;
> >>>>>>>
> >>>>>>> Although this is the least intuitive thing to do. I have no idea who designed Perl. I hope they are proud of all the chaos they have created :)
> >>>>>>>
> >>>>>>> -Michael
> >>>>>>
> >>>>>> Can't see where the solution is 'least intuitive'. As far as I can see from the patch, now the directory of installed addons is read with Perl functions ( not in a extra forked process ). The definition of filenames is used consequently ( 'meta-<addon name>', possible services are stored in /etc/init.d/<addon name> ).
> >>>>>
> >>>>> To be clear, I am complaining about the Perl language design instead of the code. In my view something like
> >>>>>
> >>>>> name = name.replace(“meta-“, “”)
> >>>>>
> >>>>> is a thousand times easier to read.
> >>>>>
> >>>>> Best,
> >>>>> -Michael
> >>>>>
> >>>>
> >>>> Okay, that makes it clearer.
> >>>> Perl has its roots in pattern matching. So the base functions of matching and replacing are defined with minimal overhead in writing.
> >>>> Maybe the designer(s) of Perl had in mind the description of COBOL as "speaking english with your computer". This was in the beginnings of computing, which did not experience all of us. ;)
> >>>> With this single example you are right. Your formulation is much more readable ( besides the fact that name is a string object ). But if you have many such snippets in your code, the Perl way makes a shorter and thus more understandable program.
> >>>
> >>> I disagree. Why would the usage of something that is hard to comprehend be better when it is being used more often? Getting used to the wrong pattern doesn’t make it right.
> >>>
> >>> My main complaint about this is, that it is not clear what is being manipulated. It is better to use the variable name and then there is no confusion about it. The same is true for loads of other Perl-isms like shift, chomp, chop, etc.
> >>>
> >>> Shorter code does not necessarily mean that it executes faster. Probably the opposite is true in this case, because you would have to compile a regular expression first and the Python version would simply perform a string search.
> >>>
> >>> The point is, that code should be obvious. It avoids bugs and it makes it easier to understand for reviewers what is supposed to go on.
> >>>
> >>
> >> The main difference between the Perl and the Python approach is the basic language class. Python works on objects and with their methods ( functions ), Perl works on plain variables with operators. Types are determined by the operator and context. Each operator has also a result which isn't destroyed by a assigment or statement end. This result is hold in a special variable which can be manipulated directly.
> >> If I review code, I should know this basics. Thus it doesn't make a real difference in comprehension ( to me ) if I read a object-oriented Python code or a Perl code. Nevertheless is the Perl code shorter. In our special example Perl avoids the generation (in written code ) of an intermeditiate variable by use of the implicit $_.
> >> But this is a case of habit. Some like to talk in English only, some like to talk in their native language to be more exact. ;)
> > 
> > Yes, I understand that. But what do you do when you have more than one variable? Most of my code does.
> > 
> 
> My Perl learning here included the existence of the implicit $_. I let it in place because the section covered by foreach (@pak) was using it.
> However if the preference is to be explicit with variables, I have no problem in doing that in any future bug fixes etc that I do.
> 

I think it is necessary in this situations, to introduce an extra variable. The 'foreach (@pak)' construction is defined as traversing the list @pak and delivering the elements in $_. That is sufficient for such operations. 

Best,
Bernhard

> Regards,
> Adolf.
> 
> >> Best,
> >> Bernhard
> >>> Best,
> >>> -Michael
> >>>
> >>>>
> >>>> Best,
> >>>> Bernhard
> >>>>
> >>>>>>
> >>>>>> - Bernhard
> >>>>>>
> >>>>>>
> >>>>>>>
> >>>>>>>>
> >>>>>>>> 		# Check which of the paks are services
> >>>>>>>> -		my @svc = `find /etc/init.d/$_ 2>/dev/null | cut -d"/" -f4`;
> >>>>>>>> -		foreach (@svc){
> >>>>>>>> +		if (-e "/etc/init.d/$_") {
> >>>>>>>> 			# blacklist some packages
> >>>>>>>> 			#
> >>>>>>>> 			# alsa has trouble with the volume saving and was not really stopped
> >>>>>>>> 			# mdadm should not stopped with webif because this could crash the system
> >>>>>>>> 			#
> >>>>>>>> -			chomp($_);
> >>>>>>>> 			if ( $_ eq 'squid' ) {
> >>>>>>>> 				next;
> >>>>>>>> 			}
> >>>>>>>> --
> >>>>>>>> 2.29.2
> > 
>
Michael Tremer Dec. 11, 2020, 8:45 a.m. UTC | #9 
Hi Adolf,

You changed some already messy code, so there is no point in changing variable names. It would probably make the diff even more complicated and won’t make a difference to the user anyways.

With new code, I would use explicit variable names. Languages like Python force you to do that (for x in y) and there is no way around it. I personally consider that better language design.

But it is great to have a conversation about this with you guys :)

-Michael

> On 10 Dec 2020, at 17:45, Adolf Belka <ahb.ipfire@gmail.com> wrote:
> 
> Hi,
> 
> 
> On 10/12/2020 16:46, Michael Tremer wrote:
>> Hi,
>>> On 10 Dec 2020, at 16:26, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
>>> 
>>> 
>>> 
>>>> Gesendet: Donnerstag, 10. Dezember 2020 um 15:48 Uhr
>>>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
>>>> An: "Bernhard Bitsch" <Bernhard.Bitsch@gmx.de>
>>>> Cc: "IPFire: Development-List" <development@lists.ipfire.org>
>>>> Betreff: Re: [PATCH] Fix for bug 12539
>>>> 
>>>> Hello,
>>>> 
>>>>> On 10 Dec 2020, at 15:35, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
>>>>> 
>>>>> Hi,
>>>>> 
>>>>>> Gesendet: Donnerstag, 10. Dezember 2020 um 14:29 Uhr
>>>>>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
>>>>>> An: "Bernhard Bitsch" <Bernhard.Bitsch@gmx.de>
>>>>>> Cc: "Adolf Belka" <ahb.ipfire@gmail.com>, development@lists.ipfire.org
>>>>>> Betreff: Re: [PATCH] Fix for bug 12539
>>>>>> 
>>>>>> Hi,
>>>>>> 
>>>>>>> On 10 Dec 2020, at 14:13, Bernhard Bitsch <Bernhard.Bitsch@gmx.de> wrote:
>>>>>>> 
>>>>>>> Hello,
>>>>>>> 
>>>>>>>> Gesendet: Donnerstag, 10. Dezember 2020 um 13:48 Uhr
>>>>>>>> Von: "Michael Tremer" <michael.tremer@ipfire.org>
>>>>>>>> An: "Adolf Belka" <ahb.ipfire@gmail.com>
>>>>>>>> Cc: development@lists.ipfire.org
>>>>>>>> Betreff: Re: [PATCH] Fix for bug 12539
>>>>>>>> 
>>>>>>>> Hello,
>>>>>>>> 
>>>>>>>> Great work. This looks like how it should be.
>>>>>>>> 
>>>>>>>>> On 7 Dec 2020, at 15:01, Adolf Belka <ahb.ipfire@gmail.com> wrote:
>>>>>>>>> 
>>>>>>>>> The installer recognises cups and cups-filters both as cups and puts
>>>>>>>>> two instances of cups in the add-on services table.
>>>>>>>>> Based on input from Michael Tremer this patch replaces the command
>>>>>>>>> returning the second element between hyphens with one that takes
>>>>>>>>> what comes after "meta-" using Perl code rather than a shell command.
>>>>>>>>> The second find command was changed as per Michael's suggestion.
>>>>>>>>> 
>>>>>>>>> Tested in my ipfire test bed system and only results in one cups
>>>>>>>>> entry.
>>>>>>>>> Signed-off-by: Adolf Belka <ahb.ipfire@gmail.com>
>>>>>>>>> ---
>>>>>>>>> html/cgi-bin/services.cgi | 9 +++++----
>>>>>>>>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>>>>>>>> 
>>>>>>>>> diff --git a/html/cgi-bin/services.cgi b/html/cgi-bin/services.cgi
>>>>>>>>> index 26ab4f314..36954ba70 100644
>>>>>>>>> --- a/html/cgi-bin/services.cgi
>>>>>>>>> +++ b/html/cgi-bin/services.cgi
>>>>>>>>> @@ -161,19 +161,20 @@ END
>>>>>>>>> 	my $lines=0; # Used to count the outputlines to make different bgcolor
>>>>>>>>> 
>>>>>>>>> 	# Generate list of installed addon pak's
>>>>>>>>> -	my @pak = `find /opt/pakfire/db/installed/meta-* 2>/dev/null | cut -d"-" -f2`;
>>>>>>>>> +	opendir (DIR, "/opt/pakfire/db/installed") || die "Cannot opendir /opt/pakfire/db/installed/: $!";
>>>>>>>>> +	my @pak = sort readdir DIR;
>>>>>>>>> 	foreach (@pak){
>>>>>>>>> 		chomp($_);
>>>>>>>>> +		next unless (m/^meta-/);
>>>>>>>>> +		s/^meta-//;
>>>>>>>> 
>>>>>>>> Although this is the least intuitive thing to do. I have no idea who designed Perl. I hope they are proud of all the chaos they have created :)
>>>>>>>> 
>>>>>>>> -Michael
>>>>>>> 
>>>>>>> Can't see where the solution is 'least intuitive'. As far as I can see from the patch, now the directory of installed addons is read with Perl functions ( not in a extra forked process ). The definition of filenames is used consequently ( 'meta-<addon name>', possible services are stored in /etc/init.d/<addon name> ).
>>>>>> 
>>>>>> To be clear, I am complaining about the Perl language design instead of the code. In my view something like
>>>>>> 
>>>>>> name = name.replace(“meta-“, “”)
>>>>>> 
>>>>>> is a thousand times easier to read.
>>>>>> 
>>>>>> Best,
>>>>>> -Michael
>>>>>> 
>>>>> 
>>>>> Okay, that makes it clearer.
>>>>> Perl has its roots in pattern matching. So the base functions of matching and replacing are defined with minimal overhead in writing.
>>>>> Maybe the designer(s) of Perl had in mind the description of COBOL as "speaking english with your computer". This was in the beginnings of computing, which did not experience all of us. ;)
>>>>> With this single example you are right. Your formulation is much more readable ( besides the fact that name is a string object ). But if you have many such snippets in your code, the Perl way makes a shorter and thus more understandable program.
>>>> 
>>>> I disagree. Why would the usage of something that is hard to comprehend be better when it is being used more often? Getting used to the wrong pattern doesn’t make it right.
>>>> 
>>>> My main complaint about this is, that it is not clear what is being manipulated. It is better to use the variable name and then there is no confusion about it. The same is true for loads of other Perl-isms like shift, chomp, chop, etc.
>>>> 
>>>> Shorter code does not necessarily mean that it executes faster. Probably the opposite is true in this case, because you would have to compile a regular expression first and the Python version would simply perform a string search.
>>>> 
>>>> The point is, that code should be obvious. It avoids bugs and it makes it easier to understand for reviewers what is supposed to go on.
>>>> 
>>> 
>>> The main difference between the Perl and the Python approach is the basic language class. Python works on objects and with their methods ( functions ), Perl works on plain variables with operators. Types are determined by the operator and context. Each operator has also a result which isn't destroyed by a assigment or statement end. This result is hold in a special variable which can be manipulated directly.
>>> If I review code, I should know this basics. Thus it doesn't make a real difference in comprehension ( to me ) if I read a object-oriented Python code or a Perl code. Nevertheless is the Perl code shorter. In our special example Perl avoids the generation (in written code ) of an intermeditiate variable by use of the implicit $_.
>>> But this is a case of habit. Some like to talk in English only, some like to talk in their native language to be more exact. ;)
>> Yes, I understand that. But what do you do when you have more than one variable? Most of my code does.
> 
> My Perl learning here included the existence of the implicit $_. I let it in place because the section covered by foreach (@pak) was using it.
> However if the preference is to be explicit with variables, I have no problem in doing that in any future bug fixes etc that I do.
> 
> Regards,
> Adolf.
> 
>>> Best,
>>> Bernhard
>>>> Best,
>>>> -Michael
>>>> 
>>>>> 
>>>>> Best,
>>>>> Bernhard
>>>>> 
>>>>>>> 
>>>>>>> - Bernhard
>>>>>>> 
>>>>>>> 
>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> 		# Check which of the paks are services
>>>>>>>>> -		my @svc = `find /etc/init.d/$_ 2>/dev/null | cut -d"/" -f4`;
>>>>>>>>> -		foreach (@svc){
>>>>>>>>> +		if (-e "/etc/init.d/$_") {
>>>>>>>>> 			# blacklist some packages
>>>>>>>>> 			#
>>>>>>>>> 			# alsa has trouble with the volume saving and was not really stopped
>>>>>>>>> 			# mdadm should not stopped with webif because this could crash the system
>>>>>>>>> 			#
>>>>>>>>> -			chomp($_);
>>>>>>>>> 			if ( $_ eq 'squid' ) {
>>>>>>>>> 				next;
>>>>>>>>> 			}
>>>>>>>>> --
>>>>>>>>> 2.29.2
diff mbox series

Patch

diff --git a/html/cgi-bin/services.cgi b/html/cgi-bin/services.cgi
index 26ab4f314..36954ba70 100644
--- a/html/cgi-bin/services.cgi
+++ b/html/cgi-bin/services.cgi
@@ -161,19 +161,20 @@  END
 	my $lines=0; # Used to count the outputlines to make different bgcolor
 
 	# Generate list of installed addon pak's
-	my @pak = `find /opt/pakfire/db/installed/meta-* 2>/dev/null | cut -d"-" -f2`;
+	opendir (DIR, "/opt/pakfire/db/installed") || die "Cannot opendir /opt/pakfire/db/installed/: $!";
+	my @pak = sort readdir DIR;
 	foreach (@pak){
 		chomp($_);
+		next unless (m/^meta-/);
+		s/^meta-//;
 
 		# Check which of the paks are services
-		my @svc = `find /etc/init.d/$_ 2>/dev/null | cut -d"/" -f4`;
-		foreach (@svc){
+		if (-e "/etc/init.d/$_") {
 			# blacklist some packages
 			#
 			# alsa has trouble with the volume saving and was not really stopped
 			# mdadm should not stopped with webif because this could crash the system
 			#
-			chomp($_);
 			if ( $_ eq 'squid' ) {
 				next;
 			}