Page MenuHomePhabricator

Deprecate `LLVM_LIBRARY_DIRS`
Needs ReviewPublic

Authored by Ericson2314 on Dec 31 2021, 5:22 PM.

Details

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

Event Timeline

Ericson2314 created this revision.Dec 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
mstorsjo added subscribers: beanz, mstorsjo.

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
62

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
62

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.

beanz added a comment.Jan 1 2022, 6:45 PM

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
117–120

@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?

Rebase on top of D116497

I will probably just close this, but doing the rebase in case we should get rid
of LLVM_LIBRARY_DIRS after all.

Ericson2314 edited the summary of this revision. (Show Details)Jan 7 2022, 5:58 PM