miyagawa / cpanminus

cpanminus - get, unpack, build and install modules from CPAN
http://cpanmin.us
752 stars 213 forks source link

cpanm fails under perl 5.39.1 due to broken bundled CPAN::Meta::Check 0.017 #662

Closed mauke closed 1 year ago

mauke commented 1 year ago
$ ~/perl5/perlbrew/perls/perl-5.39.1/bin/cpanm --test-only List::Util
--> Working on List::Util
Fetching http://www.cpan.org/authors/id/P/PE/PEVANS/Scalar-List-Utils-1.63.tar.gz ... OK
Attempt to call undefined import method with arguments via package "CPAN::Meta::Prereqs" (Perhaps you forgot to load the package?) at /home/mauke/perl5/perlbrew/perls/perl-5.39.1/bin/cpanm line 152.
BEGIN failed--compilation aborted at /home/mauke/perl5/perlbrew/perls/perl-5.39.1/bin/cpanm line 152.
Compilation failed in require at /home/mauke/perl5/perlbrew/perls/perl-5.39.1/bin/cpanm line 119.

This is due to https://rt.cpan.org/Ticket/Display.html?id=149085. CPAN::Meta::Check 0.018 has a fix. Can you release an update?

miyagawa commented 1 year ago

I can make a new release but perl shouldn't break this and should revert the change IMO.

mauke commented 1 year ago

There is quite a range of possible behavior if you write use Some::Module "1.23":

  1. If Some::Module uses Exporter, this is a version check: Some::Module->VERSION("1.23");
  2. If Some::Module implements its own custom export logic or uses a different exporter module, this is likely an error: "1.23" is not a valid symbol name.
  3. If Some::Module does not export anything and you're on perl < 5.39.1, nothing happens. This is likely a bug: You probably intended to specify version requirements, but perl just ignores them.
  4. If Some::Module does not export anything and you're on perl 5.39.1, this throws an error.

There is currently a PR in blead perl (https://github.com/Perl/perl5/pull/21279) that changes the behavior of (4) to (1) if the imported "symbol" looks like a number instead.

Personally I think it's better to fix the code where possible, so we don't have to rely on fallback logic in Exporter or UNIVERSAL.

miyagawa commented 1 year ago

Agreed that we should better fix where possible, in the long term. Not sure if perl has the freedom to break this suddenly though - might be better to start warning when this happens and then turn it into an error in a future release, or something.

haarg commented 1 year ago

The change in core has broken many modules and the current plan is to relax the error to a deprecation warning. Perl/perl5#21269

zakame commented 1 year ago

Also encountered this during my docker-perl update for 5.39.1 - yeah I'd much rather wait for 5.39.2.

demerphq commented 1 year ago

@miyagawa we broke this because long term it will allow us to help developers identify a long standing bug category when building on case insensitive file systems. That we allowed these buggy uses of import to be ignored for so long was itself a bug. Many people were writing "version checks" which weren't actually being executed, thus creating a bug. Also many people were using use_ok() improperly in their tests, thus creating technical debt that would become visible if they ever added an import method. Ultimately this demonstrates that treating import() calls to code that doesn't implement import() as a no-op was a bad idea and stored up a lot of broken code and broken expectations. IMO we should fix such things in the long run.

So there really isn't a good answer here. This is especially true for cpanm, since you fatpack modules even though the module that breaks cpanm is already fixed on CPAN you have hard bound your code to the broken version, so the only way out of the problem (as far as 5.39.1 is concerned) is for you to release a fix. We are in a similar position with perl 5.39.1, since it is released already, even though we have already downgraded the exception to a deprecation warning 5.39.1 has been released. People wont see the effect of that revert/downgrade until 5.39.2.

IMO the fact that a module like CPAN::Meta::Check was broken (in the sense that it attempted to perform a version check that was in fact a no-op), demonstrates that even super experienced devs could fall into this bug and is a good argument for us taking measures to ensure it is fixed long run.

I am sorry for the inconvenience of having to roll a new release, but IMO long run getting this issues out from under the carpet (so to speak) is for the good of the ecosystem.

miyagawa commented 1 year ago

Fixed with 1.7047. https://cpanmin.us/ should also be updated.

zakame commented 1 year ago

Thanks @miyagawa ! 🎉 😻

karenetheridge commented 1 year ago

I would suggest also updating the trial series (the latest version is currently 1.7907).