<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">On Mar 7, 2013, at 2:36 PM, Damien Buhl <<a href="mailto:damien.buhl@lecbna.org">damien.buhl@lecbna.org</a>> wrote:<br><div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;">On 03/07/2013 03:42 PM, Brad King wrote:> Note that exported targets can put arch-specific information into the<br>> targets file it generates. Therefore lib/ is better than share/ as<br>> a place for the package configuration file.<br>><br>> Also it will take some manual work to get imported targets to work<br>> when building without CMake. A fallback would be to just produce the<br>> basic file with autotools and do the full target exports only when<br>> building with CMake.<br><br>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.<br><br>><br>> The new patch looks good, though I haven't had time to try building it.<br>><br>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.<br><br>On 03/07/2013 08:17 PM, Brad King wrote:<br><blockquote type="cite">On 03/07/2013 02:07 PM, Argyrios Kyrtzidis wrote:<br><blockquote type="cite">On Mar 7, 2013, at 9:59 AM, Brad King <<a href="mailto:brad.king@kitware.com">brad.king@kitware.com</a> <<a href="mailto:brad.king@kitware.com">mailto:brad.king@kitware.com</a>>> wrote:<br><blockquote type="cite">On 03/07/2013 12:32 PM, Jordan Rose wrote:<br><blockquote type="cite">AFAIK libclang is forward-compatible across minor versions, but not<br>backwards (i.e. because new features can be introduced with minor versions).<br></blockquote></blockquote><br>That is correct.<br><br><blockquote type="cite">In that case the logic in the package version file needs to be<br>something like:<br><br>set(PACKAGE_VERSION @LIBCLANG_LIBRARY_VERSION@)<br>if("${PACKAGE_FIND_VERSION_MAJOR}" EQUAL @CLANG_VERSION_MAJOR@ AND<br> NOT "${PACKAGE_FIND_VERSION_MINOR}" GREATER @CLANG_VERSION_MINOR@)<br> set(PACKAGE_VERSION_COMPATIBLE 1)<br> if("${PACKAGE_FIND_VERSION_MINOR}" EQUAL @CLANG_VERSION_MINOR@)<br> set(PACKAGE_VERSION_EXACT 1) # TODO: check patch level too?<br> endif()<br>endif()<br></blockquote><br>The versions for the libclang API are<br><br>CINDEX_VERSION_MAJOR<br>CINDEX_VERSION_MINOR<br><br>in $clang_source/include/clang-c/Index.h<br></blockquote><br>Great. Damien, please fold this into your next patch revision.<br>You can use file(STRINGS) to parse the values out of the header.<br></blockquote><br>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.<br><br>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 :<br><br> set_target_properties(libclang<br> PROPERTIES<br> OUTPUT_NAME "clang"<br> VERSION ${LIBCLANG_LIBRARY_VERSION}<br> DEFINE_SYMBOL _CINDEX_LIB_)<br><br>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.<br><br>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.<br></div></blockquote><div><br></div><div>How about defining ${LIBCLANG_LIBRARY_VERSION} as concatenation of ${CINDEX_VERSION_MAJOR}.${CINDEX_VERSION_MINOR}.${CLANG_VERSION_MAJOR}.${CLANG_VERSION_MINOR},</div><div>for example: libclang 0.12.3.3</div><div><br></div><div>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.</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;"><br>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.<br><br>I believe this is dangerous and error-prone, but I could be missing something.<br><br>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.<br><br>Tell me what you prefer and you think it's correct and I'll do it.<br><br>Thanks for your collaboration on this :),<br>Cheers,<br>--<br>Damien Buhl<br>alias daminetreg</div></blockquote></div><br></body></html>