Bug #28982 for Module-CPANTS-Analyse: Fwd: [PATCH] manifest checking

Hi, Thomas. The following patch to Module-CPANTS-Analyse-0.72 brings two minor fixes: 1. makes manifest checking more compliant to the standard handling as done by ExtUtils::Manifest 2. avoid using chdir() in Module::CPANTS::Analyse->unpack which may introduce inconvenient mistakes The issue with the manifest solves the case of a MANIFEST which includes comments such as #commented-file-name In the current version (0.72), MCA would complain about a "#commented-file-name" in MANIFEST not found in the distribution. There was some coincidences in the 0.72 implementation that make it deal correctly with lines such as # comment or # comment (which made things very misleading until I understood the current behavior). That was because s/\s.*$//; was meant to strip the comments in lines like filename # comment but chomped all of lines like # comment and left only "#" in a line like # comment and then that was discarded because of the following line of code next unless /\w/; These have been solved according to the following diff: diff -u -r Module-CPANTS-Analyse-0.72/lib/Module/CPANTS/Kwalitee/Manifest.pm Module-CPANTS-Analyse/lib/Module/CPANTS/Kwalitee/Manifest.pm --- Module-CPANTS-Analyse-0.72/lib/Module/CPANTS/Kwalitee/Manifest.pm Sat Jun 30 18:15:25 2007 +++ Module-CPANTS-Analyse/lib/Module/CPANTS/Kwalitee/Manifest.pm Fri Aug 24 14:24:51 2007 @@ -24,8 +24,10 @@ my @manifest; while (<$fh>) { chomp; - s/\s.*$//; - next unless /\w/; + next if /^\s*#/; # discard pure comments + + s/\s.*$//; # strip file comments + next unless $_; # discard blank lines push(@manifest,$_); } close $fh; The corresponding code is a straightforward analog to the one found in sub maniread() from ExtUtils::Manifest. While I was fixing this, I wrote a test like this: { my $a=Module::CPANTS::Analyse->new({ dist => 't/eg/Good-Dist-0.01.tar.gz' }); $a->unpack; $a->analyse; is($a->d->{manifest_matches_dist}, 1, 'manifest matches dist'); } { my $a=Module::CPANTS::Analyse->new({ dist => 't/eg/no-manifest-0.01.tar.gz' }); $a->unpack; $a->analyse; is( $a->d->{manifest_matches_dist}, undef, 'manifest does not match dist' ); } and found that it crashed (not finding the second tarball) even though the objects were recreated. It worked alright if I run only the code in one brace or another. So it was a matter of global changes. The culprit was sub unpack() in Module::CPANTS::Analyse that did chdir($me->testdir); which changed the current directory and screwed the rest of the test which used relative paths. The solution was: --- Module-CPANTS-Analyse-0.72/lib/Module/CPANTS/Analyse.pm Sat Jun 30 18:15:25 2007 +++ Module-CPANTS-Analyse/lib/Module/CPANTS/Analyse.pm Fri Aug 24 19:44:39 2007 @@ -46,13 +46,12 @@ return 'cant find dist' unless $me->dist; copy($me->dist,$me->testfile); - chdir($me->testdir); $me->d->{size_packed}=-s $me->testfile; my $archive; eval { $archive=Archive::Any->new($me->testfile); - $archive->extract(); + $archive->extract($me->testdir); }; That is using Archive::Any->extract() with a directory as argument made the chdir() unnecessary and got rid of this side-effect. The patch is attached together with t/plugin_manifest.t - a new test with focus on Module::CPANTS::Kwalitee::Manifest and t/eg/Good-Dist.tar.gz - a distribution with a high kwalitee (including a good MANIFEST) t/eg/no-manifest.tar.gz - a distribution without a MANIFEST t/eg/bad-manifest.tar.gz - a distribution with a MANIFEST that did not match Best regards, Adriano Ferreira