Re: [PATCH] perl-Encode-Locale: New module dependency for LWP::UserAgent

[Rob Brewer <ipfire-devel@grantura.co.uk>]

[-- Attachment #1: Type: text/plain, Size: 12843 bytes --]

Hi Adolf ++,

On Sunday 19 June 2022 11:37 Adolf Belka wrote:

> Hi Rob and All,
> 
> On 19/06/2022 00:48, Rob Brewer wrote:
>> Hi Adolf ++,
>>
>> On Saturday 18 June 2022 10:33 Adolf Belka wrote:
>>
>>> Hi Stefan, Rob and all,
>>>
>>> On 18/06/2022 10:29, Stefan Schantl wrote:
>>>> Hello Adolf,
>>>>
>>>> thanks for working on this and sending your patch.
>>>>
>>>> Everything looks good except the rootfile. When adding new perl
>>>> modulles
>>>> they are some kind of special and must  not contain any  architecture
>>>> specific directory names.
>>>>
>>>> These needs to  be replaced by a "xxxMACHINExxx".
>>> Duuuh.
>>>
>>> I know that but still managed to miss it. My only excuse is that I was
>>> on vacation for 2 weeks and did not work on any patches so forgot about
>>> that or didn't even see the architecture specific bits in the rootfile
>>> lines. :-) :-)
>>>
>>>
>>> I do have a question now about this patch. The need for it was triggered
>>> by an email from Rob. I had thought that he had identified error
>>> messages about the missing Encode::Locale for LWP::UserAgent in core
>>> parts of IPFire.
>>>
>>> However i see now from this message from Rob
>>> https://lists.ipfire.org/pipermail/development/2022-June/013691.html
>>> that it was identified from a personal add-on that he runs to generate
>>> ADSL graphs.
>>>
>>> In IPFire LWP::UserAgent is used for the IDS core package and for the
>>> Proxy Accounting add-on. I have both activated in my IPFire systems and
>>> checking the logs there are no entries at all related to Encode, encode,
>>> Locale or locale. So it looks like for IPFire its use of UserAgent.pm
>>> does not need the Encode/Locale.pm module.
>>>
>>> My feeling would therefore be that Encode::Locale should not be
>>> installed into the core package set unless my comments about
>>> UserAgent.pm use in IPFire has a fault in it.
>>>
>>> Rob could make an additional personal add-on for Encode::Locale so his
>>> ADSL data graphing add-on continues to work.
>>>
>>> It doesn't seem correct to add another package into the core IPFire set
>>> that won't be used by nearly all IPFire systems.
>>>
>>> Feedback welcome, especially if I have misunderstood or missed anything.
>>>
>>> Regards,
>>>
>>> Adolf.
>>>
>> I understand your concerns, in my application I use LWP::UserAgent to
>> access the router web page and this produced errors that Encode::Locale
>> was missing and failed to compile after the new UserAgent.pm was
>> introduced on CU 165. Checking my backups before CU 165 shows that
>> 'require Encode::Locale' was
>> introduced into  LWP::UserAgent  in the version at the update to CU 165.
>>
>> It would seem that this latest version of UserAgent assumes that
>> Encode::Locale is present so I would be surprised that other programs
>> using UserAgent were not affected by the missing perl module.
> 
> Valid point, so I had a search through the coding.
> 
> I have gone through the UserAgent .pm code and also the places in IPFire
> that specify the LWP::UserAgent module and found the following interesting
> results.
> 
> 
> In UserAgent.pm in the env_proxy subroutine both the Encode and
> Encode::Locale modules are defined as Required. These two modules are
> completely separate modules.
> 
> The Encode::Locale modules says
> 
> "The |Encode::Locale| module looks up the charset and encoding and
> arranges for the Encode module to know this encoding under the name
> "locale". It means bytes obtained from the environment can be converted to
> Unicode strings by calling |Encode::encode(locale => $bytes)| and
> converted back again with |Encode::decode(locale => $string)|"
> 
> In UserAgent, only the Encode::decode(locale => $string) command is used.
> For something to be done with the output from this command there should be
> the perl Encode module also installed but that is not present in IPFire.
> From what you wrote it looks like you didn't need to install that module
> for your addon to work again. It looks like your addon is using the
> env_proxy subroutine from UserAgent and that is what is triggering the
> need for Encode::Locale. Interestingly your addon is calling the env_proxy
> subroutine in UserAgent, which then decodes the locale and places the
> result in a variable called "locale" for the Encode perl module to use if
> it was installed.
> 
> 
> In IPFire LWP::UserAgent is defined as "use" in ids-functions.pl,
> functions.pl and accounting.cgi for the Proxy Accounting addon.
> 
> In both ids-functions.pl and functions.pl the only call to UserAgent is
> for the new command and this is used to request SSL hostname verification
> and to specify the CA file path.
> 
> In accounting.cgi there is no subroutine call from UserAgent, so it looks
> like it has been defined to use LWP::UserAgent but is not actually ever
> used. So probably that use command could be removed from that cgi script.
> 
> So the reason that IPFire does not get any error messages related to
> Encode::Locale is that the env_proxy subroutine from UserAgent is not
> called for use anywhere in IPFire.
> 
> Based on this I think we should not install the Encode::Locale module and
> you should add this perl module into the build of your addon.
> 
> Regards,
> 
> Adolf
> 
Your comments are very helpful :)

