This is an archive of the discontinued LLVM Phabricator instance.

Resubmit (Fixed) "[lit] Force site configs to be run before source tree configs"
AcceptedPublic

Authored by zturner on Sep 15 2017, 12:23 PM.

Details

Summary

Despite a couple of targeted fixes that went through, there were still problems in some standalone build configurations. This turned out to be rather tricky, but can be explained simply:

Before: the check targets would run lit.py from the source tree
After: the check targets would run llvm-lit.py from the build tree

The interim fixes that were applied only made it try to run the llvm-lit.py from the *clang* build tree, which since this is a standalone environment is not going to be correct. We emit a bunch of source -> build tree mappings into the llvm-lit.py for example, and these would be all wrong if we use the wrong llvm-lit.py. Bottom line is that llvm-lit.py is tied to a particular build tree, and if you're going to run tests, it needs to be there.

The fix I've done here is to actually do that. We already have ${LLVM_MAIN_SRC_DIR} as a result of running llvm-config, so I can add an out-of-tree subdirectory to reach in and grab llvm/utils/llvm-lit. This causes it to run the lit configure step and generate llvm-lit.py into <compiler-rt-build-dir>/bin.

After this, everything works.

I'm submitting this as two separate patches. The first is the original patch which is broken. The second is the full diff including new changes to fix. This way you can diff v1 and v2 and see only the new stuff.

Diff Detail

Event Timeline

zturner created this revision.Sep 15 2017, 12:23 PM
zturner updated this revision to Diff 115457.Sep 15 2017, 12:23 PM

Update with fixes

rnk edited edge metadata.Sep 15 2017, 12:52 PM

This is problematic because people already complain about how compiler-rt depends on the LLVM source tree for gtest sources: https://bugs.llvm.org/show_bug.cgi?id=33693

In D37920#872453, @rnk wrote:

This is problematic because people already complain about how compiler-rt depends on the LLVM source tree for gtest sources: https://bugs.llvm.org/show_bug.cgi?id=33693

I don't think this is making things any worse. Even before this patch, the compiler-rt test suite need lit.py to run which is, as you guessed, in the source tree. Indeed, the lit command it generates points directly into the source tree.

That aside, how important is this use case? It doesn't appear to be currently supported given that a lit.py already needs to exist, and if only one person is complaining, then I think the complexity reduction from the new lit workflow outweighs one person's pain (granted, I'm biased, but still)

Another thing to think about. We already support a -DLIT_COMMAND=<path> cmake argument. If the user specifies that, it will just use whatever lit.py they specify.

You don't actually need a llvm-lit.py if you're pointing into the build tree, so a user could already download just lit.py, set the cmake variable, and then the check targets should work.

zturner updated this revision to Diff 115492.Sep 15 2017, 2:19 PM

Incorporated suggestions from beanz@ offline. Specifically, the concern is that someone could download and install lit via a package manager, but we would still try to add an out of tree source directory. This update makes it so that we generate llvm-lit iff a source tree is available, and we *use* the generated llvm-lit whenever -DLLVM_EXTERNAL_LIT is not specified.

rnk accepted this revision.Sep 15 2017, 2:28 PM

Looks good, sounds like a plan: run lit.py like we used to if we don't have or can't find an llvm-lit.in to configure.

This revision is now accepted and ready to land.Sep 15 2017, 2:28 PM
eugenis edited edge metadata.Sep 15 2017, 2:49 PM

compiler-rt part LGTM

compiler-rt/CMakeLists.txt
352

What's the point of generating llvm-lit when LLVM_EXTERNAL_LIT is specified?

zturner added inline comments.Sep 15 2017, 3:01 PM
compiler-rt/CMakeLists.txt
352

My thinking was mostly just "might as well". But it's not imperative. On the other hand, it doesn't hurt anything, and we have to do it anyway if LLVM_EXTERNAL_LIT is not specified.

I will try to land this now, will be watching the sanitizer bots this time.