This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Clean up CMake binary dir handling
Needs ReviewPublic

Authored by Ericson2314 on Aug 24 2022, 2:21 PM.

Details

Reviewers
sebastian-ne
beanz
phosek
ldionne
bollu
sscalpone
awarzynski
rafauler
Amir
maksfb
NoQ
jdoerfert
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Summary

There are a few goals here:

  • Match what D132316 for LLVM_BINARY_DIR, for CMAKE_BINARY_DIR.
  • Decrease the usages of LLVM_LIBDIR_SUFFIX, preparing us for D130586.
  • Deduplicate code repeated across projects

Do this in this way:

  • Shuffle around CMAKE_MODULE_PATH appending to allow include-ing our own modules earlier.
  • Create new private/internal CMake modules and use them:
    • LLVMGNUDirs

      Like upstream CMake GNUInstallDirs, but sets variations of LLVMPROJ_BINARY_DIR. These are not CACHE PATHS because they are not intended to be user-modifyable.

      LLVMPROJ_BINARY_LIBDIR is based on the base name of CMAKE_INSTALL_LIBDIR rather than LLVM_LIBDIR_SUFFIX. This cannot just end with "lib", because the "install" and "binary" base names need to line up for various relative paths to work. It doesn't just use LLVM_LIBDIR_SUFFIX because we are trying to phase that out.

      Also has a compat shim that defaults CMAKE_INSTALL_LIBDIR based on LLVM_LIBDIR_SUFFIX.
    • LLVMSetIntDirs

      Just here to deduplicate the defining of LLVM_LIBRARY_OUTPUT_INTDIR and friends between projects.
  • Cleanup custom install path initialization code in the runtimes

Diff Detail

Event Timeline

Ericson2314 created this revision.Aug 24 2022, 2:21 PM
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
Ericson2314 requested review of this revision.Aug 24 2022, 2:21 PM
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
arichardson added inline comments.
cmake/Modules/GNUBinaryDirs.cmake
6 ↗(On Diff #455373)

I find this name a bit confusing, maybe something like CMAKE_BUILD_BINDIR (and the same for _LIBDIR/_INCLUDEDIR would be less surprising? That way it's clear that we are referring to binaries/libraries/includes in the build output (and it mirrors CMAKE_INSTALL_INCLUDEDIR, etc.)

I’m not sure if it’s the case for all places (as CMAKE_CFG_INTDIR is not defined at install time), but I think the CMAKE_BINARY_LIBDIR introduced here serves the same purpose as the already existing LLVM_LIBRARY_DIR.
Same for CMAKE_BINARY_INCLUDEDIR, which is LLVM_INCLUDE_DIR and CMAKE_BINARY_BINDIR which is LLVM_TOOLS_BINARY_DIR.

E.g. llvm/CMakeLists.txt uses

set( CMAKE_LIBRARY_OUTPUT_DIRECTORY ${LLVM_LIBRARY_DIR} )
set( CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${LLVM_LIBRARY_DIR} )

while clang uses

set( CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX} )
set( CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX} )

and this patch replaces the clang code with

set( CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_LIBDIR} )
set( CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_LIBDIR} )
compnerd added inline comments.
cmake/Modules/GNUBinaryDirs.cmake
3 ↗(On Diff #455373)

Should this perhaps be moved further down near the usage?

6 ↗(On Diff #455373)

I think you mean ${CMAKE_BINARY_DIR}/bin as this is in a block where CMAKE_BINARY_BINDIR is undefined.

10 ↗(On Diff #455373)

Is the default spelling for include not include not inc?

I think I like the spirit of this change, which seems to be to move us closer to CMAKE_foo variables and further from LLVM_foo variables for equivalent functionality. I have a comment, but this essentially LGTM. The libc++ CI failures (in particular the bootstrapping build failure) seems to be real, though, so it should be understood before landing the patch.

libcxx/CMakeLists.txt
411

Instead, I'd do this:

if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
  set(_include_target_dir "${CMAKE_INSTALL_INCLUDEDIR}/${LLVM_DEFAULT_TARGET_TRIPLE}/c++/v1")
  set(_install_library_dir "lib${LLVM_LIBDIR_SUFFIX}/${LLVM_DEFAULT_TARGET_TRIPLE}")
else()
  set(_include_target_dir "${LIBCXX_INSTALL_INCLUDE_DIR}")
  set(_install_library_dir "lib${LLVM_LIBDIR_SUFFIX}")
endif()

set(LIBCXX_INSTALL_INCLUDE_TARGET_DIR "${_include_target_dir}" CACHE PATH
    "Path where target-specific libc++ headers should be installed.")
set(LIBCXX_INSTALL_LIBRARY_DIR "${_install_library_dir}" CACHE PATH
    "Path where built libc++ libraries should be installed.")

IMO that's easier on the eye.

Ericson2314 marked 4 inline comments as done.

Rebase, avoid sed

cmake/Modules/GNUBinaryDirs.cmake
3 ↗(On Diff #455373)

Sure

6 ↗(On Diff #455373)

@compnerd oops thanks.

@arichardson CMAKE_BINARY_* is the already existing naming convention of built-in variables. I agree it is not the best, but I don't want to buck the standard.

10 ↗(On Diff #455373)

Oops yes it is.

Ericson2314 edited the summary of this revision. (Show Details)Sep 13 2022, 10:15 PM

Fix typo in compiler-rt

Fix misspelled variable CLANG_CURRENT_SOURCE_DIR -> CLANG_SOURCE_DIR

ldionne added inline comments.Sep 14 2022, 2:03 PM
libcxx/CMakeLists.txt
411

Gentle ping on this.

Ericson2314 added inline comments.Sep 14 2022, 2:16 PM
libcxx/CMakeLists.txt
411

Ah right, I have this "throw away temporary style as soon as possible" on all the runtimes right now. Would you prefer I do instead do "define all the defaults, define all the cache vars" for all of them?

I am a little partial to the current style because if it is closer to what I would do if cmake has proper expressions. (something like:

set(LIBCXX_INSTALL_INCLUDE_TARGET_DIR
  "${
    if(...)
      stuff
    else()
     stuff
    endif()
  }" CACHE PATH
    "Path where target-specific libc++ headers should be

). But if that if you are sure your want it consistently the other way I can switch it.

A bigger issue might be when the variables depend on each other, e.g. when I get the library install dir base name for the library binary dir.

Is it a good idea to define variables starting with CMAKE? That might be confusing for later maintenance by other developers because chances are that they will think those variables are CMake provided variables, try to look up into CMake document to see what they are, and then find out they are not at all.

@tianshilei1992 that is a fair point. I would be open to calling them something else.

I just don't want to call them LLVM_* because that would be confusing since there is (a) LLVM in particular (b) the monorepo / project as a whole, and this variable are about *neither* of them, being scoped to the current build.

I mainly named this way to match the`GNUInstallDirs` naming scheme, where CMAKE_INSTALL_PREFIX goes with` CMAKE_INSTALL_*DIR`; here CMAKE_BINARY_DIR goes with CMAKE_BINARY_*DIR. I admit relative vs absolute paths, in addition to _PREFIX vs _DIR, do not make the analogy exact though.

I also was hoping something like this could be upstreamed into CMake itself, but using different names now doesn't prevent upstream from adopting CMAKE_* were this to become official.

I agree with @tianshilei1992, I think we should avoid introducing new CMAKE_ variables to avoid confusion. The same applies to module names, for example I don't think we should be introducing GNUBinaryDirs which can be easily confused for GNUInstallDirs.

I would personally go with the LLVM_ prefix, I agree that this can lead to some confusion given that LLVM is both the name of the top-level project as well as one of the subprojects, but I think that ship has already sailed we already have proliferation of LLVM_ named variables throughout the codebase.

Regarding GNUBinaryDirs, it seems like we always use the following pattern:

# Must go before the first `include(GNUInstallDirs)`.
include(LLVMLibdirSuffix)

# Must go below project(..)
include(GNUInstallDirs)
include(GNUBinaryDirs)

That introduces a lot of duplication and is potentially error-prone since we rely on particular include order. Could we instead introduce a single LLVMInstallDirs module which would contain the content of LLVMLibdirSuffix and GNUBinaryDirs and would also include GNUInstallDirs, so LLVMInstallDirs is all you would need to include in individual subprojects?

Dedup code, rename variables

Ericson2314 edited the summary of this revision. (Show Details)Nov 7 2022, 7:57 AM

I have done the deduping @phosek requested, and changed the variable names from CMAKE_* to LLVMPROJ_* which hopefully satisfies everyone's criteria. Happy with other non LLVM_ options too.

@Ericson2314 @phosek What's the state of this patch?