I carried out a similar analysis on the instances of UserAgent and agree 
that it is my program that is broken (In my defense I wrote it about 15 
years ago and had to refresh my understanding of what was happening).

It turns out that in my code there was a declaration of an unused   
LWP::Simple module which was also upgraded at the same time as UserAgent in 
IPFire.

LWP::Simple now contains:

use LWP::UserAgent ();
use HTTP::Date ();
our $ua = LWP::UserAgent->new;  # we create a global UserAgent object
$ua->agent("LWP::Simple/$VERSION ");
$ua->env_proxy;

so it is the 'eny_proxy'  declared in the new LWP::Simple, which requires 
Encode::Locale in UserAgent and causes my error.

Commenting out 'use LWP::Simple' in my code prevents the error.

I don't know if LWP::Simple is used in any current IPFire code but I would 
expect the 'Encode/Locale.pm' error to be displayed if it were used.

Thank you for taking the time to look at this, I must admit I was surprised 
by the complicated relationship between the perl modules used and that this 
programming bug took 15 years to surface.


Thanks again

Rob


>> Looking at my ADSL graph code there is nothing unusual about the use of
>> UserAgent and I raised this query expecting that other code such as that
>> mentioned above would be similarly affected as mine and this would help
>> sort problems elsewhere. If as you suggest that the other scripts on
>> IPFire aren't affected I am happy to make changes to my code.
>>
>> Regards
>>
>> Rob
>>
>>
>>
>>>> For more details see:
>>>>
>> 
https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=config/rootfiles/common/perl-HTTP-Date;h=75c250b7bad745eb70cec3115e79e24d231d69c1;hb=HEAD
>> 
<https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=config/rootfiles/common/perl-HTTP-Date;h=75c250b7bad745eb70cec3115e79e24d231d69c1;hb=HEAD>
>>>> Best regards,
>>>>
>>>> -Stefan
>>>>
>>>> Am 17. Juni 2022 22:50:45 schrieb Adolf Belka <adolf.belka(a)ipfire.org>:
>>>>
>>>>> - UserAgent.pm now has a dependency on Encode/Locale.pm
>>>>> - lfs and rootfile created
>>>>> - Module added to make.sh
>>>>>
>>>>> Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
>>>>> ---
>>>>> config/rootfiles/common/perl-Encode-Locale |  6 ++
>>>>> lfs/perl-Encode-Locale                     | 79 ++++++++++++++++++++++
>>>>> make.sh                                    |  1 +
>>>>> 3 files changed, 86 insertions(+)
>>>>> create mode 100644 config/rootfiles/common/perl-Encode-Locale
>>>>> create mode 100644 lfs/perl-Encode-Locale
>>>>>
>>>>> diff --git a/config/rootfiles/common/perl-Encode-Locale
>>>>> b/config/rootfiles/common/perl-Encode-Locale
>>>>> new file mode 100644
>>>>> index 000000000..b3c4d8fb7
>>>>> --- /dev/null
>>>>> +++ b/config/rootfiles/common/perl-Encode-Locale
>>>>> @@ -0,0 +1,6 @@
>>>>> +#usr/lib/perl5/site_perl/5.32.1/Encode
>>>>> +usr/lib/perl5/site_perl/5.32.1/Encode/Locale.pm
>>>>> +#usr/lib/perl5/site_perl/5.32.1/x86_64-linux-thread-multi/auto/Encode
>>>>> +#usr/lib/perl5/site_perl/5.32.1/x86_64-linux-thread-
>> multi/auto/Encode/Locale
>>>>> +#usr/lib/perl5/site_perl/5.32.1/x86_64-linux-thread-
>> multi/auto/Encode/Locale/.packlist
>>>>> +#usr/share/man/man3/Encode::Locale.3
>>>>> diff --git a/lfs/perl-Encode-Locale b/lfs/perl-Encode-Locale
>>>>> new file mode 100644
>>>>> index 000000000..a51208971
>>>>> --- /dev/null
>>>>> +++ b/lfs/perl-Encode-Locale
>>>>> @@ -0,0 +1,79 @@
>>>>>
>> 
+###############################################################################
>>>>> +#
>>>>> #
>>>>> +# IPFire.org - A linux based firewall
>>>>> #
>>>>> +# Copyright (C) 2007-2019  IPFire Team  <info(a)ipfire.org>
>>>>> #
>>>>> +#
>>>>> #
>>>>> +# This program is free software: you can redistribute it and/or
>>>>> modify        #
>>>>> +# it under the terms of the GNU General Public License as published
>>>>> by        #
>>>>> +# the Free Software Foundation, either version 3 of the License, or
>>>>> #
>>>>> +# (at your option) any later version.
>>>>> #
>>>>> +#
>>>>> #
>>>>> +# This program is distributed in the hope that it will be useful,
>>>>> #
>>>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> #
>>>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>>> #
>>>>> +# GNU General Public License for more details.
>>>>> #
>>>>> +#
>>>>> #
>>>>> +# You should have received a copy of the GNU General Public License
>>>>> #
>>>>> +# along with this program.  If not, see
>>>>> <http://www.gnu.org/licenses/>.       #
>>>>> +#
>>>>> #
>>>>>
>> 
+###############################################################################
>>>>> +
>>>>> +
>>>>>
>> 
+###############################################################################
>>>>> +# Definitions
>>>>>
>> 
+###############################################################################
>>>>> +include Config
>>>>> +VER        = 1.05
>>>>> +
>>>>> +THISAPP    = Encode-Locale-$(VER)
>>>>> +DL_FILE    = ${THISAPP}.tar.gz
>>>>> +DL_FROM    = $(URL_IPFIRE)
>>>>> +DIR_APP    = $(DIR_SRC)/$(THISAPP)
>>>>> +TARGET     = $(DIR_INFO)/$(THISAPP)
>>>>> +
>>>>>
>> 
+###############################################################################
>>>>> +# Top-level Rules
>>>>>
>> 
+###############################################################################
>>>>> +
>>>>> +objects = $(DL_FILE)
>>>>> +
>>>>> +$(DL_FILE) = $(DL_FROM)/$(DL_FILE)
>>>>> +
>>>>> +$(DL_FILE)_BLAKE2 =
>>>>>
>> 
f66bac8ebf012e7673b344b3899bed755558b80833a68b009b6083aeadd9d69748a63bee4e5e3c20dffaf7f2551fd6c9c778273ae992752c426e081d35715dee
>>>>> +
>>>>> +install : $(TARGET)
>>>>> +
>>>>> +check : $(patsubst %,$(DIR_CHK)/%,$(objects))
>>>>> +
>>>>> +download :$(patsubst %,$(DIR_DL)/%,$(objects))
>>>>> +
>>>>> +b2 : $(subst %,%_BLAKE2,$(objects))
>>>>> +
>>>>> +dist:
>>>>> + @$(PAK)
>>>>> +
>>>>>
>> 
+###############################################################################
>>>>> +# Downloading, checking, b2sum
>>>>>
>> 
+###############################################################################
>>>>> +
>>>>> +$(patsubst %,$(DIR_CHK)/%,$(objects)) :
>>>>> + @$(CHECK)
>>>>> +
>>>>> +$(patsubst %,$(DIR_DL)/%,$(objects)) :
>>>>> + @$(LOAD)
>>>>> +
>>>>> +$(subst %,%_BLAKE2,$(objects)) :
>>>>> + @$(B2SUM)
>>>>> +
>>>>>
>> 
+###############################################################################
>>>>> +# Installation Details
>>>>>
>> 
+###############################################################################
>>>>> +
>>>>> +$(TARGET) : $(patsubst %,$(DIR_DL)/%,$(objects))
>>>>> + @$(PREBUILD)
>>>>> + @rm -rf $(DIR_APP) && cd $(DIR_SRC) && tar zxf $(DIR_DL)/$(DL_FILE)
>>>>> + cd $(DIR_APP) && perl Makefile.PL
>>>>> + cd $(DIR_APP) && make $(MAKETUNING) $(EXTRA_MAKE)
>>>>> + cd $(DIR_APP) && make install
>>>>> + @rm -rf $(DIR_APP)
>>>>> + @$(POSTBUILD)
>>>>> diff --git a/make.sh b/make.sh
>>>>> index 2a4f6d0bd..dd84cdc99 100755
>>>>> --- a/make.sh
>>>>> +++ b/make.sh
>>>>> @@ -1373,6 +1373,7 @@ buildipfire() {
>>>>> lfsmake2 perl-Digest
>>>>> lfsmake2 perl-Digest-SHA1
>>>>> lfsmake2 perl-Digest-HMAC
>>>>> +  lfsmake2 perl-Encode-Locale
>>>>> lfsmake2 perl-libwww
>>>>> lfsmake2 perl-LWP-Protocol-https
>>>>> lfsmake2 perl-Net-HTTP
>>>>> --
>>>>> 2.36.1