Paul Cochrane

Paul Cochrane

Multi-purpose software developer and geek

Helping to clean CPAN: January 2016

8 minute read

… 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: Tcl

Tcl is a Tcl extension module for 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 27th of Feb 2011
  • 2 pull requests outstanding, 0 closed, PRs from 2012 and 2013
  • 0 open issues in GitHub; 18 open RT tickets
  • latest release: 11th of Feb, 2011 (https://metacpan.org/release/Tcl)
  • 2 reverse dependencies via metacpan
  • Core kwalitee metrics passing (from http://cpants.cpanauthors.org/dist/Tcl):
  • Extra kwalitee metrics failing:
    • use warnings (should use warnings pragma)
  • Experimental kwalitee metrics failing:
    • meta yml has provides (should mention modules dist provides)
    • has separate license file (should have separate license file)

This tells me that the project hasn’t been that active for a while, but could still use some TLC.

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.

  • EUMM project
  • README in plain text: convert to Markdown?
  • perl Makefile.PL runs ok
  • running make test gives an error in t/call.t
  • no .travis.yml
  • doesn’t use coveralls
  • copyright info needs updating
  • doesn’t provide a META.json
  • doesn’t use a lib/ dir for code; (all code is in base dir)
  • uses XS

Running the test suite gave a segfault, which was the catalyst for one of my first pull requests (PR#4).

make test failure in call.t

prove states that “All 15 subtests passed”, however the test overall fails. To run the test individually, need to run

$ PERL_DL_NONLAZY=1 perl -Iblib/lib -Iblib/arch t/call.t

which gives a segfault after the last test (it turns out that using PERL_DL_NONLAZY=1 actually has no effect).

Running this through the debugger

$ perl -d -Iblib/lib -Iblib/arch t/call.t

shows that all tests pass. However, the segfault occurs when quitting out of the debugger. This smells of unfreed memory in maybe the XS code.

Tried using tcl8.4-dev in the Travis builds to make things pass, however that didn’t help. What’s odd is that 5.20.3 passes (even before the tcl8.4 change). Is this maybe a Perl issue (however unlikely that may be)?

Running t/call.t through gdb:

$ gdb --args perl -Iblib/lib -Iblib/arch t/call.t

gives

ok 15
[New LWP 30652]
[LWP 30652 exited]

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff6956ccc in XS_Tcl_DeleteCommand () from blib/arch/auto/Tcl/Tcl.so
(gdb) where
#0  0x00007ffff6956ccc in XS_Tcl_DeleteCommand () from blib/arch/auto/Tcl/Tcl.so
#1  0x00000000004a5920 in Perl_pp_entersub ()
#2  0x000000000049e753 in Perl_runops_standard ()
#3  0x0000000000435677 in Perl_call_sv ()
#4  0x00000000004ae667 in S_curse ()
#5  0x00000000004aef8d in Perl_sv_clear ()
#6  0x00000000004af31f in Perl_sv_free2 ()
#7  0x00000000004a6d52 in S_visit ()
#8  0x00000000004af72c in Perl_sv_clean_objs ()
#9  0x0000000000437848 in perl_destruct ()
#10 0x000000000041de14 in main ()

Building and testing with perl-5.8.9:

t/call.t ........... 1/15       (in cleanup) Can't call method "DeleteCommand" on an undefined value at
/home/cochrane/Projekte/OSSProjekte/tcl.pm/blib/lib/Tcl.pm line 640 during global destruction.

Line 640 of Tcl.pm is:

$interp->DeleteCommand($tclname) if defined $tclname;

which suggests that $interp is undefined at this point.

Note that it’s important to rebuild the shared library after making changes to Tcl.pm or when trying out a different Perl version:

$ make realclean && perl Makefile.PL && make && perl -Iblib/lib -Iblib /arch t/call.t

Not doing this probably explains why a newer Perl version seemed to pass the test suite even though an older version didn’t.

On a 32 bit system (perl 5.14.2; tcl-dev8.5; debian/wheezy; this would be a system matching similar systems on which the code was developed) the t/call.t test works, nevertheless here one does need PERL_DL_NONLAZY=1 so that the code can run.

Interestingly enough, there is this code in t/disposal-subs.t which doesn’t really seem to be contributing to the test, however appears after having called Perl subs within Tcl ‘after’ calls (which t/call.t also has):

$int->call('after', 3000, 'set var fafafa');
$int->icall('vwait', 'var'); # will wait for 3 seconds

Adding this code after the call('after', 250, sub {}) in t/call.t, the segfault disappears from t/call.t.

Commenting out the respective code in t/disposal-subs.t causes the code to segfault at the end of a test run just like t/call.t. Also, the number of Perl code objects in Tcl keeps increasing (as part of debug output of the t/disposal-subs.t test shows).

==> added a workaround in PR#4

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 # 0 errors
$ find ./ -name '*.pm' -or -name '*.pod' | xargs podchecker |& grep WARN # 0 warnings

… so nothing needs to be done here.

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 ' $'. It can be helpful to load the files found directly into vim:

$ vim $(git grep ' $' | cut -d':' -f1 | sort | uniq)

which resulted in 6 files needing to be checked.

Add files to .gitignore

Need to add MYMETA.* and cover_db to .gitignore.

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 .
  • strictures errors found in test files and Tcl.pm
  • most errors are strictures errors. Subroutine prototypes are also used.

After a discussion with the maintainer, it was decided not to add warnings pragmas since he is averse to their use (as outlined in the discussion surrounding PR#6. RIBASUSHI pointed out that they add a small amount of time to the module’s startup time. A common use case for this module is to run small pieces of Tcl code often, hence the startup time can become significant in terms of the module’s usage.

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:

$ dzil cover

In this distribution, the coverage is:

58.6% total coverage

… and all tests pass. Huh? Why not when running normally?

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://'

All links ok.

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.

One minor typo in Tcl.pm found.

GitHub issues

  • none to address

RT ticket analysis

  • RT#25822 should be closed as the patch was merged in 2007.

Overview of the pull requests made

Conclusion

Many thanks to VKON for the discussions and for merging my pull requests!

I’d planned to do a few more things to help out, for instance, these items made it onto my cleanup todo list:

  • use :encoding(UTF-8) instead of :utf8
  • use Test::More in test suite
  • avoid calling DeleteCommand on undefined interp in t/disposal-subs.t
  • move Tcl.pm into a lib/ dir?
  • add a META.json

however the month came to an end and I ran out of time and had to move on to other things.

Support

If you liked this post and want to see more like this, please buy me a coffee!

buy me a coffee logo

You may also enjoy

A dinosaur learns poetry

15 minute read

Not a real dinosaur and not real poetry. Just me updating my Python project setup knowledge.

Avoiding Ansible apt_key on Debian

12 minute read

The apt_key Ansible module has been deprecated and there isn’t a drop-in replacement. There is a workaround available which achieves the same result but req...