[llvm-commits] [llvm] r122341 - in /llvm/trunk: docs/TestingGuide.html test/lit.cfg utils/lit/lit/TestRunner.py

[Tobias Grosser]

On 12/21/2010 02:07 PM, David A. Greene wrote:
> Tobias Grosser<grosser at fim.uni-passau.de>  writes:
>
>> On 12/21/2010 12:37 PM, David A. Greene wrote:
>>> Tobias Grosser<grosser at fim.uni-passau.de>   writes:
>>>
>>>> I just read your replies and have still an open question. You replied
>>>> the patch provided by me, that prepends the llvm tools directory to the
>>>> path will not solve the problem as it is too brittle. Can you explain this.
>>>
>>> Because you never really know what else lit might prepend to the PATH,
>>> as the PR demonstrates.
>> This is something we can control and nothing I expect to happen very
>> often. Lit is prepending exactly three items.
>
> Except testing was broken for over a year for me and we could never
> figure out why.  I really, really don't trust magic paths.

OK.

>>> It's just more solid to say what you really
>>> mean.
>> Sure. However replacing strings like " opt " is nothing that I would
>> expect while writing test files. So this step is not 100% solid.
>
> Why not?  It's a tool name, right?

Because until now only things prepended with % where replaced. So a
check:

; RUN: opt %s -pass -analyze | grep "loop1: opt simple, loop2: opt none"

Here 'opt' would be replaced, right?

Or if the tool is not freestanding. Maybe someone is using a shell 
subexpression to execute a tool:

; RUN: if `opt %s -pass -analyze | grep marker` : ...

Whould 'opt' be replaced in this line?

Those are definitely not very common use case, however they are valid 
test files, if not documented otherwise.
The failure will be obvious for the first one, however less obvious for 
the second one (where still the wrong opt is gonna be executed)

In any case I just wanted to point out, that this might happen. I am OK 
with taking the risk, as it is very convenient to have the absolute 
paths in the command line, and the cases I where your solution is 
incorrect could be forbidden by policy.
Using %opt instead of opt is probably more obvious, however I believe 
this would complicate the test files unnecessarily.

>> I am just worried we gonna forget to add tools to this explicit list.
>
> Yes, that's a risk.
>
>> Do you think we can in addition fix the PATH order as proposed in my
>> patch? In case we forget to add some tools, it would still do the
>> right thing. We just do not get the convenient absolute path and are
>> doomed if someone prepends another path. I believe this will not
>> happen too often and I think this would be a bug in lit. We can
>> e.g. add a test case, that tests that the first component of the path
>> is the tools directory.
>
> I'm a little hesitant to do this fix two different ways.  I'm not the
> owner of lit though, so go ahead and propose/commit it if you want.
No need to push my changes in. However if you agree it is a good thing, 
or at least does not hurt to fix the path order, I would go ahead and 
commit.

>> Furthermore it will fix the problem for the moment, without failing
>> any builds. So you have some time to figure out what went wrong.
>
> I think I know what went wrong.  Just have to go fix it now.  :)

Good luck.

Tobi