Skip to content
This repository

Replace Devel::BeginLift with an injected BEGIN block #39

Closed
schwern opened this Issue October 01, 2011 · 24 comments

5 participants

Michael G. Schwern Buddy Burden Chip Salzenberg Adam Lesperance Ross Attrill
Michael G. Schwern
Owner

On a hunch, I contacted Zefram to see if he had any bright ideas about replacing Devel::BeginLift. Turns out, he does.

Michael G Schwern wrote:
> I have two possibilities I wonder if you have any thoughts on.  The first is
> can I use Devel::Declare to insert a BEGIN block around the function
> definition?

You have the problem that "func" or whatever is still seen outside the
BEGIN block, and you can't take that back, but you can turn "func foo {
... }" into something like "func do { BEGIN { *foo = sub { ... }; } };".

>             Possibly ignoring for a moment the hairy bit where I have to find
> the end of the subroutine block...

That's a solved problem.  Some of the code you're invoking from
Devel-Declare is already using the necessary trick, to inject a semicolon
after the block.  The trick is that immediately after the open brace
of the block you inject a BEGIN block, the code in which invokes
B::Hooks::EndOfScope to get code run when the end of the block is seen,
and *that* code injects whatever you need after the block.  You'll need
to do that to inject the close brace of the outer BEGIN block.

I'm going to try and do that in the feature/inject_BEGIN branch.

Michael G. Schwern
Owner

Well whadya know, it works. It's going to take some work to make it play nice with Devel::Declare::MethodInstaller::Simple, but it's going to work. Adding it as a DDMS feature would be easier than trying to override the necessary bits in Method::Signatures.

Chip Salzenberg
Collaborator

+1

Buddy Burden

What still needs to be done for this issue?

Adam Lesperance
lespea commented June 12, 2012

Any updates on this? We'd like to be able to use Method::Signatures but we can't since BeginLift doesn't compile on Windows...

Chip Salzenberg
Collaborator

Why not? Maybe I can help with that. Nothing about Windows should make BeginLift impossible.

Ross Attrill

Chipdude - WRT your question about why Devel::BeginLift won't compile on Win32 - there is a bug 78105 logged for Devel::BeginLift - see https://rt.cpan.org/Public/Bug/Display.html?id=78105

Buddy Burden
Owner

While we wait for a solution to getting BeginLift working on Windows, I wonder if it wouldn't be possible to come up with a workaround. Can one of you Windows folks test to see if using the compile_at_BEGIN configuration flag makes this work? IOW, when you do:

use Method::Signatures { compile_at_BEGIN => 0 };

it won't try to load BeginLift at all. So, if this works on Windows, perhaps we could just move the BeginLift prereq from required to recommended or something like that ... which is not entirely true, but at least it would let the Windows folks install it.

Thoughts?

Ross Attrill

Hi Buddy - I have done an initial review of this.

By putting

use Method::Signature { compile_at_BEGIN => 0 };

into every test case in the distribution, then at least 42 test files pass. Without this only 8 test files pass.

Fundamental test sets like method.t pass with the workaround.

As such, right now we could force install of Method::Signatures and use it in basic situations with the above workaround.

I have only done an initial review. I will do some further analysis on this later in the week and check that I have done all of this properly. Let me know if you would like me to forward my test results so far.

Buddy Burden
Owner

... then at least 42 test files pass. Without this only 8 test files pass.

Which (if any) still fail? If we can get it to the point where all the tests pass without BeginLift, perhaps we could just change the default value for compile_at_BEGIN on Windows and call it a day ...

Ross Attrill
Buddy Burden
Owner

Some of the tests are failing because of require Devel::BeginLift statements within Method::Signatures - eg. line 522.

Yes, but line 522 is:

if( defined $name && $self->_do_compile_at_BEGIN ) {
    require Devel::BeginLift;
    Devel::BeginLift->setup_for_cv($code);
}

which means that, if compile_at_BEGIN is off, it shouldn't be firing. Hmmm ... but, of course, we made it lexically scoped, so perhaps you're in an inner scope or something. How about this: go to line 535 and change this:

