[lit] Force site configs to be run before source-tree configs
ClosedPublic

Authored by zturner on Tue, Sep 12, 12:11 PM.

Details

Summary

This patch simplifies LLVM's lit infrastructure by enforcing an ordering that a site config is always run before a source-tree config. There are multiple ways of running lit. The most common are:

Run lit.py from the source tree against a directory or file also in the source tree, specifying various parameters like llvm_site_config=<foo>

  • This is what the ninja check targets currently do.

Run llvm-lit.py from the build tree against a directory or file in the source tree, specifying no other parameters.

  • This is what developers usually do.

Run llvm-lit.py from the source tree against a directory or file in the build tree.

  • This is necessary to run asan tests, where you cannot currently run against a directory in the source tree.

Run lit.py from the source tree against a directory in the build tree.

  • Not aware of anyone who does this, but in theory it should work.

A significant amount of the complexity from lit config files arises from the fact that inside of a source-tree config file, we don't yet know if the site config has been executed. It is *always* necessary to execute a site config first, because it initializes variables that are handed down through CMake that the main configuration depends on.

This patch solves the problem by emitting a mapping from source tree config file to binary tree site config file in llvm-lit.py. Then, during discovery when we find a config file, we check to see if we have a target mapping for it, and if so we use that instead.

This mechanism is generic enough that it does not affect external users of lit. They will just not have a config mapping defined, and everything will work as normal.

On the other hand, for us it allows us to make many simplifications:

  1. We are guaranteed that a site config will be executed first
  2. Inside of a main config, we no longer have to assume that attributes might not be present and use getattr everywhere.
  3. We no longer have to pass parameters such as --param llvm_site_config=<path> on the command line.
  4. It is future-proof, meaning you don't have to edit llvm-lit.in to add support for new projects.
  5. All of the duplicated logic of trying various fallback mechanisms of finding a site config from the main config are now gone.

One potentially noteworthy thing that was required to implement this change is that whereas the ninja check targets previously used the first method to spawn lit, they now use the second. In particular, the first method is now deprecated and no longer works.

I have tested the following:

  • Running check targets ninja check-llvm, ninja check-llvm-unit, ninja check-lld, ninja check-clang, ninja check-clang-unit
  • Running llvm-lit.py against individual directories in the llvm/test, llvm/unittests, lld/test, clang/test, clang/unittests folder.
  • Running lit.py against corresponding directories in the build folder.

I think these three scenarios are enough to show that everything works, but if someone wants to help me test compiler-rt that would be great.

Diff Detail

Repository
rL LLVM
zturner created this revision.Tue, Sep 12, 12:11 PM
zturner added a reviewer: dlj.
rnk added a comment.Tue, Sep 12, 1:58 PM

This is awesome! phab doesn't tell you the diffstat, so I patched it in, and it is: 177 insertions(+), 788 deletions(-) Nobody understood that code anyway, so this is great.

We should wait to gather more consensus before committing, though.

In D37756#868616, @rnk wrote:

This is awesome! phab doesn't tell you the diffstat, so I patched it in, and it is: 177 insertions(+), 788 deletions(-) Nobody understood that code anyway, so this is great.

We should wait to gather more consensus before committing, though.

This all sounds great. Any downsides of which we should be aware?

In D37756#868616, @rnk wrote:

This is awesome! phab doesn't tell you the diffstat, so I patched it in, and it is: 177 insertions(+), 788 deletions(-) Nobody understood that code anyway, so this is great.

We should wait to gather more consensus before committing, though.

This all sounds great. Any downsides of which we should be aware?

None that I'm aware of, but it's hard to deny that there's a lot of black magic and workarounds that have been baked in over the years (in part to workaround the things this patch is trying to solve "for real").

Definitely want to hear from others if there is anything they can think of.

Just to re-iterate, this should have no effect on external projects. All I did was pipe a magic config_map dictionary through the builtin parameter dictionary. If it's not there (which will be the case for non LLVM based projects), the behavior will be the same as it was before this patch

rnk added a comment.Tue, Sep 12, 2:13 PM

I think the main usage model this de-supports is putting tools on PATH and running lit.py directly on tests from the source directory like this:

PATH=$PATH:$build/bin llvm/utils/lit/lit.py llvm/test/Debug/X86/foo.ll

This patch removes the logic from lit.cfg that looks for llvm-config on PATH and tries to use that to find the site config relative to the build directory. Anyone still doing this is encouraged to use llvm-lit[.py] instead.

If you use cmake to generate Visual Studio projects on Windows, do they use lit to run the tests? If so, do they run with this patch correctly? I have no opinion one way or the other but it's an environment nobody seems to have thought of before (including me).

dlj added a comment.Tue, Sep 12, 5:09 PM

This looks like an excellent cleanup.

clang-tools-extra/test/lit.cfg
89 ↗(On Diff #114874)

Minor nit: it looks like you can assign this directly instead of using the 'path' intermediate. (This would avoid shadowing imports of os.path.)

Same in other cfg files.

97 ↗(On Diff #114874)

(Another minor nit) This is duplicated at the top, so it should be fine to remove this extra import.

compiler-rt/test/lit.common.cfg
256 ↗(On Diff #114874)

Can this use lit_config.fatal? (I suspect so, but I'm not sure whether this is *always* evaluated in a context where lit_config is available.)

If you use cmake to generate Visual Studio projects on Windows, do they use lit to run the tests? If so, do they run with this patch correctly? I have no opinion one way or the other but it's an environment nobody seems to have thought of before (including me).

I just tested running check-llvm from inside Visual Studio using a Visual Studio generated CMake project and it worked fine. I didn't run the rest of them, since if one works they all should.

If you use cmake to generate Visual Studio projects on Windows, do they use lit to run the tests? If so, do they run with this patch correctly? I have no opinion one way or the other but it's an environment nobody seems to have thought of before (including me).

I just tested running check-llvm from inside Visual Studio using a Visual Studio generated CMake project and it worked fine. I didn't run the rest of them, since if one works they all should.

Awesome, thanks!

MatzeB accepted this revision.Wed, Sep 13, 11:06 AM
MatzeB added a subscriber: MatzeB.

This turned out to be a simple directed change. Looks good to me.
(But maybe wait for further feedback).

llvm/utils/lit/lit/discovery.py
43–49 ↗(On Diff #114874)

Add a comment to explain what we use this for.

This revision is now accepted and ready to land.Wed, Sep 13, 11:06 AM

If nobody has any further comments I'd like to try to land this today.

dlj accepted this revision.Wed, Sep 13, 2:58 PM

I did have a couple of remaining formatting suggestions:
https://reviews.llvm.org/D37756#868958

But I'm excited to see this change, with or without those tweaks.

In D37756#870195, @dlj wrote:

I did have a couple of remaining formatting suggestions:
https://reviews.llvm.org/D37756#868958

But I'm excited to see this change, with or without those tweaks.

Maybe I'll fix the duplicate import in this one, and the rest in a followup? I think the other issues are already fixed in the subsequent followup patches that introduces the shared code, so it's probably easier to just let it go in then.

This revision was automatically updated to reflect the committed changes.
jordan_rose added a subscriber: jordan_rose.
  • Run lit.py from the source tree against a directory in the build tree.

    Not aware of anyone who does this, but in theory it should work.

For the record this is my most common workflow when working with Swift, so I'm glad to hear it'll keep working. I suspect it's the most common workflow for anyone using lit outside of LLVM itself who hasn't bothered to make an equivalent of llvm-lit.py.

I'm unhappy that I can't run lit's own tests without having to configure, though. That seems like a regression.

(Yes, they do depend on FileCheck, but I might have one of those already installed on my system.)

I'm unhappy that I can't run lit's own tests without having to configure, though. That seems like a regression.

(Yes, they do depend on FileCheck, but I might have one of those already installed on my system.)

Can you clarify? What are the exact set of commands you used to run, and what do you have to do now?

jordan_rose added a comment.EditedTue, Sep 19, 3:45 PM

Previously I was just able to do this:

utils/lit/lit.py -sv utils/lit/tests

as long as FileCheck was in my PATH. I can see why that's not a meaningful confiuse case for a real project that actually has an interesting lit.site.cfg, but for just working on lit it seems redundant.