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

[Ericson2314]

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.

[Ericson2314]

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

[beanz]

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]

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]

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.

[Ericson2314]

Split out Clang changes into separate commit

[Ericson2314]

clang/CMakeLists.txt

26 ↗

(On Diff #396913)

Sure. Done in D116492.

[Ericson2314]

clang/CMakeLists.txt

26 ↗

(On Diff #396913)

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

[Ericson2314]

lld/CMakeLists.txt

17

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

[beanz]

LGTM

[nikic]

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]

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).

[Ericson2314]

In D116492#3290087, @nikic wrote:

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?

[Ericson2314]

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