Skip to content
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

strip underscore from versions when considering "accepts module" #28

Open
karenetheridge opened this issue Jul 16, 2015 · 14 comments
Open

Comments

@karenetheridge
Copy link
Member

As discussed on irc (#distzilla, 2015-07-15), the underscore in a module's $VERSION should be stripped before passing it to version.pm in accepts_module() (or a private submethod thereof). This will restore consistency with CPAN.pm and other tools.

(We also need to ensure cpanm and Module::Build do the same thing.)

cc @miyagawa @Leont @shadowcat-mst

@shadowcat-mst
Copy link
Member

cpanm just uses CMR, I think? EUMM does the s///, then passes it to CMR. Not sure what CPAN.pm does (it definitely strips, but I dunno if it uses CMR or its own code still)

@miyagawa
Copy link
Member

is this related to #26?

If so, stripping alpha-ness from $VERSION will make a false requirement that passes in CMR's accepts_module() but will fail on runtime. (See the issue linked above)

I usually don't lurk on #distzilla so a summary for that, or a specific problem that you need to address with an actual failing example, would be helpful for me to complete the context.

also - yes, cpanm uses CMR, hence miyagawa/cpanminus/issues/463 and #26

@karenetheridge
Copy link
Member Author

Yes. I don't understand the (presumably failing?) example in #26 though; can someone clarify?

@miyagawa
Copy link
Member

suppose you have installed a dev version of Foo 2.000_001 from CPAN.

package Foo;
our $VERSION = '2.000_001';

and your code has a requirement for Foo => 2.000001.

This issue is about making that requirement pass, by eliminating _ before comparison inside CMR.

That's fine until in the runtime you have a code that says:

use Foo 2.000001;

at this point perl will blow up: Foo version 2.000001 required--this is only version 2.000_001.

@karenetheridge
Copy link
Member Author

That's fine until in the runtime you have a code that says:

use Foo 2.000001;

at this point perl will blow up: Foo version 2.000001 required--this is only version 2.000_001.

We covered that in the #distzilla discussion -- the conclusion was basically
"we shouldn't have to support authors who do that, because that's dreadfully
wrong -- therefore the authors can keep both halves."

That is -- any author that declares an underscore version but doesn't include a follow-up line $VERSION = eval $VERSION; is being naughty and wrong.

@miyagawa
Copy link
Member

"we shouldn't have to support authors who do that, because that's dreadfully wrong -- therefore the authors can keep both halves.

Well, it's not just "authors" who does this - it could be an end user, or a tool (a la PrereqScanner, cpanfile, Module::Metadata) that programatically ends up calling module->VERSION(requirement).

I honestly don't have a strong preference, but just wanted to point out that this issue contains an incorrect assumption (which you've covered as a wrong usage, just fine) and will possibly lead to a technical debt.

@karenetheridge
Copy link
Member Author

I think you misunderstood. It's only authors that are declaring $VERSIONs with an underscore but neglecting the eval line -- that's the bad bit. There's nothing wrong with calling ->VERSION on any module, whether or not it's a trial release.

@miyagawa
Copy link
Member

There's nothing wrong with calling ->VERSION on any module, whether or not it's a trial release.

But it's not OK to call, if the installed version is a devel release and there's no eval by the author.

The end result of this is that, as an end user, even if you have requires => { Foo => "2.001" } in your requirement, you're not sure if you can safely call Foo->VERSION(2.001), depending on the dev/trial state.

@karenetheridge
Copy link
Member Author

But it's not OK to call, if the installed version is a devel release and there's no eval by the author.

Why is that relevant for this discussion? It's not safe to call today if there's no eval by the author. The breakage isn't made worse, and the code was just as wrong yesterday as it is today.

@miyagawa
Copy link
Member

Why is that relevant for this discussion? It's not safe to call today if there's no eval by the author.

Sure, but you do know that it's not safe, because tools (i.e. cpanm or CPAN::Meta::Requirements) tells you that the installed version doesn't satisfy your requirement before running that code.

@miyagawa
Copy link
Member

I'm merely pointing out that, by accepting this change, there'll be a mismatch in a list of prereqs and runtime perl's UNIVERSAL:::VERSION behavior. i.e. if a user installs a dev version of the module, the user can't specifically require that version.

If we all think that is a use case that can be ignored, and using a module with use Module VER with the version number that only exists as a dev version is un-supported, I'm fine.

cc @dagolden because you pointed out that fact in #26

@miyagawa
Copy link
Member

Here's a convoluted scenario:

  • Author A uploads Foo-2.0_1 (dev) to CPAN
  • User X installs Foo-2.0_1 with --dev
  • Author A re-uploads Foo-2.01 (non-dev) to CPAN. It gets indexed
  • Author B uploads Bar, with a prereq Foo => 2.01
  • User X installs Bar

Today, because '2.0_1' doesn't satisfy the requirement '2.01' cpanm will try to upgrade to the "non-dev" version of Foo. With this change, cpanm will fail to upgrade it because '2.0_1' will now satisfy '2.01'. In addition a runtime call of use Foo 2.01 will fail.

Again, this is convoluted, and it must be an author A's fault of reusing the same version number in non-dev and dev. Just wanted to point out that, by accepting this change we're deprecating the currently supported scenario.

Look, I'm the one who requested this first in #26 :)

@Leont
Copy link
Member

Leont commented Jul 18, 2015

I'd rather fix this in version.pm

@karenetheridge
Copy link
Member Author

I'd rather fix this in version.pm

Me too. But can we do both? I suspect the CMR fix will be faster to implement, so it can at least move the problem one layer lower while we fret with what to change in version.pm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants