[PATCH] Adds a CMake package configuration file for libclang

[Manuel Klimek]

+echristo

On Wed, Mar 6, 2013 at 4:40 PM, Damien Buhl <damien.buhl at lecbna.org> wrote:

> On 03/07/2013 01:17 AM, Manuel Klimek wrote:
>
>> 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 :)
>>
> In fact in my previous mail I was adressing both comments, but I sent the
> mail accidentally. :|
>
> Thank you for taking time for this, it's nice Manuel that you cc'ed Brad
> King.
>
> Brad it's really nice that you replied so fast and that you help me in a
> great and detailed way like this.
>
>
> On 03/06/2013 05:26 PM, Brad King wrote:
> >> +++ 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.
>
> I'm aware of this. I tried to make something equal to
> llvm/cmake/modules/CMakeLists.**txt which also declares a variable in the
> same way to provide package configuration file for llvm libraries.
>
>
> >
> >> +set(LLVM_INSTALL_PREFIX ${CMAKE_INSTALL_PREFIX})
> >
> > You don't seem to actually use this variable anywhere in
> > the configure_file inputs.
> >
>
> Yes this is silly indeed to have it defined. :) It was to have the same as
> llvm/cmake/modules/CMakeLists.**txt, I removed it in the new patch.
>
>
> > Also CMake will not look in "clang" for a package called "libclang"
> > without extra options to find_package.
>
> Hmm... Indeed I thought I tested it without, but when I repeat my test now
> I see that I should always have had a define for libclang_DIR because it
> ran through. Thanks for pointing this out, I fixed it.
>
>
> > 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)
> >
>
> It's nice to think to the future indeed, I changed this, but took
> LIBCLANG_LIBRARY_VERSION instead of ${CLANG_VERSION_MAJOR}.${**CLANG_VERSION_MINOR},
> since it's also used in the package version file. :)
>
>
> > 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.
>
> This would be nice and especially for people patching libclang, testing
> their custom build of libclang.
>
> It should be pretty easy to add this, by adding an EXPORT param to the
> install call in the add_clang_library macro (see root CMakeLists.txt), and
> we could add an install(EXPORT...) call there, to put a full import/export
> support per library. I'll try to make a new patch for this once this first
> patch get merged, because this requires same modifications for the install
> of llvm/ libraries too.
>
>
> >
> >> +++ 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.
> >
>
> Ihis makes sense, I didn't know, because often FindPackages file performs
> status outputs. Thanks. :)
>
>
> >> 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>
> >
>
> @Manuel: I would be happy to add this to the patch, is there already any
> starting point ? Because the llvm libs also have package configuration file
> for cmake, but I didn't found any place (i.e. I fgrep'ed in llvm/autoconf/
> without any match) where this was already configured through the autoreconf
> files.
>

I have no idea about the configure build, cc'ed echristo for ideas here...

Cheers,
/Manuel


>
> Please find a new patch with the different modifications discussed as
> attachment.
>
> Cheers and thank you for your help,
>
> --
> Damien Buhl
> alias daminetreg
>
> P.S. The patch is also available on github here :
> https://github.com/daminetreg/**clang/tree/libclang-cmake-**package-config<https://github.com/daminetreg/clang/tree/libclang-cmake-package-config>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130306/e25a77e9/attachment.html>