BBC: Blead Breaks Sub::Exporter #21269
Comments
|
This is a bug in Sub::Exporter's test suite, and is fixed by rjbs/Sub-Exporter#17 |
|
Happy Birthday @rjbs. |
Bisecting shows that this failure shows up in blead at this commit: ... which is consistent with @demerphq's explanation. I suspect that since this commit went into blead just two days ago, we'll see other CPAN breakage connected to that commit and have to file tickets for other distros upstream. |
jkeenan
added
BBC
Yep. Very very likely. This patch is specifically intended to detect people doing stuff that wouldn't be detected prior to it. |
|
Hey, thanks! I'm out of town on business, but this all looks like Yves is probably right, and I'll dig into it pretty soon. |
|
Also affected: LEONT/CPAN-Meta-Check-0.017.tar.gz |
|
Re CPAN::Meta::Check: |
|
Re: CPAN::Meta::Check, the offending line of code is I guess this is intended to be a version Check, but since it's not a number it gets treated as an argument to import. We could probably handle this case more gracefully in UNIVERSAL::import(), but I'm not sure if we should, it seems preferable to keep it very simple. But perhaps we could add a bespoke error for this case to make it easier to understand what is going on. Again, given the highly experienced nature of the author of this code it seems likely we will discover quite a bit more code where people thought they were doing something that were actually no-ops in disguise. |
|
The impact of this breakage is massive. On both FreeBSD and Linux, I built perls from the v5.39.1 tag, attempted to install Task::CPAN::Reporter against them and generate CPANtesters reports from a list of 500 modules. Because Sub::Exporter is a prereq to Test::Reporter::Transport::Metabase, even though the modules were actually tested, no reports were sent and all were marked We should consider reverting the breaking commit until we can better assess the impact of the change. |
That sounds like an over-reaction to me personally. When @rjbs can roll a new release of Sub-Exporter things will go back to normal. Presumably if he wasn't on holiday this would be fixed already. We can wait a few days IMO. Just force install Sub::Exporter for now. The only thing broken there is a test that doesn't quite do what he thought it did. |
|
Looks like Moose is also hit by this # Failed test '... got the right list of applicable methods for Foo'
# at t/cmop/methods.t line 211.
# Structures begin differing at:
# $got->[28] = 'import'
# $expected->[28] = 'isa'
# Failed test '... got the right list of applicable methods for Bar'
# at t/cmop/methods.t line 302.
# Structures begin differing at:
# $got->[28] = 'import'
# $expected->[28] = 'isa'
# Looks like you failed 2 tests of 81.Out of my to-install list, I see ~80 failures, but didn't dive into them thoroughly. |
|
@dur-randir it would be helpful if you could look into the different cases you are seeing and give us some kind of summary of the error modes you see. The Moose one is interesting, it is enumerating the list of methods in a given class, and is now seeing an import method that didn't used to exist. This is interesting because it is really a very different type of error compared to the errors that are expected from this and possibly we can fix it without breaking the intent of this change. Cases where people really are calling non-existent imports with arguments thinking they are doing something useful, like a version check or import, really should be errors, even if it is irritating that so many cases are to be found in key parts of the CPAN toolchain. Those can be managed for the time being in the internals where they are important. So have a breakdown of the different causes of this would be very useful if possible. |
Released a fixed version |
|
|
Also affected: XAVIER/Graphics-ColorNames-Mozilla-0.11.tar.gz |
|
Also affected: PMAKHOLM/Encode-IMAPUTF7-1.05.tar.gz |
This has broken code in its test logic. It is attempting to import a sub from a class. Issue reported in: https://rt.cpan.org/Ticket/Display.html?id=149090
This also appears to be broken test logic. It attempts to import subs, but it does not use Exporter or define its own import method. It inherits from Encode::Encoding which also appears to not use Exporter or provide an import method. Issue report in: https://rt.cpan.org/Ticket/Display.html?id=149091 Author is asking for a new owner and has issues that are over a decade old. |
|
I will push a patch shortly which will make the new UNIVERSAL::import() method handle version numbers the same as Exporter would, and silently convert into Which should cover some of the issues reported here. Basically there are three classes of issues that the UNIVERSAL::import() patch has surfaced:
Case 1 should be fixed by my latest patch in #21279. Case 2 is a legit problem with the code that we report is broken, and that code should simply be fixed. Case 3 is a bit of a grey zone. I see broadly two positions one could take: A) You could argue that we should be able to add special methods to UNIVERSAL so people should just ignore any methods which are defined in it. B) You could also argue that since we have not defined a UNIVERSAL::import/UNIVERSAL::unimport for a long time that we should continue to not define it and do some other trick to achieve the goals we are trying to achieve here. I lean towards the A) interpretation. |
|
TAP::Formatter::TeamCity also fails on 5.36, so it's a red herring. |
|
Fix for JSON-Any at karenetheridge/JSON-Any#3 |
|
Also affected: TEVERETT/Class-Prototyped-1.13.tar.gz |
|
Patched Perl::Critic: Perl-Critic/Perl-Critic#1037 |
|
Also affected: IBB/Acme-Damn-0.08.tar.gz |
|
Also affected: GFUJI/UNIVERSAL-DOES-0.005.tar.gz |
|
Also affected (with 5.39.[12]): GWILLIAMS/Attean-0.033.tar.gz @kasei, please take note |
|
While the changed behaviour has long been documented to be otherwise, do we have a plan what to do with the breakage? I found fixing this mostly to be makework, and I'm an active maintainer. I think we should have a plan for when to revert 2dcf3cf or how to mitigate the breakage well before the next stable release. |
|
On Tue, 22 Aug 2023, 18:28 Max Maischein, ***@***.***> wrote:
While the changed behaviour has long been documented to be otherwise, do
we have a plan what to do with the breakage?
I found fixing this mostly to be makework, and I'm an active maintainer.
I think we should have a plan for when to revert 2dcf3cf
<2dcf3cf>
or how to mitigate the breakage well before the next stable release
It's already been reverted, in the sense that the error has already been
downgraded to a deprecation warning. (Weeks ago.)
If we revert totally then we are saying "we will never be able to catch
inappropriate use of import", which just means that buggy code will
proliferate forever like it had up until now, with people writing code that
they think does something but which does not, or does something different
than they think. It also means that it is unlikely we will ever have a way
to detect broken require/use statements on case insensitive file systems.
Which basically means that certain tickets will never be closed.
While I understand how it might feel like make work I don't think the label
strictly speaking fits here as once the technical debt is paid off we will
reliably be able to detect errors that previously we could not, which means
it is a step beyond pure makework. I do recognize however that paying
this technical debt off benefits a minority of our community. But I still
think we should pay it off, on principle i think it is bad policy to say
that we shouldnt ignore issues just because they dont affect a favoured
subsection of our user base. One could argue that reverting totally would
amount to saying that Win32 and Mac users will have to deal with certain
bugs just because Linux users don't want to fix their broken code. Which
doesn't seem fair.
It *is* unfortunate that so much incorrect code has hidden underneath this
particular stone, and I recognize it is frustrating to have to pay off such
technical debt that seems to only have aesthetic impact.
Yves
|
|
Also affected: OALDERS/App-perlimports-0.000052.tar.gz Fail with 5.39.1: http://www.cpantesters.org/cpan/report/000be336-4122-11ee-9721-b8056e8775ea Pass with 5.39.2: http://www.cpantesters.org/cpan/report/bcb2a7b4-4135-11ee-afe7-8ff06d8775ea In the pass report the warning: @perl-ide this may be interesting for you for https://github.com/perl-ide/App-perlimports |
|
Also affected: KHEDIN/MVC-Neaf-0.2901.tar.gz JDS/DBIx-Table-TestDataGenerator-0.005.tar.gz Both fail reports are with 5.39.2, both have no tests with 5.39.1 because they were not testable due broken dependencies. |
|
Also affected: IPENBURG/WWW-Wookie-v1.1.4.tar.gz The FAIL report is with 5.39.2, code was not testable with 5.39.1 due broken dependencies |
|
Also affected: COUDOT/Lemonldap-NG-Manager-2.17.0.tar.gz KIMOTO/SPVM-0.989040.tar.gz Both fails with 5.39.2, both pkgs here not testable with 5.39.1 due broken dependencies |
|
I think we're actually in good shape with respect to this BBC ticket. In the last few days I installed a plain, unthreaded I'm going to mark this ticket as Closable ... but it's the sort of thing we should probably keep open until our preparations for perl-5.40.0 ramp up in April. |
jkeenan
added
the
Closable?
|
Changing from an exception to a warning stopped the majority of these failures, but there are still many things broken just from the addition of a UNIVERSAL::import method. And while this is currently marked as a deprecation, I'm not sure we can actually commit to converting it to an exception at the end of a deprecation cycle. There are still many modules impacted that I want to file patches for. And I think we should consider changing to just a normal warning rather than a deprecation. |
I agree. |
|
I would still like to send out more patches to address the warnings, and I don't see a need to create a new issue for the same problem. We're considering changing the warning from a deprecation to a normal warning a release blocker. I hope to address that soon. |
haarg
removed
the
Closable?
Fixes <kasei#167>. See also <Perl/perl5#21269>.
Move the `Exporter::Tiny` setup before loading anything else so that the `import()` is visible. Use `require` for `Attean` so that `Attean->import()` is not called. Fixes <kasei#167>. See also <Perl/perl5#21269>.
jkeenan
added
the
Closable?
|
I conducted a review of this ticket over the past few days. I installed blead perl at a commit a little bit after the 5.40.0 release, install I then explored the distros on this list to see whether bug reports or pull requests had been filed at their issue trackers. In some cases I re-ran the test suites and got a PASS on the second attempt. In some but not all cases I installed missing prerequisites. There was one distribution, Dependencies-Searcher, for which no bug report had been filed; I did so. You can see my analysis in this gist. The long and short of this is that I see no reason to keep this ticket open, as everything which Perl 5 Porters can reasonably be asked to do on the ticket we have done. @haarg, if you would still like to keep this ticket open, can you please self-assign it and specify the conditions that would have to be met in order to close the ticket? Thank you very much. |
|
Template 3.102 released. |
This is a bug report for perl from "Carlos Guevara" [email protected],
generated with the help of perlbug 1.43 running under perl 5.39.1.
BBC: Blead Breaks Sub::Exporter
Please see http://fast-matrix.cpantesters.org/?dist=Sub::Exporter
Flags
Perl configuration
The text was updated successfully, but these errors were encountered: