Skip to content

Fix _sub_attrs for perl 5.37.9#9

Merged
karenetheridge merged 1 commit intomoose:masterfrom
demerphq:yves/fix_sub_attrs_and_spelling_issues
Mar 5, 2023
Merged

Fix _sub_attrs for perl 5.37.9#9
karenetheridge merged 1 commit intomoose:masterfrom
demerphq:yves/fix_sub_attrs_and_spelling_issues

Conversation

@demerphq
Copy link
Copy Markdown
Contributor

@demerphq demerphq commented Mar 2, 2023

This fixes an issue with _sub_attrs() where it does not localize $SIG{__DIE__} when it does its eval probe. A fix to Perl 5.37.9 made it so that $SIG{__DIE__} is always called on a compile error, unlike before where it depended on the nature of the error and how many errors.

See:
Perl/perl5#20357
https://rt.cpan.org/Ticket/Display.html?id=146848
Perl/perl5#20885
fany/MIME-Signature#3

This also fixes the spelling issues reported in https://rt.cpan.org/Ticket/Display.html?id=146847

It also bumps the version. Once this is released with the _sub_attrs fix, MIME::Signature needs to be updated to depend on the new version. Please coordinate with fany/MIME-Signature#3 and fany/MIME-Signature#4

In Perl/perl5#20357 we fixed an inconsistency in
how eval EXPR handles compile errors so that the behavior is consistent
regardless of the number or type of compile errors that are encountered.
This means that $SIG{__DIE__} is called if there is an error compiling
the code regardless of how the code fails to compile.

The _sub_attrs() function does not localize $SIG{__DIE__} when it probes
to see if the coderef is an lvalue function. This then breaks modules
like MIME::Signature, see fany/MIME-Signature#3
and Perl/perl5#20885 and also
https://rt.cpan.org/Ticket/Display.html?id=146848 for relevant bug
reports.

This patch fixes these issues.
@demerphq demerphq changed the title Yves/fix sub attrs and spelling issues Fix _sub_attrs for perl 5.37.9 and also correct some spelling issues reported during dzil test, bump version to 2.16 Mar 2, 2023
demerphq added a commit to demerphq/MIME-Signature that referenced this pull request Mar 2, 2023
Class::Method::Modifiers version < 2.16 are subtly broken in
Perl 5.37.9, this in turn breaks MIME::Signature/MIME::Disclaimer.

I have created moose/Class-Method-Modifiers#9
which should fix Class::Method::Modifiers. This patch should be
applied once that (or an equivalent fix) is merged and a new version
is released.

I have asked the owner of that repo to coordinate with this PR.

See:
Perl/perl5#20357
https://rt.cpan.org/Ticket/Display.html?id=146848
Perl/perl5#20885
fany#3
@haarg
Copy link
Copy Markdown
Member

haarg commented Mar 2, 2023

Can you change this to only include the __DIE__ change? The spelling changes are unfortunately dependent on how your local environment is set up, but there's nothing wrong with the existing text. "Behavior" is correct for US English. And the version numbers for this dist are automatically incremented by Dist::Zilla on release - the next release will be 2.15.

@demerphq
Copy link
Copy Markdown
Contributor Author

demerphq commented Mar 2, 2023

Done.

demerphq added a commit to demerphq/MIME-Signature that referenced this pull request Mar 2, 2023
Class::Method::Modifiers version < 2.15 are subtly broken in
Perl 5.37.9, this in turn breaks MIME::Signature/MIME::Disclaimer.

I have created moose/Class-Method-Modifiers#9
which should fix Class::Method::Modifiers. This patch should be
applied once that (or an equivalent fix) is merged and a new version
is released.

I have asked the owner of that repo to coordinate with this PR.

See:
Perl/perl5#20357
https://rt.cpan.org/Ticket/Display.html?id=146848
Perl/perl5#20885
fany#3
@demerphq demerphq changed the title Fix _sub_attrs for perl 5.37.9 and also correct some spelling issues reported during dzil test, bump version to 2.16 Fix _sub_attrs for perl 5.37.9 Mar 2, 2023
@karenetheridge
Copy link
Copy Markdown
Member

I am Canadian and did the last release; using the en_US, en_CA and en_UK aspell dictionaries there are no errors reported.

@demerphq
Copy link
Copy Markdown
Contributor Author

demerphq commented Mar 2, 2023 via email

@karenetheridge
Copy link
Copy Markdown
Member

LANG=en_CA.UTF-8
LC_ALL=en_CA.UTF-8
LC_CTYPE=en_CA.UTF-8

@karenetheridge
Copy link
Copy Markdown
Member

karenetheridge commented Mar 2, 2023

Can you change this to only include the DIE change?

Done.

This doesn't seem to have happened -- those two commits are still here. You would need to git rebase -i HEAD~3, delete the two commits interactively, and then git push --force-with-lease to update the PR.

@demerphq demerphq force-pushed the yves/fix_sub_attrs_and_spelling_issues branch from 3f7c698 to 602a6b4 Compare March 2, 2023 18:26
@demerphq
Copy link
Copy Markdown
Contributor Author

demerphq commented Mar 2, 2023

"force-with-lease"? Never used that, and it wasn't necessary FWIW. I just fat fingered it earlier and pushed to my master and not to my branch.

@demerphq
Copy link
Copy Markdown
Contributor Author

demerphq commented Mar 2, 2023

LANG=en_CA.UTF-8

Huh. I don't get it, did you add "behavior" to your dictionary? That isn't UK/Canadian spelling: https://www.btb.termiumplus.gc.ca/tpv2guides/guides/wrtps/index-eng.html?lang=eng&lettr=indx_catlog_b&page=9U6Ot1RIV-vo.html

Anyway, doesn't matter, except if you are interested in getting to the bottom of why your tests and mine for the same thing don't produce the same results.

@karenetheridge
Copy link
Copy Markdown
Member

As I said, I have the en_US, en_CA and en_UK dictionaries installed for aspell.

git push --force-with-lease is the safer version of git push --force.

@karenetheridge
Copy link
Copy Markdown
Member

Is there a simple way of demonstrating the problem this solves with a test? What is the repro case?

@haarg
Copy link
Copy Markdown
Member

haarg commented Mar 5, 2023

It would be rather annoying to replicate. The problem in MIME::Signature is a script that does the equivalent of:

$SIG{__DIE__} = sub {
    warn @_;
    exit 111;
};

This is a broken __DIE__ handler, but wasn't previously being triggered due to a core bug. Testing this would require a subprocess.

I don't really think it needs its own test.

@karenetheridge karenetheridge merged commit 47ceab0 into moose:master Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants