[PATCH] Adds a CMake package configuration file for libclang

[Damien Buhl]

Hi Manuel,

> On 03/06/2013 10:01 AM, Manuel Klimek wrote:
> for the future: it'll be easier to get your patches reviewed if you
> don't directly address a single dev (in this case Chandler, whose review
> queue is constantly over-subscribed anyway).

Thank you for the tip ;), on other projects I usually address any 
developer, but the code owner file brought me to address it only to 
Chandler.

On 03/06/2013 10:01 AM, Manuel Klimek wrote:
> Looking at your patch, it generally looks good if the high level  direction is right...

Do you mean that libclang shouldn't be so easily accessible for user 
code ? Or am I missing something ?




On 03/06/2013 05:26 PM, Brad King wrote:
> On 03/06/2013 10:01 AM, Manuel Klimek wrote:
>> Looking at your patch, it generally looks good if the high
>> level direction is right... I'm cc'ing Brad King, who has
>> responded to your original pull request, in the hope that he
>> can confirm that this looks good from a cmake perspective.
>
> I just looked at the patch as posted here:
>
>   http://thread.gmane.org/gmane.comp.compilers.clang.scm/68052
>
> though I have not actually tried building with it.
>
> It is a great start.  Thanks for working on it.
>
>> +++ b/tools/libclang/cmake/modules/CMakeLists.txt
>> ...
>> +set(libclang_cmake_builddir "${CMAKE_BINARY_DIR}/share/clang/cmake")
>
> FYI, the location inside the build tree is just a staging area
> for install(FILES) and the files should never be loaded from
> there.
>
>> +set(LLVM_INSTALL_PREFIX ${CMAKE_INSTALL_PREFIX})
>
> You don't seem to actually use this variable anywhere in
> the configure_file inputs.
>
>> +++ b/tools/libclang/cmake/modules/CMakeLists.txt
>> ...
>> +install(FILES
>> +    ${libclang_cmake_builddir}/libclang-config.cmake
>> +    ${libclang_cmake_builddir}/libclang-config-version.cmake
>> +    DESTINATION share/clang/cmake)
>
> In the future the file may contain architecture-specific information.
> Also CMake will not look in "clang" for a package called "libclang"
> without extra options to find_package.  Finally, there could be more
> than one version of libclang in the same prefix (someday).  I suggest
> installing the package configuration file and package version file to
>
>   lib/cmake/libclang-<version>
>
> where <version> is "${CLANG_VERSION_MAJOR}.${@CLANG_VERSION_MINOR}".
>
>> +#       target_link_libraries(youTarget clang)
>
> s/you/your/ ?
>
> Using link_directories/target_link_libraries with a library name
> will work but it will not hook up a dependency to cause the app
> to re-link when the clang library file changes.  To achieve that
> you need full target export/import support:
>
>   http://www.cmake.org/Wiki/CMake/Tutorials/Exporting_and_Importing_Targets
>
> That can be added later though.  The basic find_package(libclang)
> functionality will work without this.
>
>> +++ b/tools/libclang/cmake/modules/libclang-config.cmake.in
>> ...
>> +message(STATUS "libclang version: ${LIBCLANG_PACKAGE_VERSION}")
>
> Package configuration files should not display any messages.
> If the project loading it wants to display that kind of verbose
> information it can do so itself.
>
>> The other question is that I think that there are still
>> many (basically all) package releases that use the
>> configure-based build. We'll need a solution for those as
>> well. Is the idea that we just copy the correctly versioned
>
> Yes, you need to configure the two files
>
>   libclang-config.cmake.in
>   libclang-config-version.cmake.in
>
> with proper @...@ replacements and install them.
>
>> files into <prefix>/share/clang/cmake?
>
> Yes, though see above recommendation about install destination:
>
>    lib/cmake/libclang-<version>
>
>> If yes, how does cmake find them there?
>
> The CMake find_package command:
>
>   http://www.cmake.org/cmake/help/v2.8.10/cmake.html#command:find_package
>
> will search in <prefix>/lib/cmake/<name>* where <name> is the
> package to be found (case insensitive) and "*" globs any suffix
> on the directory name (like "-<version>").  The command documents
> how it constructs the list of possible <prefix>es.
>
> -Brad
>