BBC: SYBER/Test-Catch-2.0.1.tar.gz et al.: Attempt to access disallowed key 'xsi' in a restricted hash

[eserte]

Since perl 5.41.4 (not known for 5.41.3) some modules cannot be built anymore due to the following error:

Attempt to access disallowed key 'xsi' in a restricted hash at /home/e/eserte/.cpan/build/2024093023/XS-Install-1.3.5-1/blib/lib/XS/Install/ParseXS.pm line 81, <__ANONIO__> line 356.

CPAN distributions affected are

• SYBER/Test-Catch-2.0.1.tar.gz
• SYBER/next-XS-1.0.7.tar.gz
• DMOL/CPP-geos-3.7.3.1.tar.gz

All of these fail in the same XS::Install::ParseXS line.

A matrix link for the 1st of these: https://fast2-matrix.cpantesters.org/?dist=Test-Catch-2.0.1

[jkeenan]

In the time available to me I was able to bisect only one of the three failing distributions. Testing Test::Catch on unthreaded builds on Linux with the following invocation:

perl Porting/bisect.pl \
--start=bcb5884b9b0ae1345ac74420bacb865ba11baab3 \
--end=v5.41.4 \
--module=Test::Catch

... bisection pointed to c85d5a6 as the first bad commit.

c85d5a67185efe2ff72c5c523fa070bbc7ccecdf is the first bad commit
commit c85d5a67185efe2ff72c5c523fa070bbc7ccecdf
Author: David Mitchell <[email protected]>
Date:   Thu Aug 8 11:19:46 2024 +0100
Commit:     David Mitchell <[email protected]>
CommitDate: Thu Aug 15 19:31:22 2024 +0100

    ExtUtils::ParseXS: rename most object fields
    
    This big commit renames around 60 of the 75-or-so fields within an
    ExtUtils::ParseXS hash object.
    
    This is because many of the current names are confusing and follow no
    particular pattern.

@iabyn, can you take a look? Thanks.

UPDATE: Bisecting with this invocation:

$ perl Porting/bisect.pl \
--start=bcb5884b9b0ae1345ac74420bacb865ba11baab3 \
--end=v5.41.4 \
--module=next::XS

... showed that c85d5a6 was also the breaking commit for next::XS.

[Leont]

That code starts with

my $orig_pmxl = \&ExtUtils::ParseXS::_process_module_xs_line;
*ExtUtils::ParseXS::_process_module_xs_line = sub {

When you're monkey patching like that, you shouldn't be surprised when things break. And that's far from the only method it patches. XS::Install is very much voiding any warranty IMO.

[jkeenan]

Testing CPP::geos on unthreaded builds on Linux with the following invocation:

$ perl Porting/bisect.pl \
--start=bcb5884b9b0ae1345ac74420bacb865ba11baab3 \
--end=v5.41.4 \
--module=CPP::geos

... bisection pointed to 9cd5757 as the breaking commit.

9cd5757dce9a4cb7bc255f52e311c32cffe78969 is the first bad commit
commit 9cd5757dce9a4cb7bc255f52e311c32cffe78969
Author: David Mitchell <[email protected]>
Date:   Fri Aug 16 13:49:15 2024 +0100
Commit:     David Mitchell <[email protected]>
CommitDate: Mon Aug 19 11:32:03 2024 +0100

    ParseXS: stop inadvertently disabling field checks

@iabyn, can you take a look? Thanks.

[jkeenan]

@iabyn, could you comment on the 2 breaking commits mentioned above? Thanks.

[Leont]

@iabyn, could you comment on the 2 breaking commits mentioned above? Thanks.

I'm not sure what you want him to look at exactly, the problem is already understood. XS::Install::ParseXS is fiddling around with ExtUtils::ParseXS in ways that break its encapsulation because it's now using fields, so X::I will have to adapt to these changes. That's up to @syberrus to fix really.

[jkeenan]

@iabyn, could you comment on the 2 breaking commits mentioned above? Thanks.

I'm not sure what you want him to look at exactly, the problem is already understood. XS::Install::ParseXS is fiddling around with ExtUtils::ParseXS in ways that break its encapsulation because it's now using fields, so X::I will have to adapt to these changes. That's up to @syberrus to fix really.

The XS-Install distribution is currently passing all its tests at CPANtesters. Hence, its maintainer may not be aware of these problems. Could you post a ticket about them at https://rt.cpan.org/Dist/Display.html?Name=XS-Install? Thanks.

[Leont]

The XS-Install distribution is currently passing all its tests at CPANtesters. Hence, its maintainer may not be aware of these problems. Could you post a ticket about them at https://rt.cpan.org/Dist/Display.html?Name=XS-Install? Thanks.

https://rt.cpan.org/Ticket/Display.html?id=15702

[iabyn]

On Mon, Nov 11, 2024 at 01:18:17PM -0800, Leon Timmermans wrote: > @iabyn, could you comment on the 2 breaking commits mentioned above? Thanks. I'm not sure what you want him to look at exactly, the problem is already understood. XS::Install::ParseXS is fiddling around with ExtUtils::ParseXS in ways that break its encapsulation because it's now using `fields`, so X::I will have to adapt to these changes. That's up to @syberrus to fix really.

Yeah, for the record: XS-Install basically wraps several ExtUtils::ParseXS methods (including private ones), and the wrappers generally mess about with the contents of a new field added to the E::PXS object by the XS-Install distro, 'xsi', then call the original (wrapped) method. Now that E::PXS uses the 'use fields' mechanism to enforce which fields are allowed in a E::PXS object, a caller can't just add new fields (like 'xsi'). I originally added 'use fields' to E::PXS because it had become chaotic - there were in the region of 80 fields in use for an E::PXS object, mostly undocumented. 'use fields' allowed me to discover what fields were actually used and document them. There is less need for 'use fields' now, although it certainly helps spotting errors during development work. As to fixing XS-Install (and thus its broken dependents), one option would be to disable 'use fields' in production, and only use it for development; but the chances are that I (and others in future) would forget to set the appropriate env var or whatever during development. A second option would be to add 'xsi' as an allowed field - for the sole use of XS-Install - and which would be ignored by E::PXS. Both of those options suffer from the fact that I've been (and continue to be) heavily refactoring the internals of E::PXS - just being able to add an xsi field may be the least of the author's problems when messing about with the internals of E::PXS. So initially at least, I think this is the author's issue, and I don't see anything that I should/could do at the E::PXS end of things to make those distros pass again. But I'm always open to suggestions.

…

-- Spock (or Data) is fired from his high-ranking position for not being able to understand the most basic nuances of about one in three sentences that anyone says to him. -- Things That Never Happen in "Star Trek" #19

[syberrus]

No need to change anything. I already fixed that. Just not uploaded yet. Пн, 18 нояб. 2024 г. в 15:21, David Mitchell ***@***.***>:

…

On Mon, Nov 11, 2024 at 01:18:17PM -0800, Leon Timmermans wrote: > > @iabyn, could you comment on the 2 breaking commits mentioned above? Thanks. > > I'm not sure what you want him to look at exactly, the problem is > already understood. XS::Install::ParseXS is fiddling around with > ExtUtils::ParseXS in ways that break its encapsulation because it's now > using `fields`, so X::I will have to adapt to these changes. That's up > to @syberrus to fix really. Yeah, for the record: XS-Install basically wraps several ExtUtils::ParseXS methods (including private ones), and the wrappers generally mess about with the contents of a new field added to the E::PXS object by the XS-Install distro, 'xsi', then call the original (wrapped) method. Now that E::PXS uses the 'use fields' mechanism to enforce which fields are allowed in a E::PXS object, a caller can't just add new fields (like 'xsi'). I originally added 'use fields' to E::PXS because it had become chaotic - there were in the region of 80 fields in use for an E::PXS object, mostly undocumented. 'use fields' allowed me to discover what fields were actually used and document them. There is less need for 'use fields' now, although it certainly helps spotting errors during development work. As to fixing XS-Install (and thus its broken dependents), one option would be to disable 'use fields' in production, and only use it for development; but the chances are that I (and others in future) would forget to set the appropriate env var or whatever during development. A second option would be to add 'xsi' as an allowed field - for the sole use of XS-Install - and which would be ignored by E::PXS. Both of those options suffer from the fact that I've been (and continue to be) heavily refactoring the internals of E::PXS - just being able to add an xsi field may be the least of the author's problems when messing about with the internals of E::PXS. So initially at least, I think this is the author's issue, and I don't see anything that I should/could do at the E::PXS end of things to make those distros pass again. But I'm always open to suggestions. -- Spock (or Data) is fired from his high-ranking position for not being able to understand the most basic nuances of about one in three sentences that anyone says to him. -- Things That Never Happen in "Star Trek" #19 — Reply to this email directly, view it on GitHub <#22713 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB6HYZQTFRMLOCAFPLEROYT2BHLVDAVCNFSM6AAAAABQ3MGSAKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIOBSHA4DQMJZGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>