On 2015-06-22 12:36 PM, Sam Kington wrote:

Hi,

Delurking, because the latest version of Dancer is causing our unit tests at $WORK to start creating empty log files inside the current working directory. This is easily-reproducible: e.g.

Issue created and potential patch attached at https://github.com/PerlDancer/Dancer/issues/1121 I'm poking the rest of the crew to know if we want to just revert or commit that patch. A decision will be forthcoming shortly. Joy, `/anick

...

sam@chimera64:tmp$ cat spamlogfile.pl #!/usr/bin/env perl

use Dancer qw(halt); sam@chimera64:tmp$ rm -fr logs; perl spamlogfile.pl sam@chimera64:tmp$ ls -lah logs/ total 40K drwxr-xr-x 2 sam sam 4.0K Jun 22 17:01 . drwxrwxrwt 10 root root 36K Jun 22 17:01 .. -rw-r--r-- 1 sam sam 0 Jun 22 17:01 development.log

A workaround is to say use Dancer qw(:syntax), but there are two problems with that:

(1) this is a change in behaviour that wasn’t documented as such, and it requires all auxiliary non-server Dancer code to be changed; and

(2) passing :syntax imports all sorts of Dancer symbols that can potentially clash with a parent class.

I’ve spent the afternoon debugging point 2. Our end-to-end tests run under Test::Dancer, and we have a separate class that tests individual method calls as if they were Dancer requests (including setting the status code in case of errors). It previously said use Dancer () and explicitly called Dancer::status - and under Dancer 1.3138, running unit tests using this class would generate spurious empty log files.

I patched that to say use Dancer qw(:syntax) - and now the code fails because the parent class implements methods true, false and redirect, which the method call class never gets to call because it’s had Dancer’s symbols stuck in its own package instead. So this now needs to say use Dancer qw(:syntax !true !false !redirect), and in practice I needed to write an automated test script that checked that all of a parent class’s methods were explicitly not imported from Dancer, because if I change anything upstream I need to change the import statement in the child classes.

The problem appears to have been introduced in https://github.com/PerlDancer/Dancer/commit/e9e4e087528c2734dd6e9c922105eac3...

The actual problem is further down in Dancer::Config - line 227 in 1.3138 - where the settings are read. $SETTINGS at this point contains default values, and part of setting confdir involves creating a logs subdirectory. Previously the code wouldn’t get this far.

Presumably a workaround would be to not create a log directory immediately, but then if you were running a real Dancer server you’d want to know as soon as possible that your log directory wasn’t writable, so that’s not going to be acceptable to many people.

Alternatively, any code that says e.g. use Dancer qw(halt status) should be seen as clearly not wanting to actually start a Dancer server or anything like that. That sounds like a scary change to make, though.

Or you could defer parsing of the Dancer config until something actually happens. That’s also vague and terrifying.

Or just revert that particular commit / PR. I don’t fully understand why it does what it does, though. If there’s a way of cleanly telling apart (1) a hash of settings that was explicitly passed to Dancer from (2) the default settings that, chances are, you didn’t intend to use, that would be the best solution to my mind.

Sam