On Mar 7, 2013, at 2:36 PM, Damien Buhl <[email protected]> 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 <[email protected] >>> <mailto:[email protected]>> 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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
