Helping to clean CPAN: December 2016
… where I get out my virtual broom and sweep up cruft I can find in my assigned distribution for this month’s edition of the CPAN Pull Request Challenge.
This month’s module: Math::Int128
Math::Int128 allows one to manipulate 128 bits integers in Perl.
First impressions
In this section I try to get a feeling for the state of the module, how up to date it is, how often people are contributing to it, how many other distributions are depending on it, how many bugs/issues it currently has, what the CPANTS kwalitee is, etc.
- last commit 7th of April 2015 (https://github.com/salva/p5-Math-Int128)
- latest release 7th of April 2015 (https://metacpan.org/release/Math-Int128)
- 0 open issues on GitHub (https://github.com/salva/p5-Math-Int128/issues)
- 0 open pull requests (https://github.com/salva/p5-Math-Int128/pulls)
- 1 open ticket on RT (https://rt.cpan.org/Public/Dist/Display.html?Name=Math-Int128)
- 5 reverse dependencies via metacpan
- 64 test failures on cpantesters: http://www.cpantesters.org/distro/M/Math-Int128.html
- most are on Sun/Solaris, some also on FreeBSD; possibly a compiler-related issue? These platforms don’t always use gcc
- CPANTS report (http://cpants.cpanauthors.org/dist/Math-Int128)
- no core issues:
- extra issues:
- experimental issues:
The impression is of stable software which merely needs to be tested on more platforms so as to improve portability.
Initial inspection of the source code
After forking the repo and cloning a local copy, I have a look at the project to see what build system it uses, if the test suite works and the tests pass, if it could do with a Travis-CI config file (or if present, if it can be updated). The initial inspection often gives inspiration for the first batch of pull requests.
- already configured for Travis. Add Travis-Badge to GitHub page?
- uses
Dist::Zilla
dzil authordeps --missing | cpanm
works ok- there seem to be lots of dependencies for a small dist
dzil test
fails due to a missing LICENSE file even though it exists
[@DROLSKY/CopyFilesFromBuild] Cannot copy LICENSE from build: file does not exist
[@DROLSKY/CopyFilesFromBuild] Cannot copy LICENSE from build: file does not exist at /home/cochrane/perl5/perlbrew/perls/perl-5.20.3/lib/site_perl/5.20.3/x86_64-linux/Moose/Meta/Method/Delegation.pm line 110.
-
submitted a fix to this issue in PR#17
perlver .
says the minimum version of perl is v5.13.2; the specified minimum version is 5.6.perlver
thus exits with an error. A recommended fix was submitted in PR#16.- uses Travis-CI, however the list of Perls could be extended. List of Perls extended in PR#10.
- would be nice to have setup instructions for a development environment in the README.md
- interestingly enough, the Travis builds are set up via the
travis/setup.pl
script in the dist. Travis doesn’t useDist::Zilla
at all… - running
perl Makefile.PL; make; make test
gave the error thatMath::Int64
was missing. This is installed explicitly in the Travis setup script which would explain why the dist builds and tests correctly on Travis. - uses
Dist::Zilla::Plugin::MakeMaker::Awesome
to generate theMakefile.PL
. - adding
Math::Int64
to the list ofDevelopRequires
indist.ini
, then runningdzil listdeps --missing | cpanm
to install the missing package, then needed to rundzil build
and copy the generatedMakefile.PL
from.build/latest
into the working directory so thatperl Makefile.PL; make; make test
ran correctly. - removing the
-remove = License
line fromdist.ini
allowsdzil test
to run successfully to completion. - noticed that dzil test shows missing deps.
- found a cpanfile in the dist…
- looks like the module actually uses
carton
for dependency management. - nope, the cpanfile is autogenerated. Then it gets used in the
00-report-prereqs.t
test, which then gives a warning about missing required prereqs. Which test suite is actually used? - the development setup isn’t clear. How should one install deps? Via
dzil authordeps
? Viacpanm --installdeps .
? Or should carton be used (which only installs locally and doesn’t usecpanm
)? - it turns out that there’s an automatically generated
CONTRIBUTING.md
file… - the
dzil
incantation required in order to install all development-related dependenices is:dzil listdeps --author --missing | cpanm
It’s a bit confusing as to how to build the dist, however once one finds out
that a CONTRIBUTING.md
file is created as part of the dzil build
process, one can find out how to build the project. The problem was working
out how to fix the initial errors generated when running dzil
so that this
file could be created so that one could work out how to build the dist in a
development environment. Perhaps it would be a good idea to add a quick
start guide for potential developers in the README.
Code Coverage
Looking at the code coverage can give an indication of code quality. If the project is well covered, this means most changes made in pull requests can be made with some confidence that any problems will be caught by the test suite. If the code coverage is low, then this is something that one could address as a pull request (or set of pull requests).
In EUMM and Build::Module
projects, one simply needs to install
Devel::Cover
and run
$ cover -test
In Dist::Zilla
projects, one needs to install the
Dist::Zilla::App::Command::cover
plugin, after which the code coverage can
be checked via:
$ AUTHOR_TESTING=1 dzil cover
In this distribution, the coverage is:
97.8% total statement coverage; 96.7% total coverage.
which is excellent and can’t really be improved upon.
POD checks
The utility podchecker
searches through Perl source code for POD which
might not conform to the POD standard, and thus not necessarily be parseable
by all POD parsers. Fixing any issues found by podchecker
has the
positive effect of also removing any warnings noted in the project’s
documentation displayed on MetaCPAN.
Running podchecker
gives the following errors and warnings:
$ find ./ -name '*.pm' -or -name '*.pod' | xargs podchecker |& grep ERROR
ERROR: =end without =begin at line 31 in file ./inc/Config/AutoConf.pm
ERROR: =end without =begin at line 58 in file ./inc/Config/AutoConf.pm
ERROR: unresolved internal link 'Limitations' at line 707 in file ./inc/Capture/Tiny.pm
$ find ./ -name '*.pm' -or -name '*.pod' | xargs podchecker |& grep WARN
WARNING: =head2 without preceding higher level at line 29 in file ./inc/Config/AutoConf.pm
WARNING: =head2 without preceding higher level at line 56 in file ./inc/Config/AutoConf.pm
WARNING: line containing nothing but whitespace in paragraph at line 1672 in file ./inc/Config/AutoConf.pm
All of the errors and warnings come from files included as part of the distribution, however coming from other distributions. Perhaps the issues have been fixed in the upstream dists, nevertheless, this means there are no POD issues in the code of the current dist.
Nit-picking
Check for trailing whitespace
Some projects consider this a must, and will disallow commits to be submitted which contain trailing whitespace (the Linux kernel is an example project where trailing whitespace isn’t permitted). Other projects see whitespace cleanup as simply nit-picking. Either way one sees it personally, this could be a useful pull request to a project, so it’s worthwhile fixing and submitting; the worst that can happen is that the pull request is closed unmerged.
To look for files with trailing whitespace, run git grep '\s\+$'
. It can
be helpful to load the files found directly into vim
:
$ vim $(git grep '\s\+$' | cut -d':' -f1 | sort | uniq)
The list of files to fix was:
c_api.decl
Changes
inc/Capture/Tiny.pm
inc/Config/AutoConf.pm
Int128.xs
LICENSE
The files under inc/
are actually from other dists, so can be ignored.
The LICENSE
file is automatically generated, so it too can be ignored.
The remaining files can thus be cleaned up. The whitespace issues were
fixed in PR#12.
Add files to .gitignore
The files generated by Devel::Cover
needed to be ignored as well as the
automatically generated CONTRIBUTING.md
file. These changes were
incorporated into PR#11. DROLSKY made a note that the CONTRIBUTING.md
file should really be in the repository. The change ignoring this file was
removed from PR#11 and the CONTRIBUTING.md
file added in PR#15.
perlcritic
Perl::Critic will show up many
potential issues for Perl code. By simply running perlcritic
on the lib
and t
directories, one can get a further handle on the code’s quality.
$ perlcritic lib
$ perlcritic t
No problems found at the default perlcritic severity level in lib/
or
t/
. This dist has Perl files also under benchmarks
and travis
. There
is one strictures issue found in benchmarks/syp.pl
. The strictures issue
was fixed in PR#13.
Stale URLs
Links to websites can go out of date, so it’s a good idea to see if they need updating or removing. A quick grep finds all the links. After which, we just need to see which links need fixing, if any.
$ git grep 'http://\|https://\|ftp://\|git://'
Links look ok.
Copyright notices
This probably also sits in the nit-picking category for some people, however, copyright dates (theoretically) need to be kept up to date. The appropriate copyright year is usually the year of the last release. However, if a release looks imminent, then the current year is likely to be the right candidate. Some distributions put the author’s email address on the same line as the copyright date, hence this needs to be checked as well.
Files missing current year in copyright
Here we do a case-insensitive grep over the source for the word “copyright”,
the line of which we check for the existence of a year (i.e. 4 digits), look
for the appropriate year and then clean up the grep results to get something
we can pass directly to vim
.
$ COPYRIGHT_YEAR=2016
$ git grep -i copyright
$ vim $(git grep -i copyright | grep -P '\d{4}' | grep -v $COPYRIGHT_YEAR | cut -d':' -f1 | uniq)
Found that Dave Rolsky’s name was missing in the copyright holder information, so added that as well as updated the copyright year to the current release year (2015). This was submitted in PR#14.
Files missing an email address in copyright
$ vim $(git grep -i copyright | grep -P '\d{4}' | grep -v '@' | cut -d':' -f1 | uniq)
This distribution isn’t consistent in its usage of email addresses in copyright statements (and in fact can’t be, since some of the copyright statements are generated automatically), so it doesn’t seem necessary to update this information.
Spell check POD
Good documentation can be a wonder to read. Not everyone’s docs are awesome, however we can keep the error rate to a minimum. A quick spell check will pick up most typos that don’t need to be there and fixing them can help improve the quality of a project.
In general, we want to find all files containing POD and run a spell checker
(e.g. aspell
) over all files, fixing typos we come across as we go. Not
all projects require this much effort, however here’s a general-ish way to
look for and check all POD in a project:
$ files_with_pod=$(find ./lib -name '*.pm' -o -name '*.pod' \
| xargs podchecker 2>&1 \
| grep 'pod syntax' | cut -d' ' -f1)
$ for filename in $files_with_pod
do
pod2text $filename > $filename.txt;
aspell -c $filename.txt;
done
Now look for .bak
file and check differences between it and the output
.txt
file, the process looks roughly like this:
$ find ./ -name '*.bak'
$ diff -u lib/Path/To/Module.pm.pm.txt.bak lib/Path/To/Module.pm.txt
Then update the appropriate .pm
and/or .pod
files as necessary.
In this month’s distribution there were only the two files to check and so they could simply be spell checked directly. Sentence flow and grammar also looks good.
Kwalitee tests
Although CPANTS is the main kwalitee
reference, one can also run the kwalitee tests locally. One can use the
t/99kwalitee.t
test script from
http://peateasea.de/cpan-pull-request-challenge/ for this purpose.
However, the script only uses Test::Kwalitee
which doesn’t cover as many
metrics as CPANTS.
Test::Kwalitee::Extra uses
set of metrics closer to that used on CPANTS, so replace the kwalitee_ok
call with simply use Test::Kwalitee::Extra
. More information about the
many options to Test::Kwalitee::Extra
can be found on the MetaCPAN page.
Run the kwalitee tests in an ExtUtils::MakeMaker
or Build::Module
distribution like so:
$ RELEASE_TESTING=1 prove t/99kwalitee.t
or, if the distribution uses Dist::Zilla
, run
$ dzil test --author --release
To use the kwalitee tests in this month’s module, it was necessary to first
build the project with dzil build
, then enter the build directory
Math-Int128-0.23
and then to run the tests via
prove
, however specifying the test file in the project directory. In
other words:
RELEASE_TESTING=1 prove -l ../t/99kwalitee.t
../t/99kwalitee.t .. 1/29
# Failed test 'meta_yml_declares_perl_version by Module::CPANTS::Kwalitee::MetaYML'
# at ../t/99kwalitee.t line 12.
# Detail: This distribution does not declare the minimum perl version in META.yml.
# Remedy: If you are using Build.PL define the {requires}{perl} = VERSION field. If you are using MakeMaker (Makefile.PL) you should upgrade ExtUtils::MakeMaker to 6.48 and use MIN_PERL_VERSION parameter. Perl::MinimumVersion can help you determine which version of Perl your module needs.
../t/99kwalitee.t .. 28/29 # Looks like you failed 1 test of 29.
../t/99kwalitee.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/29 subtests
These errors are in essence a repetition of the warnings found on CPANTS mentioned in the “First impressions” section above. The minimum Perl version issue has been (theoretically) addressed in PR#16.
GitHub issues
There are no open GitHub issues.
RT issues
The single bug mentioned in the RT queue is an as yet unresolved discussion between the two authors of the distribution. It therefore doesn’t seem appropriate for me to weigh in here as well.
Overview of the pull requests made
- Extend list of Perls used on Travis
- merged!
- Ignore more generated files
- Remove trailing whitespace
- Add strict and warnings pragmas to syp benchmark
- Update copyright info
- Add the CONTRIBUTING documentation
- Increase minimum Perl version
- Stop ignoring LICENSE file
Conclusion
Wow, within 20 minutes, SALVA had already merged the first pull request!
The remaining PRs, however, have not yet been merged. One was commented on by DROLSKY, which prompted the creation of PR#15. It seems that the stress of the “silly season” has reduced the author’s time for looking at pull requests.
One thing that disappointed me about the result of my PRs to this month’s dist was that I really only picked at the very low hanging fruit and the “nit-picky” stuff. The dist has test failures on Sun and FreeBSD and it probably would have been better to attack things like that so that the dist is more platform independent. At least having fixed an issue like that would have felt more like real progress and not just tidying up. Oh well, maybe next month!