[LLVMdev] lit: deprecating trailing \ in RUN lines

[Alp Toker]

On 08/12/2013 11:22, Chandler Carruth wrote:
> On Sun, Dec 8, 2013 at 2:11 AM, Alp Toker <alp at nuanti.com 
> <mailto:alp at nuanti.com>> wrote:
>
>     I'd like to propose deprecating and shortly thereafter removing
>     the lit test runner feature that concatenates RUN lines ending in
>     a trailing \
>
>
> I'm really opposed to this. Especially for the Clang test suite where 
> run lines are often *very* long and hard to organize, read, and edit 
> without this feature.
>
>
>     *Rationale:**
>     *
>
>       * Trailing \ has a special meaning in various language standards
>         that we support nowadays. In the C preprocessor, for example,
>         it's handled _before_ comments. Various compilers handle this
>         differently and it introduces needless incompatibilities.
>
> What incompatibilities? I've never had this be an issue.

It's an issue if you try to run the clang tests against other compilers, 
say to check compatibility with MSVC.

The problem is that "the trailing backslash on a continued line is 
commonly referred to as a backslash-newline" -- ie. it's handled by the 
preprocessor, so has significance rather than being part of the comment.

That causes dissonance between what the compiler sees and what lit.py 
sees for no particularly good reason. One of the nice properties of lit 
tests is that they're also valid compiler inputs, so trailing slash is a 
bit unfortunate.

It less of a problem if you're just consuming the suite with make 
check-all, more of a problem for authoring.


>       * Forgetting the trailing \ between two RUN lines causes the
>         lines to be run individually. People have checked in tests
>         which they believed were getting run whereas the features
>         being tested were actually silently broken. I've been
>         committing fixes for some of these but it's exceptionally
>         time-consuming to hunt them down after the fact.
>
> I'd like to understand the rate at which this happens (per RUN-line? 
> per test-file?). It's never been a problem for me, but that is in part 
> because I check that my tests fail without my change in addition to 
> passing with my change.

If only everyone did check their changes.

See r194071 where trailing backslash caused a test to always succeed, 
and r194073 for the kind of long-term a broken test has on code quality.

Looking at the SVN log for test/ in clang, and to a lesser extent LLVM 
core, I've been doing nearly all the no-op test fixes in the last few 
months. Not all of them are related to trailing \ but those are the most 
pernicious kinds.

It's a bad idea to gamble that I or someone else will always be around 
taking the time to manually verify old tests to see if they do what 
they're meant to do.


>       * Removing trailing \ will introduce the neat property that one
>         RUN line corresponds precisely to one command that's executed.
>         This is good for humans and will enable simplifications in the
>         test runner.
>
> FWIW, I've never really had a problem that needed this. The RUN: forms 
> a prefix of a shell script in my head, and I know how to read shell 
> scripts including multiple lines.

The transformations lit does are really too complex and there's at least 
one known bug to do with closed pipes that's contributing to no-op tests 
(think the discussion thread was on cfe-dev).

In a nutshell, the script output lit forms right now is not likely not 
the pipeline you had in your head ;-)

We need to simplify this stuff to fix no-op test issues, and also to 
achieve improved source line information.

>
>       * Eliminating the trailing \ syntax will unblock work on
>         improved failure source locations in lit. Right now, when the
>         builders encounter a test RUN failure it's a matter of
>         guesswork as to which RUN line is failing, and the cycle of
>         commit-fix-and-watch-buildbots is laborious. We've all wasted
>         time with this at some point and can totally do better.
>
> While I would very much enjoy better failure reporting, I don't really 
> understand why it needs this. We have a in-python parser for the RUN: 
> lines which understands what lines a "command" spans?
>
> Anyways, even though I would *really* like better failure reporting 
> from lit, not at the cost of less readable tests. That's the tail 
> wagging the dog IMO.

So, my contention is that the \ is not making the long lines more 
readable, just pasting over the complexity and hiding bugs.

After all, long pipelines aren't how people use the LLVM tools in the 
real world and they totally miss out on testing file IO, losing 
stdout/stderr distinctions etc.

Another option is to use a different break marker and require RUN-NEXT: 
on continuation lines. But my view is that long RUN lines could do with 
simplification anyway, so removing the feature is a better way forward.

I'll throw the ball in your court to see if you have a better solution 
going forward?

Alp.



-- 
http://www.nuanti.com
the browser experts

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20131208/8b8991bd/attachment.html>