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.
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
Blead Breaks CPAN: Class::Std #16436
Comments
From carlos@carlosguevara.comIt looks like blead broke Class::Std. The install now hangs with CPU Completed install after killing the hung process: |
From @demerphqOn 24 Feb 2018 09:46, "Carlos Guevara" <perlbug-followup@perl.org> wrote: # New Ticket Created by Carlos Guevara It looks like blead broke Class::Std. The install now hangs with CPU Completed install after killing the hung process: Thanks, this looks like fallout from a fix I pushed yesterday. I will Yves |
The RT System itself - Status changed from 'new' to 'open' |
From zefram@fysh.orgdemerphq wrote:
Confirmed: it bisects to commit c99363a -zefram |
From @demerphqOn 24 February 2018 at 03:44, Zefram <zefram@fysh.org> wrote:
Which means its Class::Std's fault for not implementing can properly. Nevertheless I will find a fix. Thanks for verifying and the analysis Zefram. Yves -- |
From @demerphqOn 24 February 2018 at 03:46, demerphq <demerphq@gmail.com> wrote:
I have created a pull request against Class::Std to fix this. I also will push a patch to fix this in Carp.pm as well. Yves -- |
From @demerphqOn 24 February 2018 at 12:32, demerphq <demerphq@gmail.com> wrote:
Fixed in 17157c4 Yves -- |
From @cpansproutOn Sat, 24 Feb 2018 04:16:48 -0800, demerphq wrote:
I don’t like the current state of the code in Carp.pm, but I can’t say I have a better suggestion off the top of my head. I have not thought it through thoroughly yet. Let me just outline my concerns, before I forget them, and later if I have time I might come up with a patch: if ($in_recurse || do{ local $in_recurse = 1; $pack->can("((") }) { This will only work on newer perls (5.16+ iirc), since older ones used (), not ((. And I think even current perl allows XS modules to register overloading via just (), without ((. I need to check. ‘can’ is really not the right thing to use. Overriding ‘can’ makes sense in the presence of AUTOLOAD, but since overloading bypassing AUTOLOAD, it should be bypassing ‘can‘ as well. This is why overload.pm implements its own ‘can’. Perhaps overload.pm should be loaded unconditionally when Carp is loaded (it is more lightweight than Carp, after all). Then theoretically one could use overload::Overloaded, but unfortunately old versions have the same ‘can’ bug that will cause the recursion. Maybe calling overload::mycan directly is the solution. This is a real can of worms. -- Father Chrysostomos |
From @demerphqOn 25 Feb 2018 02:01, "Father Chrysostomos via RT" < On Sat, 24 Feb 2018 04:16:48 -0800, demerphq wrote:
I don’t like the current state of the code in Carp.pm, but I can’t say I if ($in_recurse || do{ local $in_recurse = 1; $pack->can("((") }) { This will only work on newer perls (5.16+ iirc), since older ones used (), We can add a check for () as well. ‘can’ is really not the right thing to use. Can you explain why? Since overloads are inherited it seems there is no Overriding ‘can’ makes sense in the presence of AUTOLOAD, but since If Carp unilaterally loads overload we don't need to use can at all. The I already suggested that this policy was counter productive and the only If you no longer care about loading overload then the patch becomes trivial. This is a real can of worms. A real mycan of worms? Yves -- Father Chrysostomos via perlbug: queue: perl5 status: open |
From @cpansproutOn Sat, 24 Feb 2018 15:51:03 -0800, demerphq wrote:
The text that follows was meant to be the explanation. can and AUTOLOAD go together. Since overloading does not use AUTOLOAD, using can to detect overloading can be problematic.
Not just recursion. Look at the test I added in e6bb0a4.
Well, since Carp is used *everywhere*, I believe it should remain as lightweight as possible. Now that I’ve had a chance to look at the code in more detail, I see that it only occurs for stack traces. I am loath to force another module to be loaded, considering that many quick scripts never use stack traces. But loading overload.pm on demand in that case is itself problematic, since Carp is often called when things are in an inconsistent state. You shouldn’t risk dying when trying to load something in order to report an error. (Yes, this actually happened. That’s why Carp::Heavy was eliminated.) That said, I’m still on the fence about it. I know Zefram cares about this sort of thing, too, so I would ask him which is the best approach: • Go ahead and load overload.pm unconditionally. It’s already much smaller than Carp. • Copy & paste mycan into Carp. It’s just seven lines or so (but we need two versions depending on the perl version).
s/v//r is my answer. :-) -- Father Chrysostomos |
From @demerphqOn 25 February 2018 at 02:40, Father Chrysostomos via RT
Yeah I read that text, but I am still not seeing the connection the way you do. So, it makes sense to override can if you use AUTOLOAD. And overload's dont trigger AUTOLOAD. That is clear. But, it doesnt seem to obvious to me that that means that we shouldnt It seems like you are saying something like: "Because the main reason Which doesn't seem logical to me, even if perhaps it is practical. Why not simply say "We should document that can() overrides need to be To me that there are buggy can() implementations out there is a
Thanks, i will look.
Overload is widely used as well. I also am not that sympathetic to the
I bet you couldn't even see the extra module load in normal benchmarks
I totally agree. No argument there at all. See
Isnt mycan slow compared to the internals version? Maybe in modern (05:57:51) Zefram: annoyingly, the exact thing you want is available That way BOTH carp and overload could use it in newer perls. I don't really want to see Carp slowed down massively just because we
Heh. BTW, going forward there is another solution, expose unoverloaded It wont help older builds, but in the long run it would avoid needing Yves -- |
From @demerphqOn 25 February 2018 at 02:40, Father Chrysostomos via RT
Fixed in 687fa12 Yves -- |
From @cpansproutOn Sat, 24 Feb 2018 18:42:22 -0800, demerphq wrote:
The way I see it, the fact that overloading uses weird-named subs is just an artefact of the implementation. Overloading can be inherited, but it does not consist of method calls (though they may trigger methods), as shown by the fact that AUTOLOAD is not respected, and by the fact that the fallback value (a scalar) is also inherited. In fact, overloading originally did not share internal code with method calls at all; that was just the most convenient way to get inheritance to work. I don’t think we should blur this distinction and call ‘can’, because it puts the burden on too many programmers to ensure that their ‘can’ methods behave a way that ‘can’ was not designed for to begin with. I do *not* consider what you call ‘buggy can() implementations’ to be buggy.
Based on your explanation above, it has a ‘buggy can() implementation’.
I’m not entirely opposed to loading overload.pm unconditionally. I just want it to be justified, and not just convenient because it saves two lines of code. In this instance, it is justifiable.
It already exists. It is called UNIVERSAL::can (which we can use, since we control it; even though it was designed for methods, it happens to work for this case). Unfortunately, there are (buggy) CPAN modules that diddle with it. Fortunately, Carp already has (partial) code to detect whether that has happened. (Note: There is a comment in overload.pm about ‘real’ can() leaving stubs. I think we need to get to the bottom of that before trying UNIVERSAL::can. Leaving a "()" stub behind may break things, since that’s where the fallback value is stored.) -- Father Chrysostomos |
From @cpansproutOn Sat, 24 Feb 2018 22:24:01 -0800, sprout wrote:
Never mind that note. Since overload::Overloaded used to call ->can, it obviously doesn’t matter for this case. -- Father Chrysostomos |
From carlos@carlosguevara.comThis is a bug report for perl from Carlos Guevara <carlos@carlosguevara.com>, It looks like blead broke Test::MockObject: Flags: Site configuration information for perl 5.27.9: Configured by root at Sun Feb 25 19:21:55 UTC 2018. Summary of my perl5 (revision 5 version 27 subversion 9) configuration: @INC for perl 5.27.9: Environment for perl 5.27.9: |
From @dur-randirOn Sun, 25 Feb 2018 11:39:39 -0800, carlos@carlosguevara.com wrote:
Bisect points to the same commit c99363a as in https://rt-archive.perl.org/perl5/Ticket/Display.html?id=132902 |
The RT System itself - Status changed from 'new' to 'open' |
From zefram@fysh.orgCarlos Guevara wrote:
It's down to Carp. Bisects to commit -zefram |
From @cpansproutOn Sat, 24 Feb 2018 18:42:22 -0800, demerphq wrote:
I’ve just submitted a patch against the UNIVERSAL::can module, making exactly that point: https://rt.cpan.org/Ticket/Display.html?id=124585 (As I mentioned in another message, UNIVERSAL::can is that exposure of the internal function.) -- Father Chrysostomos |
From @demerphqOn 25 February 2018 at 21:28, Father Chrysostomos via RT
I am really struggling to understand what you think is right. So first off, there are *two* UNIVERSAL::can()'s involved, the *real* That module thinks that is "buggy" to call UNIVERSAL::can() as a In one of the other posts in this thread, you argue that can() You also have said you think that UNIVERSAL::can() should not be used I do not see how it is right that a patch to a module not in core is It seems to me that we need an exposure of UNIVERSAL::can() that does I am right now quite baffled. Honestly at this point I think the right thing is to make Carp load Yves -- |
From @cpansproutOn Sun, 25 Feb 2018 12:19:07 -0800, zefram@fysh.org wrote:
Fixed in 4efd247. -- Father Chrysostomos |
@cpansprout - Status changed from 'open' to 'resolved' |
From @cpansproutOn Sat, 24 Feb 2018 22:24:01 -0800, sprout wrote:
I’ve done this in commit 4efd247, because it came up in another ticket. The use of ->can did cause CPAN breakage. -- Father Chrysostomos |
From @cpansproutThese two tickets should not have been merged. They are related, but they are two different issues, one caused by the fix for the other. -- Father Chrysostomos |
From [Unknown Contact. See original ticket]These two tickets should not have been merged. They are related, but they are two different issues, one caused by the fix for the other. -- Father Chrysostomos |
From @demerphqOn 26 February 2018 at 03:22, Father Chrysostomos via RT
Personally i find this very irritating that you just push patches On the same basis of unilateral imposition of ones own views without /me irritated that you aren't engaging properly. cheers, -- |
From @cpansproutOn Sun, 25 Feb 2018 16:14:44 -0800, demerphq wrote:
Well, I can be enigmatic. :-)
I agree with you here.
Regular modules’ ‘can’ implementations should not have to care about them. Modules that meddle with UNIVERSAL::can need to bend over backwards to make sure they are not breaking things. With great power (overriding UNIVERSAL) comes great responsibility.
I don’t think $some_object->can should be used. UNIVERSAL::can($object, ...) should be fine. I think this way, not because I think that a method designed to look for methods is a good fit for overloading, but because in this particular instance (core’s UNIVERSAL::can) it happens to do exactly the right thing. And, in being a stable interface, we can depend on it.
If merely loading some module (and a not-so-unpopular module) will break the core, and it didn’t in the previous Perl version, then what should be someone else’s problem (a broken override) becomes *our* problem. You may disagree with that, but that’s fine. I’m willing to bend over backwards to get things working. You don’t have to.
We can’t stop buggy modules from overriding things. But we can patch the one buggy module that currently does it to ‘can’.
Funny thing, I used the number 0xbaff1ed_bee in one of the tests.
I fixed this a different way before I read your message. I, too, intend to move on to more interesting things, after I’ve checked one other thing that may be broken in Carp. -- Father Chrysostomos |
From @demerphqOn 26 February 2018 at 05:02, Father Chrysostomos via RT
Indeed. :-) Perhaps a bit too close to the machine version though. ;-)
\o/
I definitely agree with the latter two points. I am not entirely
I agree with this.
I'm willing to bend over backwards as well, or I wouldn't have tried [ And really, that paragraph seems pretty close to suggesting I am The place I disagree is *not* about avoiding breakage, but rather In particular I feel that we are going through huge contortions and It feels like to me we have possibly already pushed past the point So for instance, if I was able to show that adding a "use overload;"
I think anything that overrides an Internals:: (or equivalent)
:-) [ Interestingly some time back I was told by an academic that not
But that is my point, this patch sequence has had too many patches and
Please lets resolve this discussion before you move on. I appreciate your work and opinion, and I apologize if my attempts to So I feel like we really aught to address that point and come to a Which is why I cc'ed Sawyer on my last post, and why I am cc'ing him here. Yves -- |
From @cpansproutOn Sun, 25 Feb 2018 20:28:02 -0800, demerphq wrote:
Sorry if it came across that way. I know that in the past (for example, when it came to hash randomization), you did not think it was p5p’s responsibility to patch all the broken modules, whereas I thought we should patch as many as we could. In order words, I’m trying to concede to your view, pointing out that the burden falls upon those who think differently.
If you look at my latest patch, namely, 4efd247, you will see that it actually does load overload.pm on startup, iff UNIVERSAL::can is already loaded. Otherwise it just uses Perl’s UNIVERSAL::can, which suffices.
I think the same is true of UNIVERSAL (but we already have existing modules to cope with).
To use the new function would add more conditions to Carp. The logic I have already added: +BEGIN { will still have to stay. So I see no need to proceed that way.
Oh, not at all. I actually really enjoy this kind of bug fixing, the digital counterpart to tightrope walking. Thank you for the opportunity.
Bug #132910, which was erroneously merged into this ticket, would have happened, but we would not have noticed, because it would only have happened with a new Carp on perl 5.14 or earlier.
I think you are proposing that we load overload.pm up front and just use overload::StrVal without checking for overloading. Am I right? When it comes to recent versions of overload.pm (1.18+; perl 5.16), that will just work. Version 1.01–1.17 (perl 5.8.1 to 5.14) load Scalar::Util at run time. *That* is a serious problem. (In fact, Carp currently loading overload.pm at run time is problematic, the ‘other thing that may be broken’ that I mentioned above.) Carp is sometimes called from $SIG{__DIE__} handlers, which may trigger due to a syntax error. After a syntax error, BEGIN blocks won’t run. (This actually happened. See commit 018c7c8.) I have written a test that fails in current blead because of this. That means: Alternatively, for perl 5.10.1 to 5.14, we can copy the more recent overload::StrVal into Carp. It consists of: sub { no overloading; "$_[0]" }. You can’t get much faster than that. In which case we might as well use it also in current blead and avoid extra conditions. We don’t need to load overload.pm at all in perl 5.10.1+. Did you know that the Romanian word for carp is crap (the fish, not the verb)? As for the stash vivification test, I know why Zefram avoided vivifying utf8::. It broke things on ancient perl versions. That makes sense. But overload? I think the reason is that Carp is so ubiquitous that you don’t want it leaving droppings lying around that it might not even use, every time anything loads it. But loading overloading.pm vivifies the overload stash. I think we have to live with it. Now, for perl 5.8.1 to 5.10.0, we have (1) an overload::StrVal that loads Scalar::Util at run time, and (2) an overload::Overloaded which for Carp is unusable, since it calls ->can. Either we go ahead and load Scalar::Util when loading Carp--which I don’t like, but I can be persuaded--, or we avoid overload::StrVal and do it the hard way, writing a pure-Perl StrVal. For Perl 5.8.0 (yes, I think we should support 5.8.0 still), overload::StrVal has the worst implementation yet. It unconditionally blesses any ref passed to it, which may well cause real problems. Use of overload::StrVal isn’t all that common, compared to Carp. Carp will make it common and start blessing people’s references left and right when generating a stack trace. Avoid this old StrVal at all costs! It looks as though the simplest approach to all this is: -- Father Chrysostomos |
From @cpansproutOn Sun, 25 Feb 2018 23:50:45 -0800, sprout wrote:
To make things worse, the old pure-Perl Scalar::Util::refaddr also unconditionally blesses references. It looks almost the same as the old StrVal, so it was probably copied, pasted and tweaked. That means if we were to use overload::StrVal from 5.8.1 to 5.10.0 (which uses refaddr) we would have to make sure the XS Scalar::Util was loaded. That’s getting too complicated.
All the more necessary, considering what I just discovered about refaddr. -- Father Chrysostomos |
From zefram@fysh.orgTicket [perl #132910] (BBC Test::MockObject) has in RT been merged into -zefram |
From @cpansproutOn Sun, 25 Feb 2018 23:50:45 -0800, sprout wrote:
Please review the patch on the sprout/carp-strval branch. -- Father Chrysostomos |
From @jkeenanOn 02/26/2018 03:03 AM, Zefram wrote:
I'm sorry for misunderstanding the problem. Is there any corrective Thank you very much. |
From @demerphqOn 26 February 2018 at 08:50, Father Chrysostomos via RT
For the record, I don't feel that that accurately portrays my views. I There were a lot of people claiming back then that their code was not Many of the modules involved were "baking in" specific DD dumps for The big issue here and then was that if people write buggy code that a
Ok.
Personally due to the nature of UNIVERSAL being the base class for all
Why? If we simply replace that with; use overload (); then we do not need to call UNIVERSAL::can() at all.
Heh, ok. Ill take that at face value. :-)
I dont follow. If we simply did use overload (); then we could replace this logic: # overload uses the presence of a special with a simple: return overload::StrVal($arg);
Yep.
Ok, I see.
I dont see any of this as an issue.
Doesnt this also suffer the problem you mentioned of loading code at run time?
No. How appropriate.
Would some of this be solved by moving overload.pm to dist and
Ok, well, if the patches you have pushed work then I am fine to drop Yves -- |
From @demerphqOn 26 February 2018 at 10:33, Father Chrysostomos via RT
It is definitely cleaner than what we have now. My only complaint is I hate this: qw 'UNIVERSAL isa' :-) Anyway, looks much saner to me than what we do now. cheers, -- |
From @cpansproutOn Mon, 26 Feb 2018 23:23:40 -0800, demerphq wrote:
You’re right.
Not if we load overloading.pm up front, which my patch does. I think it’s unavoidable.
Please, no. That would be a nightmare. We would end up playing this same compatibility game with overload.pm. -- Father Chrysostomos |
From @cpansproutOn Mon, 26 Feb 2018 23:51:01 -0800, demerphq wrote:
Would you prefer qw FUNIVERSAL isaF? It has FUN in it. :-)
Thank you. I’ve pushed it as 5c8d107 (with qw/.../). -- Father Chrysostomos |
From @bulk88On Tue, 27 Feb 2018 09:16:44 -0800, sprout wrote:
Commit 5c8d fails badly on Windows for me. C:\p525\src\win32>cd ..\t & perl harness -v ../dist/Carp/t/stack_after_err.t # '' # at t/stack_after_err.t line 69. # at t/stack_after_err.t line 69. Test Summary Report ../dist/Carp/t/stack_after_err.t (Wstat: 1024 Tests: 4 Failed: 4) C:\p525\src\win32> -- |
From @bulk88On Tue, 27 Feb 2018 20:04:14 -0800, bulk88 wrote:
There are no quotes in the cmd line to the process Command line: C:\p525\src\t\perl.exe -e but Carp.t which has identical code (or almost?) does put "s in the command line foe -e's arg. -- |
From @bulk88On Tue, 27 Feb 2018 20:21:24 -0800, bulk88 wrote:
If Win32's open3 sees a " char anywhere in the command line, it processes/splits/dont have time to research more, the command line differently. -- |
From @cpansproutOn Tue, 27 Feb 2018 20:21:24 -0800, bulk88 wrote:
I suspect IPC::Open3 (or something it calls) is adding the quotation marks if the argument has none. Would it be feasible to feed the program to stdin and drop -e? -- Father Chrysostomos |
From @demerphqOn 28 February 2018 at 06:41, Father Chrysostomos via RT
I just pushed: commit 01d4cc0 rework Carp/t/stack_after_err.t to not use perl -e cheers, -- |
From @cpansproutOn Wed, 28 Feb 2018 07:03:52 -0800, demerphq wrote:
Thank you. -- Father Chrysostomos |
From @demerphqOn 28 February 2018 at 16:03, demerphq <demerphq@gmail.com> wrote:
Bulk88 can you confirm this patch fixed the win32 build issue? Yves -- |
From @bulk88On Thu, 01 Mar 2018 00:22:19 -0800, demerphq wrote:
works for me C:\perl521\src\win32>cd ..\t & perl harness -v ../dist/Carp/t/stack_after_err. C:\perl521\src\win32> -- |
From @demerphqOn 1 March 2018 at 09:47, bulk88 via RT <perlbug-followup@perl.org> wrote:
Great. Thanks for the report! cheers, -- |
Migrated from rt.perl.org#132902 (status was 'resolved')
Searchable as RT132902$
The text was updated successfully, but these errors were encountered: