Details

Reviewers

stephenneuendorffer
phosek
compnerd
beanz

Summary

There is a FIXME to add other directories to LLVM_LIBRARY_DIRS. If we don't do that, however, it is the same as LLVM_LIBRARY_DIR. This is sort of a "plan B" revision in case we don't do that FIXME.

I still defined LLVM_LIBRARY_DIRS for sake of downstream projects, but
added a comment saying it was deprecated and should be removed. Is that
correct?

Diff Detail

Repository: rG LLVM Github Monorepo

Event Timeline

Ericson2314 created this revision.Dec 31 2021, 5:22 PM

Herald added a subscriber: mgorny. · View Herald TranscriptDec 31 2021, 5:22 PM

Ericson2314 requested review of this revision.Dec 31 2021, 5:22 PM

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 31 2021, 5:22 PM

Herald added subscribers: llvm-commits, lldb-commits. · View Herald Transcript

Ericson2314 added reviewers: stephenneuendorffer, phosek, compnerd.Dec 31 2021, 5:25 PM

Herald added a subscriber: JDevlieghere. · View Herald TranscriptDec 31 2021, 5:25 PM

Harbormaster completed remote builds in B141157: Diff 396839.Dec 31 2021, 6:04 PM

Ericson2314 mentioned this in D100810: [llvm] Use `GNUInstallDirs` to support custom installation dirs.Dec 31 2021, 10:06 PM

Ericson2314 added a child revision: D100810: [llvm] Use `GNUInstallDirs` to support custom installation dirs.Dec 31 2021, 10:06 PM

This looks reasonable to me, but I'm not sure if I know all the aspects that might be at play here, so I don't think I'm comfortable with accepting it on my own. Adding @beanz who should know lots about our cmake setup too.

llvm/cmake/modules/CMakeLists.txt
65	Curious: Why the reordering? I presume you're moving `LLVM_CONFIG_CMAKE_DIR` into alphabetical order, but then `LLVM_CONFIG_LIBRARY_DIR` seems out of place?

Ericson2314 added inline comments.Jan 1 2022, 5:48 PM

llvm/cmake/modules/CMakeLists.txt
65	Oh I just moved the prefix one to the top, since it is unlike the others, and put library on the bottom, moving it away from the other "weird" one (the include dirs). The ordering of cmake then binaries is simply because that is the way it was before --- the diff algorithm just happened to hide that in choosing to make `LLVM_CONFIG_BINARY_DIR` the "stable" one line.

LLVM_LIBRARY_DIR and LLVM_LIBRARY_DIRS aren’t intended to serve the same purpose. The S in DIRS is intentional to convey that it is a list of directories, not necessarily a single value, and it would definitely break many uses of LLVM_LIBRARY_DIR if it was a list. There are several (not correctly handled) build configurations where LLVM_SHLIB_OUTPUT_INTDIR and the shared library install directory are not the same as the static archive install directory. In those cases we _should_ be setting LLVM_LIBRARY_DIRS to be an array containing both LLVM_LIBRARY_DIR and LLVM_TOOLS_BINARY_DIR.

Ericson2314 added inline comments.Jan 1 2022, 6:49 PM

llvm/cmake/modules/LLVMConfig.cmake.in
121–124	@beanz it would definitely break many uses of `LLVM_LIBRARY_DIR` if it was a list. The code right here is dangerously close to doing that. So I think some change is waranted. Perhaps I can add back `set(LLVM_LIBRARY_DIRS "@LLVM_CONFIG_LIBRARY_DIRS@")` above and add a TODO next to the definitions of `LLVM_CONFIG_LIBRARY_DIRS` with the cases you mentioned?