MakeMaker's NAME must be a valid package name, not ditribution name replacing - with :: #164

miyagawa · 2013-04-07T08:53:21Z

$self->zilla->name contains a distribution name and it might be different from "main_module" package names, which is required by MakeMaker's NAME.

From perldoc ExtUtils::MakeMaker:

"NAME" must be a valid Perl package name and it must have an
associated ".pm" file. For example, "Foo::Bar" is a valid "NAME" and
there must exist Foo/Bar.pm.

cc @dagolden

…eplacing - with ::. $self->zilla->name contains distribution name and it might be different from "main_module" package names, which is required by MakeMaker's NAME. From perldoc ExtUtils::MakeMaker: "NAME" must be a valid Perl package name and it must have an associated ".pm" file. For example, "Foo::Bar" is a valid "NAME" and there must exist Foo/Bar.pm.

karenetheridge · 2013-04-07T15:45:07Z

Neither the original code nor the changes seem to guarantee what EUMM specifies - that there is both a Foo::Bar and Foo/Bar.pm. After looking for the 'modules inside' the main package, there should be a check for -e Foo/Bar.pm.

(The spec doesn't say, but surely it should also say that Foo::Bar comes from Foo/Bar.pm, not merely that such a file must exist in the distribution.)

miyagawa · 2013-04-07T19:13:53Z

My original patch was doing the path check and basically drops ^lib/ and .pm$. Is that better?
—
Sent from Mailbox for iPhone

On Sun, Apr 7, 2013 at 8:45 AM, Karen Etheridge notifications@github.com
wrote:

Neither the original code nor the changes seem to guarantee what EUMM specifies - that there is both a Foo::Bar and Foo/Bar.pm. After looking for the 'modules inside' the main package, there should be a check for -e Foo/Bar.pm.

Reply to this email directly or view it on GitHub:
#164 (comment)

miyagawa · 2013-04-07T20:09:40Z

@karenetheridge branch updated - somehow tries to match the package with filename.

You're right that the MakeMaker spec doesn't say it should also match with the original filename, but my patch would just try that since I believe it would work for 99% of the use case, and fallback to the distname conversion with the log telling something is wrong.

miyagawa · 2013-04-07T20:14:23Z

Here's what you get on my forkprove dist.ini https://github.com/miyagawa/forkprove before/after with the patch.

diff -ruP forkprove-v0.4.7/Makefile.PL forkprove-v0.4.7.patch/Makefile.PL
--- forkprove-v0.4.7/Makefile.PL        2013-04-07 13:12:43.000000000 -0700
+++ forkprove-v0.4.7.patch/Makefile.PL  2013-04-07 13:11:24.000000000 -0700
@@ -20,7 +20,7 @@
     "script/forkprove"
   ],
   "LICENSE" => "perl",
-  "NAME" => "forkprove",
+  "NAME" => "App::ForkProve",
   "PREREQ_PM" => {
     "App::Prove" => "3.25",
     "TAP::Harness" => "3.25",

dagolden · 2013-04-07T21:03:24Z

I've not paid any attention to the patches so far, but my heuristic would
be "try -e dist-name converted to module name; if that fails, use shortest
path .pm found and convert that to a module name".

On Sun, Apr 7, 2013 at 4:14 PM, Tatsuhiko Miyagawa <notifications@github.com

wrote:

Here's what you get on my forkprove dist.ini
https://github.com/miyagawa/forkprove before/after with the patch.

diff -ruP forkprove-v0.4.7/Makefile.PL forkprove-v0.4.7.patch/Makefile.PL
--- forkprove-v0.4.7/Makefile.PL 2013-04-07 13:12:43.000000000 -0700
+++ forkprove-v0.4.7.patch/Makefile.PL 2013-04-07 13:11:24.000000000 -0700
@@ -20,7 +20,7 @@
"script/forkprove"
],
"LICENSE" => "perl",

"NAME" => "forkprove",

"NAME" => "App::ForkProve",
"PREREQ_PM" => {
"App::Prove" => "3.25",
"TAP::Harness" => "3.25",

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/164#issuecomment-16022796
.

David Golden dagolden@cpan.org
Take back your inbox! → http://www.bunchmail.com/
Twitter/IRC: @xdg

miyagawa · 2013-04-07T21:20:54Z

Yep that's what ->main_module already does, and this patch additionally does a) scan package with Module::Metadata and b) if the found matches with the path name, and feed that result into MakeMaker's NAME.

miyagawa · 2013-04-09T00:16:36Z

lib/Dist/Zilla/Plugin/MakeMaker.pm

+  my $match = sub {
+    my $pkg = shift;
+    return if $pkg eq 'main' or $pkg eq 'DB';
+    $pkg =~ s!::!/!g;


does $path->name return backslash on win32? if so i need to change that to [/\\] instead of just forward slash. let me know.

miyagawa · 2013-04-14T06:02:56Z

bump. Any thought on this?

rjbs · 2015-07-25T17:33:02Z

I apologize for letting this languish. I am now committed to clearing old PRs.

I will merge this if the main_module_name is moved to a private, lazy check performed in MakeMaker when the main_module-as-pkg-name does not exist. i.e., rarely. :) In other words: only bother looking inside the main module file if Foo/Bar.pm does not exist for dist Foo-Bar.

Tests would be gerat, too, but I can live without them. ;)

rjbs · 2015-07-25T17:33:11Z

Oops, closing was a mistake!

Make sure the extracted package matches with the filename somehow.

10429b3

miyagawa reviewed Apr 9, 2013
View reviewed changes

rjbs closed this Jul 25, 2015

rjbs reopened this Jul 25, 2015

rjbs added Needs Rework Needs Rebase labels Jul 25, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MakeMaker's NAME must be a valid package name, not ditribution name replacing - with :: #164

MakeMaker's NAME must be a valid package name, not ditribution name replacing - with :: #164

miyagawa commented Apr 7, 2013

karenetheridge commented Apr 7, 2013

miyagawa commented Apr 7, 2013

Neither the original code nor the changes seem to guarantee what EUMM specifies - that there is both a Foo::Bar and Foo/Bar.pm. After looking for the 'modules inside' the main package, there should be a check for `-e Foo/Bar.pm`.

miyagawa commented Apr 7, 2013

miyagawa commented Apr 7, 2013

dagolden commented Apr 7, 2013

miyagawa commented Apr 7, 2013

miyagawa Apr 9, 2013

miyagawa commented Apr 14, 2013

rjbs commented Jul 25, 2015

rjbs commented Jul 25, 2015

MakeMaker's NAME must be a valid package name, not ditribution name replacing - with :: #164

MakeMaker's NAME must be a valid package name, not ditribution name replacing - with :: #164

Conversation

miyagawa commented Apr 7, 2013

karenetheridge commented Apr 7, 2013

miyagawa commented Apr 7, 2013

Neither the original code nor the changes seem to guarantee what EUMM specifies - that there is both a Foo::Bar and Foo/Bar.pm. After looking for the 'modules inside' the main package, there should be a check for -e Foo/Bar.pm.

miyagawa commented Apr 7, 2013

miyagawa commented Apr 7, 2013

dagolden commented Apr 7, 2013

miyagawa commented Apr 7, 2013

miyagawa Apr 9, 2013

Choose a reason for hiding this comment

miyagawa commented Apr 14, 2013

rjbs commented Jul 25, 2015

rjbs commented Jul 25, 2015

Neither the original code nor the changes seem to guarantee what EUMM specifies - that there is both a Foo::Bar and Foo/Bar.pm. After looking for the 'modules inside' the main package, there should be a check for `-e Foo/Bar.pm`.