Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upBBC Module::Install broken by 0301e899536a22752f40481d8a1d141b7a7dda82 #16300
Comments
From zefram@fysh.orgCreated by zefram@fysh.orgModule::Install fails one of its tests since core commit Perl Info |
From zefram@fysh.orgI wrote:
Turns out that Module::Install::DSL has some rather convoluted code in $ perl5.27.6 -lwe 'print "main program"; INIT { print "init block"; } BEGIN { print "begin block"; exit 0; }' That is, after a BEGIN block performs an exit(0), perl used to run This situation is not precisely the exit(0) from a CHECK block that was Module::Install::DSL does have a genuine, if not entirely respectable, However, as far as I can see there's no need at all for the module to -zefram |
From zefram@fysh.orgI have opened [rt.cpan.org #123867 (https://rt.cpan.org/Public/Bug/Display.html?id=123867) ] on the Module-Install side. -zefram |
From @LeontOn Tue, Dec 12, 2017 at 8:13 PM, Zefram <zefram@fysh.org> wrote:
Given Module::Install's rather unfortunate bundling nature, that would Leon |
|
The RT System itself - Status changed from 'new' to 'open' |
From @LeontOn Tue, Dec 12, 2017 at 11:03 PM, Leon Timmermans <fawaka@gmail.com> wrote:
Well, all 119 modules using Module::Install::DSL, Module::Install in Leon |
From @xsawyerxOn 12/13/2017 12:04 AM, Leon Timmermans wrote:
That is indeed a pain. What is the cost of reverting this commit instead? |
From @cpansproutOn Sun, 17 Dec 2017 03:06:01 -0800, xsawyerx@gmail.com wrote:
I seem to remember an old blog or list post by Michael Schwern predicting this very problem with Module::Install.
Buggy, unpredictable behaviour, unless you can memorise the list of complex rules for when exit does and does not prevent other blocks from running. -- Father Chrysostomos |
From @xsawyerxOn 12/17/2017 09:17 PM, Father Chrysostomos via RT wrote:
Are you referring to long-term or immediate effect? I'm wondering about temporary revert. |
From @cpansproutOn Mon, 18 Dec 2017 07:39:32 -0800, xsawyerx@gmail.com wrote:
I’m referring to the bugs that Zefram fixed, which went even deeper than Zefram realized when he was fixing them. So at present nobody but he can predict which blocks will trigger at what time if ‘exit’ is used in them.
That might be a good approach: revert and then reinstate after 5.28. In the mean time, a bunch of modules need to be released. -- Father Chrysostomos |
From @xsawyerxOn 12/18/2017 07:39 PM, Father Chrysostomos via RT wrote:
Exactly. A better phrasing for my question is "How problematic would it Zefram, can you please weigh in here? |
From zefram@fysh.orgSawyer X wrote:
The impact of retaining the bug for another year should be low. -zefram |
From @karenetheridgeOn Thu, 21 Dec 2017 23:03:05 -0800, zefram@fysh.org wrote:
Is kicking the can down the road a significant win? |
From @andkAlso affected (discovered by Slaven): MANWAR/Test-Strict-0.40.tar.gz -- |
From @xsawyerxOn 12/22/2017 09:02 AM, Zefram wrote:
I'm not sure there is such readiness. We are fairly behind. Considering this fix is for such a long-standing low-priority bug, |
From zefram@fysh.orgSawyer X wrote:
You mean... a deprecation warning? Technically, if we revert perl_parse() to return zero for exit(0), -zefram |
From @xsawyerxOn 12/25/2017 10:29 AM, Zefram wrote:
Yes.
Wherever we can warn here instead of die, it is better. Where we cannot, |
From zefram@fysh.orgFollowing discussion with Sawyer, where he determined that I tried adding a deprecation warning, but this turned out to cause noise. We expect to fix the bug again during the 5.29 cycle. This would consist -zefram |
From @iabynOn Wed, Dec 27, 2017 at 09:23:52PM +0000, Zefram wrote:
Since this change has been reverted for 5.28, I presume I can remove this -- |
From @xsawyerxOn 04/20/2018 12:36 AM, Dave Mitchell wrote:
Yup. |
From @khwilliamsonOn Fri, 20 Apr 2018 03:54:26 -0700, xsawyerx@gmail.com wrote:
Pinging this ticket -- |
From @karenetheridgeOn Thu, 28 Mar 2019 19:33:04 -0700, khw wrote:
I've been re-releasing all the modules I am aware of (that I have the ability to) that use Module::Install::DSL; I think the upper portions of the cpan river are now clean. Are we able to assess the remaining impact to the river, and perhaps establish the final list of distributions that are still of concern? |
From @jkeenanOn Sat, 30 Mar 2019 03:54:10 GMT, ether wrote:
For what it's worth ... I went to this (possibly inaccurate) list of CPAN distributions which are dependent upon Module::Install: http://deps.cpantesters.org/depended-on-by.pl?dist=Module-Install I extracted the 127 top-level distros, then excluded all distros beginning with Acme, Bundle or Task. That left 114 distros, which I attempted to install against Perl 5 blead (commit 930ded6, Apr 2 2019) on FreeBSD-11.2 (threaded) using cpanm as the installer. (I.e., the same approach I take toward monthly testing of the "CPAN-River-3000".) Certain modules failed to install for reasons that were expected given that they, or their prerequisites, do not get a PASS grade during the monthly CPAN-River-3000 process. Other modules failed to install because of upstream breakage in blead. However, I couldn't identify any distributions which failed due to Module::Install itself. Thank you very much. |
From @xsawyerxOn 4/3/19 2:53 PM, James E Keenan via RT wrote:
Can you share this list, please? |
From @jkeenanOn 4/7/19 1:43 AM, Sawyer X wrote:
Attached. |
From @jkeenanModule::Install |
From @jkeenanOn Wed, 03 Apr 2019 11:53:39 GMT, jkeenan wrote:
CPANtesters currently all green for this distro. http://matrix.cpantesters.org/?dist=Module-Install;perl=5.29.10;reports=1 Can this ticket be marked Resolved? Thank you very much. -- |
From @iabynOn Sat, Apr 13, 2019 at 07:02:10PM -0700, James E Keenan via RT wrote:
Not really. The change in blead, v5.27.6-180-g0301e89953, that broke M::I I'm going to move this from 5.30.0 blocker to 5.32.0 blocker -- |
From @karenetheridgeOn Tue, 23 Apr 2019 08:03:46 -0700, davem wrote:
Since nothing on cpan seems to be affected anymore, this should be safe to resolve |
From @jkeenanOn Fri, 26 Apr 2019 04:05:42 GMT, ether wrote:
I have created the following branch: smoke-me/jkeenan/rt-132577-module-install ... in which the reverted portion of the original commit is re-applied. This can facilitate CPAN smoking (which we will have to arrange). Thank you very much. -- |
The relevant branch is now called commit 429da47739b54c684c7c3b6b37de386c3ae59261
Author: James E Keenan <jkeenan@cpan.org>
AuthorDate: Fri May 31 08:21:16 2019 -0400
Commit: James E Keenan <jkeenan@cpan.org>
CommitDate: Tue Dec 17 15:13:31 2019 -0500
Revert "revert perl_run() 0 -> 256 return mapping"
This reverts commit 9f9606382c45ba5e9600dddf96abfe956762af99.
This reversion is for the purpose of seeing what CPAN breakage results.
In RT 132577, Dave Mitchell wrote:
"The change in blead, v5.27.6-180-g0301e89953, that broke M::I [was]
reverted by v5.28.0-RC1-15-g9f9606382c, due to the M::I breakage. But in
theory we would like to reapply athe change at some point in the future
when a fixed M::I has been included in all the distributions which
include it and are affected by it. I'm going to move this from 5.30.0
blocker to 5.32.0 blocker."
Signed-off-by: James E Keenan <jkeenan@cpan.org>
diff --git a/perl.c b/perl.c
index 70424cdbab..3e39fe35ed 100644
--- a/perl.c
+++ b/perl.c
@@ -2686,7 +2686,7 @@ int
perl_run(pTHXx)
{
I32 oldscope;
- int ret = 0;
+ int ret = 0, exit_called = 0;
dJMPENV;
PERL_ARGS_ASSERT_PERL_RUN;
@@ -2707,8 +2707,10 @@ perl_run(pTHXx)
case 0: /* normal completion */
redo_body:
run_body(oldscope);
- /* FALLTHROUGH */
+ goto handle_exit;
case 2: /* my_exit() */
+ exit_called = 1;
+ handle_exit:
while (PL_scopestack_ix > oldscope)
LEAVE;
FREETMPS;
@@ -2722,7 +2724,12 @@ perl_run(pTHXx)
if (PerlEnv_getenv("PERL_DEBUG_MSTATS"))
dump_mstats("after execution: ");
#endif
- ret = STATUS_EXIT;
+ if (exit_called) {
+ ret = STATUS_EXIT;
+ if (ret == 0) ret = 0x100;
+ } else {
+ ret = 0;
+ }
break;
case 3:
if (PL_restartop) {On FreeBSD-11, I installed this branch, installed Because those distros have prerequisites in addition to Module-Install, more than 900 distros were actually installed. I examined the Inference: Right now would be an appropriate time to re-apply the patch provided originally by Zefram in Dec 2017 (0301e89). If we were to re-apply it before Friday's anticipated release of perl-5.31.7, we would get CPAN-River-3000 and CPANtesters results. If those results were unfavorable, we could roll back before perl-5.31.8 and code-freeze in January 2020. @iabyn, @tonycoz, @xsawyerx, @zefram, @karenetheridge, et al. please comment. Thank you very much. |
|
Dependency lists are not appropriate to figure out the impact of Module::Install bugs. Module::Install gets bundled with the release, and normally won't be listed as a prerequisite. To find the distributions impacted by this, you need to search for Makefile.PL files using inc::Module::Install::DSL. There are currently 110 distributions that could be impacted by this, and I'm certain that most of them have not had new releases in the last two years. |
Thanks for that clarification. Is there any way you could provide me with a list of those distros in a form suitable for feeding to I could then feed them into the already installed branch perl. Thank you very much. |
|
All I have is the grep.cpan.me link that I provided. |
Not yet in Thank you very much. |
|
cpanm accepts this form of release to install if you include the .tar.gz extension. |
|
I'm going to remove the tag of BBC on this case since the commit originally raised was reverted. At this point, the ticket sounds like us discussing if we should put the bug fix back in. Can someone recommend a new Subject for this ticket? |
|
If I'm understanding the issue, the discussion seems to be if we're going to fix #1537 Which jim has provided a branch here at: https://github.com/Perl/perl5/tree/smoke-me/jkeenan/gh-16300-module-install. Looking closer at #1537, though I think the change is already in blead as 625e8b0 and staged for 5.32. @jkeenan @tonycoz Am I groking this correctly? |
|
@toddr, no that's the (re-)reversion of the fix. That comment in perl.c still has: |
|
@tonycoz does this mean the fix is not in blead anymore? Considering this list, my main concerns are whether they are still being maintained and whether the authors are able to produce new versions with the fixes. Considering these modules haven't had a release in a while is less concerning to me, especially due to the fact that the authors are highly responsive (at least the biggest releaser on the list - @andk). Any strong objections? |
|
The majority of the dists are from ADAMK, who is not active at all, but he has given blanket permission for some people to take over his modules. |
Correct, the code in perl.c: I have no objections to re-reinstating the fix. |
I have comaint on most of them, and have been doing releases for distributions higher on the cpan river, or when someone squawks (e.g. filing an RT ticket). Many of these distributions have no reverse dependencies at all though so I have not been feeling it is urgent to make updates to them. |
|
Yesterday I (re-)created branch I then installed that branch on FreeBSD-11, installed Rough overall conclusions:
Now, since I've been staring at this for 2 days and am fatigued, there's a good chance that my analysis may have missed the point. But it does appear that re-applying the patch gives good results and hence should be applied to blead (though perhaps not for 5.32). Someone else should try to confirm my findings. Thank you very much. |
@karenetheridge, Am I correct that if an up to date Module::Install::DSL is installed, then the file inc/Module/Install/DSL.pm located with the CPAN distro is not used? If so that reduces the scope of this problem, right? |
Not really. In the end, the local version takes over: https://metacpan.org/release/Module-Install/source/lib/Module/Install.pm#L3-17 |
I then repeated this Thank you very much. |
|
I would like to reintroduce @tonycoz's patch. Any objections? |
|
I'm not certain that people have been looking at the correct commits. 0301e89 is the original commit that broke modules using Module::Install::DSL. This was later adjusted in two different commits. 9f96063 is a partial revert by Dave Mitchell to fix issues with embedded perl. This is related to #16565, not this issue. This is the commit @jkeenan was testing reverting. Reverting this patch will have no impact on Module::Install::DSL. 857320c is a different partial revert by Zefram to fix the issues with Module::Install::DSL. Reverting this commit will break all users of old Module::Install::DSL, which is all users of Module::Install::DSL. |
|
What needs to be done here, if anything, to at least remove this from being a 5.32 blocker? |
|
I don't think it needs to be a blocker. Applying the full "fix" here would still break a number of modules, for very little benefit. It isn't hard to fix the modules, but it won't happen on a short time frame. |
|
It sounds like there is nothing actionable here, so we're ok to close the case? |
|
Considering the length of this ticket, I'd suggest we open a ticket for the issue that's left and neither this new ticket nor this ticket should be blockers. (But to be honest, at this point, I've lost what issue remains...) |
Migrated from rt.perl.org#132577 (status was 'open')
Searchable as RT132577$