… where I get out my virtual broom and sweep up cruft in my assigned distribution for this month’s edition of the CPAN Pull Request Challenge.
This month’s module: Crypt::OpenSSL::RSA
Crypt::OpenSSL::RSA performs RSA encoding and decoding, using the openSSL libraries.
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 18th of June 2013 (https://github.com/monken/Crypt-OpenSSL-RSA)
- latest release 25th of August 2011 (https://metacpan.org/release/Crypt-OpenSSL-RSA)
- issues are disabled on GitHub
- issues are tracked on RT: https://rt.cpan.org/Public/Dist/Display.html?Name=Crypt-OpenSSL-RSA
- 2 open issues on RT
- issues are relatively recent; last updated within the last 9 months
- 3 open pull requests
- one of the pull requests has prepared the way for a 0.29 release
- 45 reverse dependencies via metacpan
- … so reasonably important
- 90 test failures on cpantesters: http://www.cpantesters.org/distro/C/Crypt-OpenSSL-RSA.html?oncpan=1&distmat=1&version=0.28&grade=3
- CPANTS report (http://cpants.cpanauthors.org/dist/Crypt-OpenSSL-RSA)
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.
- no Travis config: maybe add one?
- added in PR#5
perl Makefile.PLworks ok
cpanm --installdeps .works ok
make testruns and passes; the
t/bignum.ttests are skipped without reason (maybe provide one?)
- the reason is that
Crypt::OpenSSL::Bignumis required in order to run these tests.
- reason for skip in tests provided in PR#16
- the reason is that
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
Dist::Zilla projects, one needs to install the
Dist::Zilla::App::Command::cover plugin, after which the code coverage can
be checked via:
In this distribution, the coverage is:
64.1% total statement coverage; 47.1% total coverage.
Most of the code is covered in the Perl module, however just under half of the XS code is covered by the tests.
Crypt::OpenSSL::Bignum (and hence getting the bignum
tests to be run), the coverage rose to:
94.0% total statement coverage; 76.8% total coverage.
which is now pretty good.
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.
podchecker gives the following errors and warnings:
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 ' $'. It can be
helpful to load the files found directly into
In this month’s module only two files were found with trailing whitespace:
Fixed in PR#6
Add files to .gitignore
None required in this dist.
Perl::Critic will show up many
potential issues for Perl code. By simply running
perlcritic on the
t directories, one can get a further handle on the code’s quality.
In this month’s module, there is no
lib/ directory, hence we just run:
to test everything in the current directory. Two problems are found:
The first issue can be fixed by adding
use strict to the
This issue was fixed in PR#11.
The second issue isn’t really an issue: the test is checking that subclassing the module works, hence the a package statement appears in the test which doesn’t agree with the file’s name. This issue can be safely ignored.
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.
Links look ok.
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
README needs to be updated. Updated in PR#15.
Files missing an email address in copyright
This distribution doesn’t use email addresses in its copyright strings, hence this update is unnecessary.
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
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:
Now look for
.bak file and check differences between it and the output
.txt file, the process looks roughly like this:
Then update the appropriate
.pod files as necessary.
In this month’s distribution only the one file needed to be checked, so that made the process simple. No spelling errors were found.
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.
set of metrics closer to that used on CPANTS, so replace the
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
distribution like so:
or, if the distribution uses
For this month’s distribution the output effectively repeats the warnings found on CPANTS mentioned in the “First impressions” section above. The issues have been addressed by already-submitted PRs.
GitHub issues are disabled; all issues are handled on RT.
Deeper knowledge of the codebase and of cryptography with OpenSSL are needed in order to address the issues in RT. I think I’ll leave them to brighter people than me…
Overview of the pull requests made
- Add Travis-CI configuration file - https://github.com/monken/Crypt-OpenSSL-RSA/pull/5
- Purge trailing whitespace - https://github.com/monken/Crypt-OpenSSL-RSA/pull/6
- Add a LICENSE section to the main module - https://github.com/monken/Crypt-OpenSSL-RSA/pull/7
- Ignore files automatically generated by cover - https://github.com/monken/Crypt-OpenSSL-RSA/pull/8
- Set the minimum Perl version - https://github.com/monken/Crypt-OpenSSL-RSA/pull/9
- Add warnings pragma to main module - https://github.com/monken/Crypt-OpenSSL-RSA/pull/10
- Add strict and warnings pragmas to Makefile.PL - https://github.com/monken/Crypt-OpenSSL-RSA/pull/11
- Convert README to Markdown format - https://github.com/monken/Crypt-OpenSSL-RSA/pull/12
- Update META* files to dist tarball state - https://github.com/monken/Crypt-OpenSSL-RSA/pull/13
- Remove commented-out META resources - https://github.com/monken/Crypt-OpenSSL-RSA/pull/14
- Update copyright year to match dist release year - https://github.com/monken/Crypt-OpenSSL-RSA/pull/15
- Provide reason for skipping bignum tests - https://github.com/monken/Crypt-OpenSSL-RSA/pull/16
- Format code-like text elements in POD as code - https://github.com/monken/Crypt-OpenSSL-RSA/pull/17
Given that it’s been 5 years since the last release and 3 years since the last commit, it could be a while before any of the pull requests get looked at let alone merged. Then again, you never know!