Thursday, April 24, 2014

A non-exceptional failure in the catch block

In various development versions of Crypt::SSLeay, I was specifying Devel::CheckLib as a dependency. In response to a request from a user, I decided to make it optional:

sub filter_libs {
    my $opt = shift;
    my $libs = shift;
    try {
       require Devel::CheckLib;
       Devel::CheckLib->import;
    }
    catch {
       return $libs;
    };

OUCH!

I am assuming the intent is obvious: If Devel::CheckLib is not available, just return the same list of libraries we were given. When using eval, it is easy to remember that eval { return "Hello" } does not return from the enclosing function. But, somehow, using the magic keywords try/catch, I forgot that! So, this check is completely useless, and execution continues regardless of the outcome of require.

Embarrassing, but true.

A simple:

return unless eval {
   require Devel::CheckLib;
   Devel::CheckLib->import;
   1;
}

would have worked. In fact, now that I am thinking about this, it probably would have been even better to use:

return $libs unless eval { require Devel::CheckLib; 1 };
    Devel::CheckLib->import;

That way, if the skip mechanism does not work, at least Makefile.PL crashes with a decent explanation.

3 comments:

  1. Your metadata is still wrong -- https://metacpan.org/source/NANIS/Crypt-SSLeay-0.72/META.json#L63

    That's because of this line: https://metacpan.org/source/NANIS/Crypt-SSLeay-0.72/Makefile.PL#L117 -- it should be prereqs => { ... }

    Moreover, things used in Makefile.PL should be *configure* prereqs, not build.

    If you're generating your META.* via Makefile.PL, be careful of META_ADD vs. META_MERGE - you don't want conditional prereqs (evaluated according to the conditions on *your* machine) to make it into META.* that everyone sees.

    -Ether (ether@cpan.org)

    ReplyDelete
    Replies
    1. Thank you. I will fix that. I managed to squeeze way too many errors into this one.

      Delete
    2. I am trying not to require Devel::CheckLib ... But, also convey via the meta mechanism that it is useful for this build. Maybe I misunderstood `recommends`.

      Delete