Page MenuHomePhabricator

Search for llvm-config in LLDB_PATH_TO_LLVM_BUILD first
AbandonedPublic

Authored by tfiala on Oct 20 2016, 9:05 AM.

Details

Reviewers
vivkong
beanz
Summary

If LLDB_PATH_TO_LLVM_BUILD is present, search for llvm-config there first before looking in CMake or system paths.

(The patch is a modified fix from @tfiala)

Diff Detail

Repository
rL LLVM

Event Timeline

vivkong updated this revision to Diff 75182.Oct 20 2016, 9:05 AM
vivkong retitled this revision from to Search for llvm-config in LLDB_PATH_TO_LLVM_BUILD first.
vivkong updated this object.
vivkong added reviewers: labath, beanz, tfiala.
vivkong set the repository for this revision to rL LLVM.
vivkong added subscribers: lldb-commits, tfiala.
tfiala accepted this revision.Oct 20 2016, 9:19 AM
tfiala edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Oct 20 2016, 9:19 AM

(It would be good to wait for feedback from the others, though).

tfiala added inline comments.Oct 20 2016, 9:22 AM
cmake/modules/LLDBStandalone.cmake
20

One question here would be what happens if FIND_PATHS stays "". Does find_program deal with an empty HINTS? If not, the HINTS clause itself might need to be conditionalized.

beanz requested changes to this revision.Oct 20 2016, 9:26 AM
beanz edited edge metadata.

LLDB_PATH_TO_LLVM_BUILD is a swift-ism that doesn't match LLVM's CMake conventions. In LLVM we don't pass in the path to build directories, instead we pass in LLVM_CONFIG to standalone builds, and we derive the build directory from the output of llvm-config.

Passing in LLVM_CONFIG is more flexible, robust, and consistent with how other LLVM subprojects work, and I see no reason to change that convention.

This revision now requires changes to proceed.Oct 20 2016, 9:26 AM

I agree. Don't introduce redundant variables when the same result can be achieved using existing means.

(note that find_program makes LLVM_CONFIG a cache variable implicity, so you don't need to have any explicit support for altering it)

labath edited edge metadata.Oct 20 2016, 10:11 AM
labath added subscribers: krytarowski, Eugene.Zelenko.

+cc: lldb standalone users

I'll defer to @beanz on this.

I think it's possible to specify -DLLVM_CONFIG:PATH option, if that does the job.

tfiala commandeered this revision.Oct 21 2016, 11:50 AM
tfiala edited reviewers, added: vivkong; removed: tfiala.

@beanz and I discussed. This isn't needed here at all. The issue is entirely in the current manifestation of the GitHub side of swift-lldb.

Closing this out, we'll resolve in GitHub.

labath resigned from this revision.Oct 27 2016, 11:36 AM
labath removed a reviewer: labath.
vivkong requested changes to this revision.Oct 27 2016, 1:00 PM
vivkong edited edge metadata.

This is not required anymore and it can be closed. Thanks.

tfiala abandoned this revision.Oct 28 2016, 2:00 PM

I'm at a loss for the workflow to close this. I originally commandeered it to do an abort on it, but that didn't work. I'm going to abandon it, and see if I can kill it then.