[PATCH] Adds a CMake package configuration file for libclang

[Manuel Klimek]

On Wed, Mar 6, 2013 at 11:33 AM, Damien Buhl <damien.buhl at lecbna.org> wrote:

> 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 ?


No, I meant: if that's what cmake integration should look like; that's why
I pulled in Brad, who was nice enough to put in a lot of feedback, so I can
basically say: please address Brad's comments :)

Cheers,
/Manuel


>
>
>
>
>
> 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<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<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<http://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<http://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<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
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130306/ead08fd8/attachment.html>