Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

t/re/pat.t: Compilation error during make minitest due to @{^CAPTURE} #17293

Closed
jkeenan opened this issue Nov 12, 2019 · 12 comments
Closed

t/re/pat.t: Compilation error during make minitest due to @{^CAPTURE} #17293

jkeenan opened this issue Nov 12, 2019 · 12 comments
Assignees
@tonycoz

Comments

@jkeenan
Copy link
Contributor

jkeenan commented Nov 12, 2019

When running make minitest on blead (following up on a #p5p comment by @khwilliamson), t/re/pat.t fails to compile. A trimmed-down version of the error message:

t/re/pat ....................... Can't locate Tie/Hash/NamedCapture.pm in @INC 
(you may need to install the Tie::Hash::NamedCapture module) 
(@INC contains: ../lib . ../ext/re 
# ... long list of directories -- not including ext/Tie-Hash-NamedCapture ...
... 
/home/jkeenan/gitwork/perl/lib .. . .) at re/pat.t line 674.
BEGIN failed--compilation aborted at re/pat.t line 674.

In the core distribution, we have ext/Tie-Hash-NamedCapture/NamedCapture.pm. I tried various ways of dealing with this failure. I tried adding ../ext/Tie-Hash-NamedCapture to @INC. I tried adding a skip("message") if is_miniperl; statement (as in t/re/subst.t). None of these worked, i.e., none of them enabled the file to compile. Whenever I hit line 674 below, I got the fatal error.

 672         $_= "ace";
 673         /c(?=.$)/;
 674         is($#{^CAPTURE}, -1, $message);

Any ideas?
@khwilliamson
Copy link
Contributor

@hvds I'm proposing that we schedule regular smokes of minitest in order to catch breaking changes like this. Can you offer insights into the importance of this?

@demerphq
Copy link
Collaborator

demerphq commented Nov 12, 2019 via email

@jkeenan
Copy link
Contributor Author

jkeenan commented Nov 12, 2019 via email

@demerphq
Copy link
Collaborator

demerphq commented Nov 12, 2019 via email

@jkeenan
Copy link
Contributor Author

jkeenan commented Nov 12, 2019

Bisection points to: 22f0578

22f05786af0b7f963440e47908cd5f35cf074c12 is the first bad commit
commit 22f05786af0b7f963440e47908cd5f35cf074c12
Author: Tony Cook <tony@develop-help.com>
Date:   Thu Jun 13 10:05:15 2019 +1000

    (perl #134193) allow %{^CAPTURE} to work when @{^CAPTURE} comes first
    
    gv_magicalize() is called when the GV is created, so when the array
    was mentioned first, the hash wouldn't reach this code and the magic
    wouldn't be added to the hash.
    
    This also fixes a similar problem with (%|@){^CAPTURE_ALL}, though
    @{^CAPTURE_ALL} is unused at this point.

Bisection invocation:

perl Porting/bisect.pl --target=miniperl --start=2ec4590ec8255732ec2d163daabd237cdaf886f4   -e 'q|ace| =~ /c(?=.$)/; print ((($#{^CAPTURE} == -1) ? q|AAA| : q|BBB|), "\n") && exit 0'

commit 22f05786af0b7f963440e47908cd5f35cf074c12 (refs/bisect/bad)
Author:     Tony Cook <tony@develop-help.com>
AuthorDate: Thu Jun 13 10:05:15 2019 +1000
Commit:     Tony Cook <tony@develop-help.com>
CommitDate: Wed Jun 19 14:22:10 2019 +1000

@tonycoz, can you take a look?

Thank you very much.
Jim Keenan

@tonycoz tonycoz self-assigned this Nov 12, 2019
@tonycoz
Copy link
Contributor

tonycoz commented Nov 12, 2019

My commit made access to *{^CAPTURE} and *{^CAPTURE_ALL} load Tie::Hash::NamedCapture unconditionally due to gv_magicalize() only being called once for each GV with a name longer than one character, which is why the use of @{^CAPTURE} in this code is trying (and failing) to load it.

I can see a few solutions, which are either non-trivial or could reduce performance:

  • call gv_magicalize() every time and revert the gv.c changes - this would add a performance penalty to every gv access.
  • add a gvmini.o, like we have opmini.o and #ifdef out the Tie::Hash::NamedCapture load for miniperl - I'm not fond of the *mini.o hack
  • move the implementation of %{^CAPTURE} and %{^CAPTURE_ALL} (and %+, %-) into the core binary - this would be largely be moving the tie code into universal.c

I'm most inclined to go for the last.

@khwilliamson
Copy link
Contributor

khwilliamson commented Nov 12, 2019 via email

@demerphq
Copy link
Collaborator

demerphq commented Nov 12, 2019 via email

@tonycoz
Copy link
Contributor

tonycoz commented Nov 12, 2019

I think that merely skipping this test under minitest is the way to go. Unless this has uncovered a performance issue. minitest is known to be crippled, and many tests are skipped in it that would require a load.

The gv is magicalized at compile-time, to prevent it under minitest we'd have to move the relevant tests to a new file or put them in a string eval, both of which is ugly.

@jkeenan
Copy link
Contributor Author

jkeenan commented Nov 13, 2019 •
edited
Loading

TonyC wrote:

My commit made access to *{^CAPTURE} and *{^CAPTURE_ALL} load Tie::Hash::NamedCapture unconditionally due to gv_magicalize() only being called once for each GV with a name longer than one character, which is why the use of @{^CAPTURE} in this code is trying (and failing) to load it.

I can see a few solutions, which are either non-trivial or could reduce performance:

* call gv_magicalize() every time and revert the gv.c changes - this would add a performance penalty to every gv access.

* add a gvmini.o, like we have opmini.o and #ifdef out the Tie::Hash::NamedCapture load for miniperl - I'm not fond of the *mini.o hack

* move the implementation of `%{^CAPTURE}` and `%{^CAPTURE_ALL}` (and `%+`, `%-`) into the core binary - this would be largely be moving the tie code into universal.c

I'm most inclined to go for the last.

The last sounds good to me, too.

@jkeenan
Copy link
Contributor Author

jkeenan commented Nov 13, 2019

khw wrote:

I think that merely skipping this test under minitest is the way to go. Unless this has uncovered a performance issue. minitest is known to be crippled, and many tests are skipped in it that would require a load.

As I wrote previously, I made several attempts to skip this test (or, at least, the tests within the file that pertain to @{$^CAPTURE} under make minitest but was unsuccessful. If you can figure out how to do so, please do.

Thanks.
jimk

@hvds
Copy link
Contributor

hvds commented Nov 13, 2019

@hvds I'm proposing that we schedule regular smokes of minitest in order to catch breaking changes like this. Can you offer insights into the importance of this?

I think it would be valuable to do so, but the value is limited: it's handy for it to work when you want it, but if the full test suite is not failing it's likely only telling you something about the testsuite rather than about perl - in most cases one could just as usefully discover and fix that at the point one tries to use it.

As for the fix: I agree that string eval is probably the way to go.

Hugo

tonycoz added a commit to tonycoz/perl5 that referenced this issue Nov 17, 2019
@tonycoz
move the implementation of %-, %+ into core
4b13dcb 
Previousl this could cause problems during minitest.

Fixes Perl#17293
@jkeenan jkeenan mentioned this issue Nov 17, 2019
Merged
tonycoz added a commit to tonycoz/perl5 that referenced this issue Nov 18, 2019
@tonycoz
move the implementation of %-, %+ into core
c36b060 
Previousl this could cause problems during minitest.

Fixes Perl#17293
tonycoz added a commit to tonycoz/perl5 that referenced this issue Nov 28, 2019
@tonycoz
move the implementation of %-, %+ into core
c3d3c01 
Previousl this could cause problems during minitest.

Fixes Perl#17293
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tonycoz tonycoz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@jkeenan @demerphq @khwilliamson @tonycoz @hvds