Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

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

Open
wants to merge 2 commits into from

3 participants

Tatsuhiko Miyagawa Karen Etheridge David Golden
Tatsuhiko Miyagawa

$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

Tatsuhiko Miyagawa miyagawa MakeMaker's NAME must be a valid package name, not ditribution name r…
…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.
04612a0
Karen Etheridge

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.)

Tatsuhiko Miyagawa
Tatsuhiko Miyagawa

@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.

Tatsuhiko Miyagawa

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",
David Golden
Tatsuhiko Miyagawa

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.

Tatsuhiko Miyagawa miyagawa commented on the diff
lib/Dist/Zilla/Plugin/MakeMaker.pm
@@ -169,11 +170,33 @@ sub share_dir_code {
return $share_dir_code;
}
+sub main_module_name {
+ my $self = shift;
+
+ my $file = $self->zilla->main_module->name;
+
+ my $match = sub {
+ my $pkg = shift;
+ return if $pkg eq 'main' or $pkg eq 'DB';
+ $pkg =~ s!::!/!g;
Tatsuhiko Miyagawa
miyagawa added a note

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Tatsuhiko Miyagawa

bump. Any thought on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 7, 2013
  1. Tatsuhiko Miyagawa

    MakeMaker's NAME must be a valid package name, not ditribution name r…

    miyagawa authored
    …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.
  2. Tatsuhiko Miyagawa
This page is out of date. Refresh to see the latest.
Showing with 26 additions and 3 deletions.
  1. +26 −3 lib/Dist/Zilla/Plugin/MakeMaker.pm
29 lib/Dist/Zilla/Plugin/MakeMaker.pm
View
@@ -8,6 +8,7 @@ use namespace::autoclean;
use Config;
use List::MoreUtils qw(any uniq);
+use Module::Metadata;
use Dist::Zilla::File::InMemory;
use Dist::Zilla::Plugin::MakeMaker::Runner;
@@ -169,11 +170,33 @@ sub share_dir_code {
return $share_dir_code;
}
+sub main_module_name {
+ my $self = shift;
+
+ my $file = $self->zilla->main_module->name;
+
+ my $match = sub {
+ my $pkg = shift;
+ return if $pkg eq 'main' or $pkg eq 'DB';
+ $pkg =~ s!::!/!g;
Tatsuhiko Miyagawa
miyagawa added a note

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ $file =~ /\b$pkg\.pm$/;
+ };
+
+ # per MakeMaker's spec, NAME must be a package and matching .pm file should exist
+ my $meta = Module::Metadata->new_from_file($file);
+ my $name = (grep $match->($_), $meta->packages_inside)[0];
+
+ unless ($name) {
+ ($name = $self->zilla->name) =~ s/-/::/g;
+ $self->log(["Matching package not found for %s. Falling back to %s", $file, $name]);
+ }
+
+ $name;
+}
+
sub write_makefile_args {
my ($self) = @_;
- (my $name = $self->zilla->name) =~ s/-/::/g;
-
my @exe_files =
$self->zilla->find_files(':ExecFiles')->map(sub { $_->name })->flatten;
@@ -217,7 +240,7 @@ sub write_makefile_args {
my %write_makefile_args = (
DISTNAME => $self->zilla->name,
- NAME => $name,
+ NAME => $self->main_module_name,
AUTHOR => $self->zilla->authors->join(q{, }),
ABSTRACT => $self->zilla->abstract,
VERSION => $self->zilla->version,
Something went wrong with that request. Please try again.