This is an archive of the discontinued LLVM Phabricator instance.

[cmake] Make LIT_COMMAND configurable and improve fallback support
ClosedPublic

Authored by mgorny on Sep 30 2016, 12:50 AM.

Details

Summary

Make LIT_COMMAND configurable, use source tree only when actually available and extend the default search to other common executable names 'lit.py' and 'lit', in order to increase uniformity between all LLVM projects and support using installed lit.

Changing the conditional used to determine whether in-tree or external lit is being used covers the case when LLVM_MAIN_SRC_DIR is defined but does not exist (anymore). In this case, the functions falls back to looking for installed lit rather than attempting to use a non-existing path. The same conditional is used in clang already.

Making LIT_COMMAND a cache variable in case the source tree variant is used serves two purposes. Firstly, it increases uniformity between the two branches since find_program() implicitly makes LIT_COMMAND a cache variable. Secondly, it allows overriding the lit executable used to run the tests when the LLVM source tree is provided. Gentoo is planning to use this to use installed (and byte-compiled) lit instead of re-compiling it in every LLVM project.

Extending default search is meant to increase uniformity between different LLVM projects. The 'lit.py' name is already used by a few of them, and 'lit' is the name used by utils/lit/setup.py when installing.

Diff Detail

Event Timeline

mgorny updated this revision to Diff 72996.Sep 30 2016, 12:50 AM
mgorny retitled this revision from to [cmake] Make LIT_COMMAND configurable and support more 'standard' names.
mgorny updated this object.
mgorny added reviewers: garious, ddunbar, chandlerc.
mgorny added a subscriber: llvm-commits.
mgorny updated this revision to Diff 72997.Sep 30 2016, 12:58 AM
mgorny retitled this revision from [cmake] Make LIT_COMMAND configurable and support more 'standard' names to [cmake] Make LIT_COMMAND configurable and improve fallback support.
mgorny updated this object.
mgorny updated this revision to Diff 73050.Sep 30 2016, 7:39 AM

Updated: fixed find_program() use.

beanz accepted this revision.Sep 30 2016, 2:51 PM
beanz added a reviewer: beanz.

LGTM!

This revision is now accepted and ready to land.Sep 30 2016, 2:51 PM
mgorny added a comment.Oct 1 2016, 2:36 AM

Thanks for the review. Committing in a minute.

This revision was automatically updated to reflect the committed changes.
mgorny added a comment.Oct 1 2016, 5:11 AM

@beanz, it seems that the commit has caused quite a huge breakage due to typo in the default. I've fixed it already but it seems that buildbots are sticking to the old value due to CMakeCache.txt. Any idea how I could force them to reset?

mgorny updated this revision to Diff 73196.Oct 1 2016, 6:30 AM
mgorny edited edge metadata.
mgorny removed rL LLVM as the repository for this revision.

Since there seems to be no clean way of resetting CMakeCache.txt for all buildbots, I have reverted the original change. If I'm not mistaken, this should cause the CMakeCache.txt files to be cleaned up implicitly since LIT_COMMAND will no longer be a cache variable. After all bots rebuild cleanly, it should be possible to reintroduce the change, with the correct default value.

I've updated the patch for that. The difference is that the default needs to be semicolon-separated rather than space-separated (the non-cache 'set' variant implicitly converts ARGN to a semicolon-separated list).

mgorny reopened this revision.Oct 1 2016, 6:30 AM
This revision is now accepted and ready to land.Oct 1 2016, 6:30 AM

I think the bots are clean now. @beanz, would it be ok for me to commit the updated patch? I'm going to do one more round of local testing in the meantime.

mgorny requested a review of this revision.Oct 3 2016, 11:42 PM
mgorny edited edge metadata.
beanz added a comment.Oct 4 2016, 11:00 AM

CMake has no method for cache invalidation. Reverting your patch will not have cleaned up the caches.

CMake has no method for cache invalidation. Reverting your patch will not have cleaned up the caches.

Damn it. Any idea how to proceed then?

@beanz, ok reading CMake docs I think FORCE could help there. That is:

  1. Commit the patch adding FORCE to the cache variable. This will force resetting the value on buildbots where it is set. Since in this branch of code, it currently isn't a cache variable, no people should be affected by forcing the value.
  2. Wait for all bots to rebuild it.
  3. Remove the FORCE to make variable behave normally.

I don't like the idea of committing workarounds for buildbots like this but I guess the only alternative would be to require manual action for every buildbot.

beanz added a comment.Oct 4 2016, 12:53 PM

In the past when I have had these issues I've written temporary workaround code that has inspected the variable for the bad value, then used unset to remove it from the cache and follow the normal flow from there.

Following that model you end up with a block of code that you can put a TODO comment above that says to remove it, and when it should be safe to remove.

mgorny updated this revision to Diff 73542.Oct 4 2016, 1:19 PM
mgorny edited edge metadata.

Thanks for the suggestion, @beanz. I've updated the patch appropriately, and confirmed locally that it corrects the previous value successfully.

beanz accepted this revision.Oct 4 2016, 1:32 PM
beanz edited edge metadata.

LGTM!

This revision is now accepted and ready to land.Oct 4 2016, 1:32 PM
This revision was automatically updated to reflect the committed changes.
mgorny added a comment.Oct 4 2016, 1:40 PM

Thanks again. Crossing fingers and waiting for bots to build.