Bug #49350 for Module-Build: new_from_context inlines 'Build.PL'

Tue Sep 01 21:57:17 2009 CRAKRJACK [...] cpan.org - Ticket created

Subject:

new_from_context inlines 'Build.PL'

->new_from_context() inlines Build.PL, which means that if a Build script elects to terminate early, that causes the perl environment further up the chain to exit as well. For example, Params::Classify 0.007 has an "exit 0" at the end of it's Build.PL, which ensures that if ->new_from_context is ever called on that module, it will exit early. Since Build.PL is touted as a "script", I think it should be treated as such as much as possible, and we should not let the product of a Build.PL pollute the environment above that, whether that's M::B, CPANPLUS, or whatever.

Tue Sep 01 22:24:07 2009 CRAKRJACK [...] cpan.org - Correspondence added

The partner to this bug in Params::Classify is here; https://rt.cpan.org/Ticket/Display.html?id=49351 For now, I am using the following workaround. I hope it doesnt have any other unintended side-effects :-) ## hackery for https://rt.cpan.org/Ticket/Display.html?id=49350 ## and https://rt.cpan.org/Ticket/Display.html?id=49351 my $old_new_from_context = \&Module::Build::Base::new_from_context; local *Module::Build::Base::new_from_context = sub { local *CORE::GLOBAL::exit = sub { return @_ }; $old_new_from_context->(@_); };

Tue Sep 01 22:24:08 2009 CRAKRJACK [...] cpan.org - Status changed from 'new' to 'open'

Tue Sep 01 22:25:21 2009 CRAKRJACK [...] cpan.org - Reference to https://rt.cpan.org/Ticket/Display.html?id=49351 added

Tue Sep 01 22:50:42 2009 dagolden [...] cpan.org - Correspondence added

new_from_context as implemented is really no longer necessary. After discussion on #toolchain with ewilhelm, our thought is to deprecate new_from_context and add a load_from_build_pl() class method that runs Build.PL in a separate process and then resumes it the same way the Build script would. (Lowering severity to "normal" as this is not a release-blocking bug)

Tue Sep 01 22:50:55 2009 dagolden [...] cpan.org - Severity Important changed to Normal

Tue Sep 01 23:50:52 2009 CRAKRJACK [...] cpan.org - Correspondence added

On Tue Sep 01 22:50:42 2009, DAGOLDEN wrote: Show quoted text

> new_from_context as implemented is really no longer necessary. After > discussion on #toolchain with ewilhelm, our thought is to deprecate > new_from_context

Thanks... so if new_from_context is depreciating, I guess CPANPLUS should stop using it. Would you be willing to file an RT over there for this? You'd probably be better at explaining how to upgrade that code than I... Cheers, Tyler

Wed Sep 02 05:34:04 2009 dagolden [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #49350] new_from_context inlines 'Build.PL'
Date:	Wed, 2 Sep 2009 05:33:26 -0400
To:	bug-Module-Build [...] rt.cpan.org
From:	David Golden <dagolden [...] cpan.org>

On Tue, Sep 1, 2009 at 11:50 PM, CRAKRJACK via RT<bug-Module-Build@rt.cpan.org> wrote: Show quoted text

> Thanks... so if new_from_context is depreciating, I guess CPANPLUS > should stop using it. Would you be willing to file an RT over there for > this? You'd probably be better at explaining how to upgrade that code > than I...

CPANPLUS::Dist::Build no longer uses new_from_context (since about version 0.08 in March 2009).

Wed Sep 02 09:47:24 2009 kwilliams [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #49350] new_from_context inlines 'Build.PL'
Date:	Wed, 2 Sep 2009 08:47:06 -0500
To:	bug-Module-Build [...] rt.cpan.org
From:	Ken Williams <kwilliams [...] cpan.org>

On Tue, Sep 1, 2009 at 9:50 PM, David Golden via RT<bug-Module-Build@rt.cpan.org> wrote: Show quoted text

> Queue: Module-Build > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=49350 > > > new_from_context as implemented is really no longer necessary. After > discussion on #toolchain with ewilhelm, our thought is to deprecate > new_from_context and add a load_from_build_pl() class method that runs > Build.PL in a separate process and then resumes it the same way the > Build script would.

Seems like a good idea. I'd even be in favor of keeping the method name the same and just swapping out the implementation. -Ken

Wed Sep 02 10:54:13 2009 dagolden [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #49350] new_from_context inlines 'Build.PL'
Date:	Wed, 2 Sep 2009 10:53:35 -0400
To:	bug-Module-Build [...] rt.cpan.org
From:	David Golden <dagolden [...] cpan.org>

On Wed, Sep 2, 2009 at 9:47 AM, Ken Williams via RT<bug-Module-Build@rt.cpan.org> wrote: Show quoted text

> Seems like a good idea. I'd even be in favor of keeping the method > name the same and just swapping out the implementation.

We we're being cautious because of a scary comments you put in about why it needed to happen that way. (Which we think are now stale given other implementation changes, particularly around preserving things added to @INC.) So if you support the change, I'm all for changing the guts of new_from_context. David

Wed Sep 02 12:21:11 2009 kwilliams [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #49350] new_from_context inlines 'Build.PL'
Date:	Wed, 2 Sep 2009 11:20:50 -0500
To:	bug-Module-Build [...] rt.cpan.org
From:	Ken Williams <kwilliams [...] cpan.org>

On Wed, Sep 2, 2009 at 9:54 AM, David Golden via RT<bug-Module-Build@rt.cpan.org> wrote: Show quoted text

> We we're being cautious because of a scary comments you put in about > why it needed to happen that way. (Which we think are now stale given > other implementation changes, particularly around preserving things > added to @INC.)

Reading that comment, I'm not sure what I meant by "make sure that the environment is the same". All the cases I can think of should be fine. So I say go for it. -Ken

Wed Sep 02 12:46:28 2009 dagolden [...] cpan.org - Correspondence added

Patched in trunk

Wed Sep 02 12:46:32 2009 dagolden [...] cpan.org - Status changed from 'open' to 'patched'

Mon Dec 21 13:51:33 2009 dagolden [...] cpan.org - Status changed from 'patched' to 'resolved'

Mon Dec 21 13:51:34 2009 dagolden [...] cpan.org - Fixed in 0.36 added