# Default to on.
return 1 if !exists $hints->{METHOD_SIGNATURES_compile_at_BEGIN};

to this:

# Default to on, except on Windows.
return !($^O eq 'MSWin32') if !exists $hints->{METHOD_SIGNATURES_compile_at_BEGIN};

and then you should be able to take out all your explicit { compile_at_BEGIN => 0 } blocks, and let's see if that does it. That code should be basically (or even exactly) what I'm proposing to do anyway.

Ross Attrill
Buddy Burden
Owner

Fantastic! 50 out of 52 test files pass now.

Woohoo! We're making progress now. :-)

begin.t - by definition sounds like it isn't going to work with this workaround - skip this test on Win32?

Yeah, we'd have to do that.

I made the change above at line 535 and also changed line 480:

Oh yeah ... forgot about that one. Yep, that one would have to change as well.

./t/into.t ....................... Can't locate object method "foo" via package "Foo" at ./t/into.t line 13.

Oh ... hunh. Looks like that test subtly (and probably unintentionally) depends on the BeginLift behavior. I'd just pick up the method definition and move it above the test and then it would work fine.

So, at this point, I'd just need @schwern to pop in and let me know how he'd feel about making BeginLift an optional prereq. Unless there's some way to make it optional only on Windows ... not sure I know enough Module::Build-fu to pull that off. But I'd prefer he give me some blessing before I just make an executive decision on this one.

Michael G. Schwern
Owner

My concern with making Devel::BeginLift optional on Win32 is that a bunch of code will work, but it will work differently from the Unix code. Then once we solve the BeginLift problem it will work differently yet again! Is this worse than Method::Signatures not working at all on Win32? I'm not a Windows user, somebody who is should make that call.

Michael G. Schwern
Owner

I resurrected by branch and am hacking on it. I've got it down to four failures. I'm pretty sure I can make this debate moot and remove the need for Devel::BeginLift entirely. See https://github.com/schwern/method-signatures/tree/feature/inject_BEGIN

Michael G. Schwern
Owner

Yep, that does it. @barefootcoder would you look that pull request over please? @rjattrill and @lespea could you please knock it around on Windows? I only have one VM to play with.

Buddy Burden
Owner

Okay, I should be able to look it over thoroughly this weekend. From my quick perusal just now, though, I don't see any reason why it wouldn't work. It's a little tricky in how it interacts with Devel::Declare (or maybe DDMIS), but, then, it always was, so no major change there. :-) Looks like we're actually simplifying it somewhat, actually. I'll just have to put my brain back into that mode again. ;->

Ross Attrill

From the pull request, 50/50 tests pass on Strawberry Perl 5.14.2 built for MSWin32-x64-multi-thread. Yay!

Michael G. Schwern
Owner

@barefootcoder I oscillated between "redo large chunks of Devel::Declare::MethodInstaller::Simple" and "do whatever is necessary to play nice with them" and settled on the latter. DD is largely undocumented, so we're up a creek either way.

Buddy Burden
Owner

I oscillated between "redo large chunks of Devel::Declare::MethodInstaller::Simple" and "do whatever is necessary to play nice with them" and settled on the latter.

Yeah, I could tell, and I certainly don't blame you. I can't remember what the hell DD (or DDMIS) is doing with the sub ref, although I'm pretty sure I knew at one point last year. Maybe it's passed off to Sub::Name? Something like that, I think.

DD is largely undocumented, so we're up a creek either way.

Yeah, good point. :-/

Buddy Burden

Just released new dev version which should address this issue (finally): 20121108.0047_001, coming soon to a CPAN mirror near you. Once you can see it, I'd love for any of you Windows folks to d/l it and see if it solves all your issues.

Assuming I get to see CPANTesters reports for this version, I'll try to promote it to a real release soon.

Ross Attrill
Ross Attrill

Good on my Strawberry 5.16.2 as well. Reports sent through to CPANTesters

Buddy Burden

Fixed in version 20121201.

Buddy Burden barefootcoder closed this December 02, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.