[PATCH] Document new LLVM CMake interface and emit warning when using llvm_map_components_to_libraries

[Dan Liew]

>> +  project(SimpleProject)
>
> Precede this line with::
>
>   cmake_minimum_required(VERSION 2.8.8)
>
> since that is the minimum required by LLVM.  All CMake projects should
> start with such a line so the example should show it.

Done.

>> +  set(LLVM_DIR "" CACHE FILEPATH "Path to directory containing LLVMConfig.cmake")
>> +  find_package(LLVM REQUIRED CONFIG)
>
> The find_package command creates the LLVM_DIR cache entry automatically
> so the set() line above is not needed.

Okay I've removed this.

>> +  # Link against LLVM libraries
>> +  target_link_libraries(simple-tool LLVMSupport LLVMCore LLVMIRReader)
>
> IIUC applications should still use llvm_map_components_to_libnames
> which maps LLVM components to a list of libraries but does not expand
> dependencies.  For example:
>
>  llvm_map_components_to_libnames(llvm_libs native option bitreader)
>  target_link_libraries(simple-tool ${llvm_libs})

I've left this issue for now. What is the motivation for not directly
linking against the imported targets as I have done in my toy example
project [1]? Is it ever the case that a component does not directly
map to a single library?

I wondered if it might be due to building everything in LLVM into one
large shared library. I just tried that by enabling BUILD_SHARED_LIBS
and everything still seems to be built as separate .so libraries so
using the imported targets directly still works. So that isn't the
issue.

Is this something to do with the LLVMBuild.txt stuff? I've completely
ignored these up until now because I couldn't understand what they
were doing and why it was necessary.

>> +cmake manual for details). The ``LLVM_DIR`` variable can be set to hint to
>> +``find_package(..)`` where to look for the ``LLVMConfig.cmake``.
>
> LLVM_DIR is not just a hint.  It is the place that find_package
> checks for an existing result and/or stores its new result.
> Perhaps::
>
>   It creates a ``LLVM_DIR`` cache entry to save the location where
>   ``LLVMConfig.cmake`` is found or allow the user to specify the
>   location.

Okay I've taken that but with s/location/directory/

>> +This file is available in two different locations.
>
> I think the install-tree location should be listed first since that
> is what applications will typically use, especially when LLVM has
> been installed by the package manager.  Also, this might be a good
> location to recommend pointing CMAKE_PREFIX_PATH at the top of the
> installation prefix (if it is not a system prefix) or build directory
> when building an application.  Then find_package will be able to
> locate LLVMConfig.cmake and set LLVM_DIR without the user having to
> poke around to find share/llvm/cmake.

Okay I've swapped the order. I decided not to mention
CMAKE_PREFIX_PATH. I've not used it before and based on reading its
documentation (man cmake-variables) it globally effects what find_XXX
directives do. This does not seem like a good idea. Imagine if user's
application uses ``find_package(Boost)`` but also uses
``find_package(LLVM REQUIRED CONFIG)``. If the user hasn't installed
LLVM and wants to build against LLVM in the build directory but also
the installed Boost libraries then setting CMAKE_PREFIX_PATH won't
work.

- If CMAKE_PREFIX_PATH is left unset then Boost will be found but LLVM
will not because its not installed.
- If CMAKE_PREFIX_PATH is set to the root of the LLVM build tree then
LLVM will be found but now ``find_package(Boost)`` can't find Boost.

Although looking at documentation for find_package() the search order
is quite complicated so I'm probably wrong. It is also not obvious
where using <package>_DIR is in that order from the documentation.

>> +The ``LLVMConfig.cmake`` file sets various useful variables. Notable variables
>> +include
>
> Good.  I think the list of variables would be better formatted as
> a definition list, and without the ":<type>" suffixes since they
> are not cache entries.  For example::

Done.
>
>   ``LLVM_CMAKE_DIR``
>     The path to the directory containing LLVM CMake modules.
>
>   ``LLVM_DEFINITIONS``
>     A list of preprocessor defines that should be used with
>     ``add_definitions()`` when building against LLVM.
>
>> +  set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${LLVM_CMAKE_DIR}")
>
> This can be::
>
>   list(APPEND CMAKE_MODULE_PATH ${LLVM_CMAKE_DIR})

Done.

> In the second patch:
>
>> +  message(AUTHOR_WARNING "Using llvm_map_components_to_libraries() is deprecated. Use LLVM libraries as imported targets instead.")
>
> This message may need to be updated based on the decision about the
> above comment suggesting use of llvm_map_components_to_libnames.

Yes. I've deliberately not rewritten this patch until we have a
consensus on how to link against LLVM libraries.

[1] https://github.com/delcypher/srg-llvm-pass-tutorial/blob/master/toolDemo/CMakeLists.txt

Thanks,
Dan.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Document-the-new-LLVM-CMake-interface-for-building-a.patch
Type: text/x-patch
Size: 10217 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140723/8d1588a8/attachment.bin>