[Lldb-commits] [PATCH] D54567: Fix LLDB's lit files

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

In https://reviews.llvm.org/D54567#1300641, @JDevlieghere wrote:

> In https://reviews.llvm.org/D54567#1300459, @zturner wrote:
>
> > I'm not sure how hard it would be.
> >
> > One problem is that dotest supports not just choosing a compiler, but choosing multiple compilers, as well as multiple architectures and it runs the test suite over the cross product of all of these.  That's hard to express in CMake.  This is why, for example, people end up subverting it entirely for productionizing test suite runs, such as what you see on the ubuntu bot linked earlier.  It doesn't even use the CMake variables for running the dotest suite, it just has scripts that build command lines and runs them.
>
>
> Makes sense. Just to be clear, I'm not advocating running a product for the lit suite, just having one option that controls both dotest and lit.
>
> > There is another issue I'm aware of, which is that some people's compilers have version numbers embedded in the binary name.  Right now the code that uses `LLDB_LIT_TOOLS_DIR` to find the binaries won't handle these cases, because it looks specifically for `clang` and `clang++`, but not, for example, `clang-7.0` or `clang++-hexagon-7.0`.
>
> How is this handled today? Do we have tests that do something like that?


The tests themselves don't try to do it, usually our tests try to be compiler agnostic, but then people will run the tests with one or more different compilers.  I think people use dotest in this manner, by specifying a path to a compiler with a version number in it, but I think it may be all downstream stuff, with no bot coverage.  Mostly they do it by specifying `LLDB_TEST_C_COMPILER=/path/to/clang-7.0` or something, and dotest is smart enough to figure this out.  We could certainly have this same logic in the lit files though once we get to that point.

> 
> 
>> I do think an iterative approach is better though.  This is already a big change and as this thread (and the previous patch which is what I'm trying to fix) shows, people use things in a lot of different ways so changing something has potential for lots of breakage in subtle ways.  So I still kind of prefer doing things incrementally, letting people tell me what's broken, and then working on a solution.
> 
> I'm all for iteration! We just wanna make sure we share the same "end goal".
> 
>> We could try to converge on the single `LLDB_LIT_TOOLS_DIR` approach for both dotest as well as the lit suite, because having one variable with simple semantics is nice.  But I think we should worry first about getting to a known good baseline and then working incrementally to make simplifications.
> 
> I'm worried that the directory approach is incompatible with settings a specific compiler (like gcc).

I don't think the directory approach is fundamentally incompatible with using non-clang compilers.  But it's important to differentiate what the test suite itself supports and what the test suite supports //when invoked via a CMake-generated target//.  That is to say, we don't have to expose all the power of the underlying test suite through CMake.  I actually think trying to do so is a mistake, because you're often going to be circumventing it anyway to run the test suite in some special way that wasn't how you configured CMake.  dotest.py, for example, takes tons of different command line arguments, many of which are only available if you invoke it directly, as the CMake-generated command line will never use those arguments.

So I think this is similar.  You can pass arbitrary parameters to lit when you point it to a test directory, so there's fundamental limitation in terms of what's possible this way.  Of course, we don't currently //recognize// any, and yes it means you can't specify a specific compiler binary (such as GCC anymore), but I'm not entirely sure how useful that is in the first place, because we're talking about a static CMake configuration.

So I think as far as what we expose through CMake, it should be as simple as possible and support the developer use case and the most common buildbot use cases, but more advanced use cases are still supported by directly invoking llvm-lit.py with some special arguments (which we would still need to teach the LLDB lit configuration files to understand).

Hopefully this all makes sense.


https://reviews.llvm.org/D54567