Page MenuHomePhabricator

[lldb] [cmake] Use install directories for LLVM_* variables
ClosedPublic

Authored by mgorny on Feb 9 2019, 4:52 AM.

Details

Summary

Restore the previous behavior of using install directories for
LLVM_MAIN_INCLUDE_DIR, LLVM_LIBRARY_DIR and LLVM_BINARY_DIR. The update
from llvm-config to CMake has changed the values of those values to use
LLVM_BUILD_* which is plain wrong and breaks stand-alone builds.
Instead, use the CMake counterparts of the values returned
by llvm-config.

Diff Detail

Repository
rL LLVM

Event Timeline

Indeed, these variables do not exist when running against a LLVM/Clang install-tree and this should be fixed. Did you give this a try running against a LLVM/Clang build-tree? Dumping the values here locally, gives me:

LLVM_BUILD_MAIN_INCLUDE_DIR /path/to/llvm/include
LLVM_BUILD_LIBRARY_DIR      /path/to/llvm-build/./lib
LLVM_BUILD_BINARY_DIR       /path/to/llvm-build

LLVM_INCLUDE_DIR            /path/to/llvm/include;/path/to/llvm-build/include
LLVM_LIBRARY_DIR            /path/to/llvm-build/./lib
LLVM_BINARY_DIR             /path/to/llvm-build

Hope the two paths that the build-tree LLVMConfig sets for LLVM_INCLUDE_DIR is not causing issues.

Indeed it is a problem. However, it seems to be a bug that LLVM_INCLUDE_DIR contains two directories. Apparently in regular builds it contains the directory with built includes only, with LLVM_MAIN_INCLUDE_DIR being the other directory. In other words, it's a mess ;-/.

mgorny updated this revision to Diff 186427.Feb 12 2019, 2:23 AM

Updated to use exported LLVM_MAIN_INCLUDE_DIR, as introduced by D58109. This works both for build-tree and install-tree, and avoids having to introduce any conditionals.

Indeed it is a problem. However, it seems to be a bug that LLVM_INCLUDE_DIR contains two directories.

Yes, especially since we also have LLVM_INCLUDE_DIRS. But that's not the end.. in llvm/cmake/modules/CMakeLists.txt we set:

set(LLVM_CONFIG_INCLUDE_DIRS
  "${LLVM_MAIN_INCLUDE_DIR}"
  "${LLVM_INCLUDE_DIR}"
  )

The LLVMConfig.cmake.in template stores it as:

set(LLVM_INCLUDE_DIRS "@LLVM_CONFIG_INCLUDE_DIRS@")
set(LLVM_LIBRARY_DIRS "@LLVM_CONFIG_LIBRARY_DIRS@")

# These variables are duplicated, but they must match the LLVM variables of the
# same name. The variables ending in "S" could some day become lists, and are
# preserved for convention and compatibility.
set(LLVM_INCLUDE_DIR "@LLVM_CONFIG_INCLUDE_DIRS@")
set(LLVM_LIBRARY_DIR "@LLVM_CONFIG_LIBRARY_DIRS@")

The install step fixes that and generates:

set(LLVM_INCLUDE_DIR "${LLVM_INSTALL_PREFIX}/include")
set(LLVM_LIBRARY_DIR "${LLVM_INSTALL_PREFIX}/lib${LLVM_LIBDIR_SUFFIX}")

Result:

  • LLVM itself and clients of the install-tree see LLVM_INCLUDE_DIR=/path/to/llvm-build/include
  • Clients of the build-tree see LLVM_INCLUDE_DIR=/path/to/llvm/include;/path/to/llvm-build/include

This should really be fixed. Looks like the confusion started with https://reviews.llvm.org/rL280013 and manifested with https://reviews.llvm.org/rL280380.

In other words, it's a mess ;-/.

Yes, definitely

I just caught up to this thread and wow, yeah things are far more broken than I realized. Apologies for opening this can of worms.

That being said, thanks for taking the time to look at it and clean it up.

xiaobai accepted this revision.Feb 12 2019, 3:47 PM

After looking at the dependent patch, this looks good to me.

This revision is now accepted and ready to land.Feb 12 2019, 3:47 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2019, 12:34 AM