This is an archive of the discontinued LLVM Phabricator instance.

Replace LLVM_LIBDIR_SUFFIX by CMAKE_INSTALL_LIBDIR
Needs ReviewPublic

Authored by serge-sans-paille on Nov 3 2022, 7:21 AM.

Details

Reviewers
MaskRay
tstellar
mgorny
sylvestre.ledru
bollu
sscalpone
rafauler
Amir
maksfb
NoQ
jdoerfert
Ericson2314
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Summary

There are a few advantages of using CMAKE_INSTALL_LIBDIR in place of
LLVM_LIBDIR_SUFFIX:

  1. it's a standard flag, no need to support an extra cmake configuration
  2. it has sane defaults, and picks lib64 instead of lib on some systems (including Fedora)

Diff Detail

Event Timeline

Herald added a reviewer: rafauler. · View Herald Transcript
Herald added a reviewer: Amir. · View Herald Transcript
Herald added a reviewer: maksfb. · View Herald Transcript
Herald added a reviewer: NoQ. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
serge-sans-paille requested review of this revision.Nov 3 2022, 7:21 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
serge-sans-paille edited the summary of this revision. (Show Details)Nov 3 2022, 7:22 AM

Maybe add it to the release notes?

mgorny added inline comments.Nov 3 2022, 12:05 PM
bolt/lib/RuntimeLibs/RuntimeLibrary.cpp
32

Well, one thing I immediately notice is that technically CMAKE_INSTALL_LIBDIR can be an absolute path, while in many locations you are assuming it will always be relative. Not saying that's a blocker but I think you should add checks for that into CMakeLists.txt.

clang/tools/libclang/CMakeLists.txt
234

You seem to be changing indentation here, and below.

On unrelated note, hardcoding site-packages path sounds like a bad idea but that's a problem for another patch.

Can you verify if projects enabled by LLVM_ENABLE_RUNTIMES (not LLVM_ENABLE_PROJECTS) will still be installed properly, aka in LLVM's lib directory?

Ericson2314 added inline comments.Nov 4 2022, 11:10 AM
bolt/lib/RuntimeLibs/RuntimeLibrary.cpp
32

Yes I was working on this, and I intend to use CMAKE_INSTALL_LIBDIR with an absolute path.

OK if this isn't supported initially but we should ovoid regressing where possible.

clang/include/clang/Config/config.h.cmake
39

I had called it _BASENAME in anticipation that it would be the basename of CMAKE_INSTALL_LIBDIR eventually when CMAKE_INSTALL_LIBDIR is not a relative path.

bolt/lib/RuntimeLibs/RuntimeLibrary.cpp
32

Turns out LLVM_LIBDIR_SUFFIX is used in several situations: (a) for the library install dir or (b) for the location where build artifact as stored in CMAKE_BINARY_DIR. (a) could accept a path but not (b), unless we derive it from (a) but I can see a lot of complication there for the testing step. Would that be ok to choke on CMAKE_INSTALL_LIBDIR being a path and not a directory component?

compnerd added inline comments.
bolt/lib/RuntimeLibs/RuntimeLibrary.cpp
32

I think that is absolutely reasonable. There is the CMAKE_INSTALL_FULL_LIBDIR which should be the relatively absolute path (it is relative to DESTDIR). The CMAKE_INSTALL_LIBDIR should be the relative component which is added to CMAKE_INSTALL_PREFIX.

Ericson2314 added inline comments.Dec 9 2022, 8:02 AM
bolt/lib/RuntimeLibs/RuntimeLibrary.cpp
32

Please do not do this. In NixOS we uses absolute paths for these which are *not* within CMAKE_INSTALL_PREFIX and I would very much like that to continue to work, and have put a lot of effort into it.

(I am sorry I have been a bit AWAL on LLVM things in general but hopefully will have more time as we head into the new year.)

compnerd added inline comments.Dec 9 2022, 11:21 AM
bolt/lib/RuntimeLibs/RuntimeLibrary.cpp
32

Why can NixOS not use CMAKE_INSTALL_FULL_LIBDIR? That is computed to ${CMAKE_INSTALL_PREFIX}${CMAKE_INSTALL_LIBDIR} only if it is not already defined to an absolute path.

barannikov88 added inline comments.
bolt/include/bolt/RuntimeLibs/RuntimeLibraryVariables.inc.in
17

The prefix must remain LLVM_*
In other components accordingly

barannikov88 added inline comments.Dec 20 2022, 7:14 AM
bolt/include/bolt/RuntimeLibs/RuntimeLibraryVariables.inc.in
17

The prefix must remain LLVM_*
In other components accordingly

To clarify, I mean the C++ macro name and not CMake variable name

For libc++ we like to clean up the review queue for the GitHub PR transition. Is there still interest on working on this patch? If so would it be possible to finish it before the Phabricator is changed to read only mode?