Page MenuHomePhabricator

[lld] Deprecate using llvm-config to detect llvm installation
ClosedPublic

Authored by Ericson2314 on Jan 1 2022, 10:51 PM.

Details

Summary

This is continuing in the path of D51714, which did this for Clang.

I have rearranged the source code Clang so one can diff the top-level
CMakeLists.txt of Clang and LLD, ensuring we use the same strategy for
both.

Besides diffing the two files, git diff --color-moved on LLD also helps review.

Diff Detail

Event Timeline

Ericson2314 created this revision.Jan 1 2022, 10:51 PM
Ericson2314 requested review of this revision.Jan 1 2022, 10:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 1 2022, 10:51 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Ericson2314 added a reviewer: beanz.EditedJan 1 2022, 10:52 PM
Ericson2314 edited the summary of this revision. (Show Details)

Note this is a first draft, I need to go test. CI is happy with it.

beanz added inline comments.Jan 3 2022, 10:00 AM
clang/CMakeLists.txt
26 ↗(On Diff #396913)

I assume these are re-arranged to match what you're doing in lld. Is that correct?

Generally it is preferred to do this kind of non-functional restructuring in a separate commit from the commit with a functional change to make it easier to review the functional changes.

This patch as-is seems to have cleanup to clang, and a functional change for lld. Those should likely be two separate commits.

Ericson2314 added inline comments.Jan 3 2022, 10:07 AM
clang/CMakeLists.txt
26 ↗(On Diff #396913)

Happy to split up.

The clang changes I wouldn't even say make it "better" per say, I am just reording things to put the stuff LLD also goes first for sake diffing the two. I thought that might look lke a noisy and pointless patch on its own, but I am happy to split it up if you still think it's a good idea.

beanz added inline comments.Jan 3 2022, 10:08 AM
clang/CMakeLists.txt
26 ↗(On Diff #396913)

More smaller patches is generally the LLVM philosophy, so I do think it makes sense to split it up into an NFC patch for clang, and a separate change for lld.

Split out Clang changes into separate commit

Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2022, 10:22 AM
Ericson2314 marked 2 inline comments as done.Jan 3 2022, 10:23 AM
Ericson2314 added inline comments.
clang/CMakeLists.txt
26 ↗(On Diff #396913)

Sure. Done in D116492.

Ericson2314 marked an inline comment as done.
Ericson2314 added inline comments.
clang/CMakeLists.txt
26 ↗(On Diff #396913)

Oops, that's this one! I meant D116548.

Ericson2314 added inline comments.Jan 3 2022, 10:40 AM
lld/CMakeLists.txt
17

--obj-root is the same thing as --prefix, so this is in fact a mere reordering in behavior.

beanz accepted this revision.Jan 7 2022, 12:29 PM

LGTM

This revision is now accepted and ready to land.Jan 7 2022, 12:29 PM
nikic added a subscriber: nikic.Feb 2 2022, 3:47 AM
nikic added inline comments.
lld/CMakeLists.txt
14

Is it intentional that lld now requires you to set LLVM_CONFIG instead of LLVM_CONFIG_PATH? Based on the deprecation, I would have assumed that the previous name would be retained.

nikic added a comment.Feb 2 2022, 5:22 AM

Would it be possible to share a complete new-style cmake invocation that is supposed to work? If I only point LLVM_DIR to the cmake directory, then LLVM_MAIN_SRC_DIR ends up being empty (it is set to MAIN_SRC_DIR, which, as far as I can tell, is only initialized in the LLVM_CONFIG code path).

Would it be possible to share a complete new-style cmake invocation that is supposed to work?

Our standalone LLD package in Nixpkgs works, but I see now we have been passing -DLLVM_MAIN_SRC_DIR all along.

If I only point LLVM_DIR to the cmake directory, then LLVM_MAIN_SRC_DIR ends up being empty (it is set to MAIN_SRC_DIR, which, as far as I can tell, is only initialized in the LLVM_CONFIG code path).

https://reviews.llvm.org/D51714 did just that, and it was never changed in the years since, so I figured I was missing something and it somehow worked.

lld/CMakeLists.txt
14

I was just following what Clang does, but I see now in https://reviews.llvm.org/D51714 that was preserving the existing name. I suppose this should be changed back, then?

@nikic I have tried to address your concerns in https://reviews.llvm.org/D118792