It was a relief to be able to do a cpan-outdated | cpanm on my trusty old Lenovo 3000 N100.
Test failure in Dancer
Unfortunately, the Dancer build failed again, as it had failed many times before. I hadn’t had a chance to look into the failure, and had perfunctorily filed an issue with the developers without being able to give them solid information.
The problem was a test failure in t/14_serializer/06_api.t. Here is a relevant part of the file:
# Ensure the temp directory is resolved before we destroy the environment
# (since it needs the environment to do so, and caches after the first time)
File::Spec->tmpdir;
%ENV = (
'REQUEST_METHOD' => 'GET',
'HTTP_CONTENT_TYPE' => 'application/json',
'HTTP_ACCEPT' => 'text/xml',
'HTTP_ACCEPT_TYPE' => 'text/x-yaml',
'PATH_INFO' => '/',
);
my $req = Dancer::Request->new( env => \%ENV );And, here is the test failure:
Error in tempfile() using template \XXXXXXXXXX: Could not create temp file \IpjtLLGiT3: Permission denied at C:/opt/perl-5.22.0/site/lib/HTTP/Body/OctetStream.pm line 33.
# Looks like you planned 18 tests but ran 15.
# Looks like your test exited with 13 just after 15.
t\14_serializer\06_api.t ............................
Dubious, test returned 13 (wstat 3328, 0xd00)
Failed 3/18 subtestsWell, why the heck are you trying to create a temporary file in the root directory?
See, the comment right there says the first call invocation of File::Spec->tmpdir caches the value so that HTTP::Body::OctetStream can figure out the temporary directory even though we clobber %ENV right before creating a Dancer::Request object.
After taking a look at File::Spec::Unix->_cached_tmpdir (from which File::Spec::Win32 inherits its caching implementation), the problem became obvious:
# Retrieve the cached tmpdir, checking first whether relevant env vars have
# changed and invalidated the cache.
sub _cached_tmpdir {
shift;
local $^W;
return if grep $ENV{$_} ne $tmpenv{$_}, @_;
return $tmpdir;
}The cached value is destroyed when the test script clobbers the environment!
Here is where File::Spec::Win32->tmpdir looks to figure out the temporary directory:
$tmpdir = $_[0]->_tmpdir( map( $ENV{$_}, qw(TMPDIR TEMP TMP) ),
'SYS:/temp',
'C:\system\temp',
'C:/temp',
'/tmp',
'/' );I am guessing SYS:/temp is a holdover from Netware. In any case, my system does not have a C:\system\temp, C:/temp, or /tmp directory.
So when File::Spec invalidates the cached value of the temporary directory like any decent cache should when the underlying values change, it cannot find the variables TMPDIR, TEMP, or TMP in the environment, so it tries the hardcoded locations. None of them work, so it tries the root directory as a last resort.
Well, I don’t think random Perl module builds being able to write to the root of the hard drive would be a good idea now, would it? So, it fails.
But, why doesn’t this test fail on pretty much any other system?
Note that the comment is wrong on all operating systems: The cached value of the temporary directory is invalidated on all of them.
On Unix-like systems, File::Spec only considers TMPDIR and /tmp.
Therefore, on any system where TMPDIR points to something other than /tmp, the script will do the wrong thing by disregarding the user’s preference. For example, on my MacbookPro:
$ perl -MFile::Spec -E 'say File::Spec->tmpdir; %ENV = (); say File::Spec->tmpdir'
/var/folders/fm/.../T
/tmpIn the case of Windows systems where the test script does not fail, I am assuming that people who set up Windows machines for CPANTesters added a C:\Temp at some point.
Here is the pull request.
Since all platform-specific specializations of File::Spec look first at $ENV{TMPDIR}, my proposed fix is to include that in %ENV, and to set it to whatever the original tmpdir invocation returns.
Once again, wrong comments are worse than no comments ;-)
perl 5.23.4 breaks Imager
If you look at the CPANTesters results for Imager, you’ll notice a number of failures when built with perl 5.23.4 on three different operating systems: Mac OS X, Windows 10, and Linux.
However, there are also four passing results on Linux.
If you look carefully, you’ll also notice that both the passing and the failing tests on Linux are from the same machines.
Weird, right?
No need for suspense here: The passing Linux tests are all from September 22–27 whereas the failing Linux tests are from the beginning of October.
What happened between the last passing test and the first failing test?
Dave Mitchell’s change to scope.c was committed on October 2. Here is the diff:
index 9768c30..1b89186 100644 (file)
--- a/scope.c
+++ b/scope.c
@@ -31,6 +31,10 @@ Perl_stack_grow(pTHX_ SV **sp, SV **p, SSize_t n)
{
PERL_ARGS_ASSERT_STACK_GROW;
+ if (n < 0)
+ Perl_croak(aTHX_
+ "panic: stack_grow() negative count (%"IVdf")", (IV)n);
+
PL_stack_sp = sp;
#ifndef STRESS_REALLOC
av_extend(PL_curstack, (p - PL_stack_base) + (n) + 128);Wait … what was the test failure?
panic: stack_grow() negative count (-1) at C:\Users\sinan\.cpanm\work\1445420607.16968\Imager-1.003\blib\lib/Imager.pm line 4043.
# Looks like you planned 22 tests but ran 10.
# Looks like your test exited with 25 just after 10.
t\100-base\030-countc.t .........
Dubious, test returned 25 (wstat 6400, 0x1900)
Failed 12/22 subtests _get_anonymous_color_histo in Imager.xs calls _get_anonymous_color_histo in image.c.
The latter routine returns -1 if the number of colors exceeds the maxc parameter.
In Imager.xs, that return value is used to extend the return stack.
Using EXTEND with a negative parameter value causes the check in Dave Mitchell’s commit to be hit in t/100-base/030-count.t:
@colour_usage = $im->getcolorusage(maxcolors => 2);
is(@colour_usage, 0, 'test overflow check');The invocation of getcolorusage with the maxcolors argument causes a panic, and the test is aborted.
Arguably, the underlying EXTEND with a negative argument has always been wrong. I have not investigated the behavior of EXTEND through Perl releases, but, logically, adjusting the return stack by a negative value seems like it could not have a legitimate use.
This fits the following in Dave’s reasoning:
In detail, the problems and their solutions are:
1) N is a signed value: if negative, it could be an indication of a caller’s invalid logic or wrapping in the caller’s code. This should trigger a panic. Make it so by adding an extra test to EXTEND() to always call stack_grow if negative, then add a check and panic in stack_grow() (and other places too). This extra test will be constant folded when EXTEND() is called with a literal N.
Bug report and proposed patch.
PS: You can discuss this post on /r/perl.