This is an archive of the discontinued LLVM Phabricator instance.

clang/cmake: Drop use of llvm-config for LLVM install discovery
ClosedPublic

Authored by Ericson2314 on Jun 28 2022, 6:17 PM.

Details

Summary

This has been deprecated for a while, since D51714 in 2018.

Remove it in favor of using CMake's find_package() function.

Diff Detail

Event Timeline

tstellar created this revision.Jun 28 2022, 6:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2022, 6:17 PM
Herald added a subscriber: mgorny. · View Herald Transcript
tstellar requested review of this revision.Jun 28 2022, 6:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2022, 6:17 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tstellar added inline comments.Aug 3 2022, 2:20 PM
clang/CMakeLists.txt
21–23

This doesn't look right (even though it passes my testing). I need to update this part of the patch.

tstellar updated this revision to Diff 449857.Aug 3 2022, 8:24 PM

Remove more of the existing code.

tstellar added inline comments.Aug 3 2022, 8:26 PM
clang/CMakeLists.txt
21–23

I fixed this by removing of of the existing code that copied values from LLVMConfig.cmake.

phosek accepted this revision.Aug 3 2022, 11:51 PM

This is similar to @Ericson2314's change D130735 which also removes llvm-config from LLD. I'm fine with either of the two changes. It might be better to land the Clang and LLD changes separately so they can be reverted separately if needed.

This revision is now accepted and ready to land.Aug 3 2022, 11:51 PM

@Ericson2314 Your version of these patches removed a few more things than mine (which seems better to me), do you want to just split your patch and commit the parts separately?

Ericson2314 edited the summary of this revision. (Show Details)Aug 6 2022, 6:34 AM
Ericson2314 commandeered this revision.Aug 6 2022, 6:39 AM
Ericson2314 edited reviewers, added: tstellar; removed: Ericson2314.

Add my changes but split to just Clang as @tstellar requested

mgorny accepted this revision.Aug 6 2022, 9:56 AM

This one LGTM to me as well, and doesn't seem to break Gentoo either ;-).