[llvm-commits] [PATCH][CMake] Removing LLVMLibDeps.cmake and exporting libraries for CMake clients.

[Michael Spencer]

On Wed, Sep 8, 2010 at 10:34 PM, Óscar Fuentes <ofv at wanadoo.es> wrote:
> [llvm-dev ml elided]
>
> Michael Spencer <bigcheesegs at gmail.com>
> writes:
>
>> While implementing the object file library I ran into some weird
>> linking issues. It turned out that you have to manually specify extra
>> library dependencies in LLVMLibDeps.cmake, along with what you already
>> specify in the CMakeLists.txt file for the component.
>
> Uh? What those extra library dependencies are and why they do require
> modifications to LLVMLibDeps.cmake? Please provide an example.

When I added LLVMObject, I had to go and add the following to LLVMLibDeps.cmake:

set(MSVC_LIB_DEPS_LLVMObject LLVMSupport LLVMSystem)

Apparently this file can be automatically generated, but only on non
Windows platforms with all targets enabled, neither of which I do.

>> The attached patches remove this requirement,
>
> Your patch switches from automatic to manually maintained library
> dependencies. On the case of LLVM, that increases the workload of the
> maintainer and it is the CMake equivalent of spaghetti code. Only a
> strong benefit would justify that.

Currently we have a mess of both. Some targets use
target_link_libraries, and others use LLVM_LINK_COMPONENTS. We already
have to manually maintain LLVMLibDeps.cmake. In fact just recently
someone forgot to commit that file when they made other changes.

My proposal is hardly spaghetti code. It's the same thing as #include.
Should compilers just look at all of the header files and pick which
ones to include automatically?

With target_link_libraries, you only have to specify which libraries
the target _directly_ needs to link to. Any other libraries will be
pulled in because CMake knows about the dependencies.

>> and add real CMake
>> package export capabilities. A CMake user wishing to use LLVM can now
>> use something similar to this:
>>
>> cmake_minimum_required(VERSION 2.8)
>>
>> find_package(LLVM REQUIRED)
>> add_definitions(${LLVM_CXXFLAGS})
>> add_executable(llvm-external-test llvm-external-test.cpp)
>> target_link_libraries(llvm-external-test ${LLVM_SYSTEM_LIBS}
>> ${LLVM_JIT_LIBS} ${LLVM_NATIVECODEGEN_LIBS})
>
> How is that better than
>
> http://www.llvm.org/docs/CMake.html#embedding

Because it is the standard CMake way of doing things. It allows
trivial selection between multiple versions. It is also simpler in the
case of finding non-installed builds without worrying about in/out of
source builds. It is not sensitive to location changes.

> ? Think that it is usual practice to have and use multiple LLVM builds
> with different configurations on the same system.

find_package easily supports this. In fact, if you have both a Release
and Debug build of LLVM installed, it will automatically choose the
correct build based on your own current build type.

>> llvm+clang compile with these changes on both Linux and MSVS.
>> Currently both patches must be applied at the same time. I can
>> refactor them into a 3 step process if needed.
>>
>> Does this break anything for anyone?
>
> If that is accepted (and for now I see no reason for accepting it)
> extensive testing on all major platforms (Linux, OS/X,
> Windows/mingw/MSVC++) is required before throwing away the current
> method, wich has its limitations, but we know them.

I agree it needs to be well tested. Currently the only platform that
it has yet to be tested on is OS/X.

- Michael Spencer