[PATCH] Adds a CMake package configuration file for libclang

[Argyrios Kyrtzidis]

On Mar 7, 2013, at 2:36 PM, Damien Buhl <damien.buhl at lecbna.org> wrote:

> On 03/07/2013 03:42 PM, Brad King wrote:> Note that exported targets can put arch-specific information into the
> > targets file it generates.  Therefore lib/ is better than share/ as
> > a place for the package configuration file.
> >
> > Also it will take some manual work to get imported targets to work
> > when building without CMake.  A fallback would be to just produce the
> > basic file with autotools and do the full target exports only when
> > building with CMake.
> 
> Ok, so we should also patch location of package configuration for llvm in this case. I'll make an additional patch in correlation to fix the location in llvm.
> 
> >
> > The new patch looks good, though I haven't had time to try building it.
> >
> Thanks ;) This time I tried to build several times, and cmake found the package without any problem and I was able to link to it.
> 
> On 03/07/2013 08:17 PM, Brad King wrote:
>> On 03/07/2013 02:07 PM, Argyrios Kyrtzidis wrote:
>>> On Mar 7, 2013, at 9:59 AM, Brad King <brad.king at kitware.com <mailto:brad.king at kitware.com>> wrote:
>>>> On 03/07/2013 12:32 PM, Jordan Rose wrote:
>>>>> AFAIK libclang is forward-compatible across minor versions, but not
>>>>> backwards (i.e. because new features can be introduced with minor versions).
>>> 
>>> That is correct.
>>> 
>>>> In that case the logic in the package version file needs to be
>>>> something like:
>>>> 
>>>> set(PACKAGE_VERSION @LIBCLANG_LIBRARY_VERSION@)
>>>> if("${PACKAGE_FIND_VERSION_MAJOR}" EQUAL @CLANG_VERSION_MAJOR@ AND
>>>>    NOT "${PACKAGE_FIND_VERSION_MINOR}" GREATER @CLANG_VERSION_MINOR@)
>>>>   set(PACKAGE_VERSION_COMPATIBLE 1)
>>>>   if("${PACKAGE_FIND_VERSION_MINOR}" EQUAL @CLANG_VERSION_MINOR@)
>>>>     set(PACKAGE_VERSION_EXACT 1) # TODO: check patch level too?
>>>>   endif()
>>>> endif()
>>> 
>>> The versions for the libclang API are
>>> 
>>> CINDEX_VERSION_MAJOR
>>> CINDEX_VERSION_MINOR
>>> 
>>> in $clang_source/include/clang-c/Index.h
>> 
>> Great.  Damien, please fold this into your next patch revision.
>> You can use file(STRINGS) to parse the values out of the header.
> 
> Thanks for the information Jordan & Argyrios, and thanks Brad for the package version file logic and the tip about file(STRINGS), I already used it for most FindPackage file I wrote.
> 
> However I believe the CINDEX_VERSION_MAJOR and CINDEX_VERSION_MINOR values shouldn't be used here, because currently libclang version is defined through the variable LIBCLANG_LIBRARY_VERSION, which is even used as target property version in $clang_source/tools/libclang/CMakeLists.txt, lines 88-92 :
> 
>    set_target_properties(libclang
>      PROPERTIES
>      OUTPUT_NAME "clang"
>      VERSION ${LIBCLANG_LIBRARY_VERSION}
>      DEFINE_SYMBOL _CINDEX_LIB_)
> 
> And the problem is that ${LIBCLANG_LIBRARY_VERSION} is defined as the concatenation of ${CLANG_VERSION_MAJOR}.${CLANG_VERSION_MINOR}, so the package version file should have an history telling the mapping between LIBCLANG_LIBRARY_VERSION and CINDEX_VERSION_MAJOR+CINDEX_VERSION_MINOR.
> 
> This isn't pretty at all, and I don't think it's maintainable, so shouldn't LIBCLANG_LIBRARY_VERSION be made of CINDEX_VERSION_MAJOR+CINDEX_VERSION_MINOR ? I don't think that this is doable, in fact we need to know how the policy is between the CINDEX_VERSION_MAJOR+CINDEX_VERSION_MINOR and LIBCLANG_LIBRARY_VERSION. Because users of the lib will never find_package(libclang 0.12) if they install libclang-3.3.

How about defining ${LIBCLANG_LIBRARY_VERSION} as concatenation of ${CINDEX_VERSION_MAJOR}.${CINDEX_VERSION_MINOR}.${CLANG_VERSION_MAJOR}.${CLANG_VERSION_MINOR},
for example: libclang 0.12.3.3

0.12 is the important one for linking/using the libclang API, but it is also good to know the clang version it was built with.

> 
> What do you think that would be better to do here ? Define some policy for the LIBCLANG_LIBRARY_VERSION change in relation to CINDEX_VERSION_MAJOR+CINDEX_VERSION_MINOR or be ready to store in the future some complicated mapping between these information in the package version file, and find a way that someone makes sure that this get updated in the file when tagging a new libclang version.
> 
> I believe this is dangerous and error-prone, but I could be missing something.
> 
> As a consequence if we cannot have any policy for the mapping between library version and cindex version, I would rather tend to a test for exact version equality than expecting libclang maintainers update cmake package version file each time they change CINDEX_VERSION_MAJOR.
> 
> Tell me what you prefer and you think it's correct and I'll do it.
> 
> Thanks for your collaboration on this :),
> Cheers,
> --
> Damien Buhl
> alias daminetreg

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130307/54f0fce7/attachment.html>