This is an archive of the discontinued LLVM Phabricator instance.

[LIT] Fix testing out-of-tree Clang builds
ClosedPublic

Authored by EricWF on Nov 18 2017, 2:24 PM.

Details

Summary

Currently, LIT configures the LLVM binary path before the Clang binary path. However this breaks testing out-of-tree Clang builds (where the LLVM binary path includes a copy of Clang).

This patch reverses the order of the paths when looking for Clang, putting the Clang binary directory first.

Diff Detail

Event Timeline

EricWF created this revision.Nov 18 2017, 2:24 PM
mgorny added a subscriber: mgorny.Nov 18 2017, 2:48 PM

To be honest, as I said before, I'm entirely confused by the logic there, with all the prepending, appending and shuffling around. But if you believe it gives the correct result, I'm all for it.

However, if that wouldn't be too much of a hassle, would you mind also renaming the variables to use a bit more meaningful names?

My alternative idea would be to put both paths on the initial list, and filter out None instances from the list afterwards, e.g. something like:

paths = [getattr(self.config, 'clang_tools_dir', None), self.config.llvm_tools_dir]
paths = [x for x in paths if x is not None]

Or even more abstract (if we assume we don't have to check for presence of llvm_tools_dir):

path_vars = ['clang_tools_dir', 'llvm_tools_dir']
paths = [getattr(self.config, k) for k in path_vars if hasattr(self.config, k)]

(untested)

EricWF updated this revision to Diff 123485.Nov 18 2017, 3:11 PM
EricWF accepted this revision.Nov 18 2017, 3:59 PM

Accepting for post-commit review. I don't want to be carrying this patch locally.

This revision is now accepted and ready to land.Nov 18 2017, 3:59 PM
EricWF closed this revision.Nov 18 2017, 4:00 PM