<div dir="ltr"><div style>+echristo</div><div><br></div>On Wed, Mar 6, 2013 at 4:40 PM, Damien Buhl <span dir="ltr"><<a href="mailto:damien.buhl@lecbna.org" target="_blank">damien.buhl@lecbna.org</a>></span> wrote:<br>
<div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On 03/07/2013 01:17 AM, Manuel Klimek wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
No, I meant: if that's what cmake integration should look like; that's<br>
why I pulled in Brad, who was nice enough to put in a lot of feedback,<br>
so I can basically say: please address Brad's comments :)<br>
</blockquote></div>
In fact in my previous mail I was adressing both comments, but I sent the mail accidentally. :|<br>
<br>
Thank you for taking time for this, it's nice Manuel that you cc'ed Brad King.<br>
<br>
Brad it's really nice that you replied so fast and that you help me in a great and detailed way like this.<div class="im"><br>
<br>
On 03/06/2013 05:26 PM, Brad King wrote:<br></div><div class="im">
>> +++ b/tools/libclang/cmake/<u></u>modules/CMakeLists.txt<br>
>> ...<br>
>> +set(libclang_cmake_builddir "${CMAKE_BINARY_DIR}/share/<u></u>clang/cmake")<br>
><br>
> FYI, the location inside the build tree is just a staging area<br>
> for install(FILES) and the files should never be loaded from<br>
> there.<br>
<br></div>
I'm aware of this. I tried to make something equal to llvm/cmake/modules/CMakeLists.<u></u>txt which also declares a variable in the same way to provide package configuration file for llvm libraries.<div class="im"><br>
<br>
><br>
>> +set(LLVM_INSTALL_PREFIX ${CMAKE_INSTALL_PREFIX})<br>
><br>
> You don't seem to actually use this variable anywhere in<br>
> the configure_file inputs.<br>
><br>
<br></div>
Yes this is silly indeed to have it defined. :) It was to have the same as llvm/cmake/modules/CMakeLists.<u></u>txt, I removed it in the new patch.<div class="im"><br>
<br>
> Also CMake will not look in "clang" for a package called "libclang"<br>
> without extra options to find_package.<br>
<br></div>
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.<div class="im"><br>
<br>
> Finally, there could be more<br>
> than one version of libclang in the same prefix (someday). I suggest<br>
> installing the package configuration file and package version file to<br>
><br>
> lib/cmake/libclang-<version><br>
><br>
> where <version> is "${CLANG_VERSION_MAJOR}.${@<u></u>CLANG_VERSION_MINOR}".<br>
><br>
>> +# target_link_libraries(<u></u>youTarget clang)<br>
><br>
<br></div>
It's nice to think to the future indeed, I changed this, but took LIBCLANG_LIBRARY_VERSION instead of ${CLANG_VERSION_MAJOR}.${<u></u>CLANG_VERSION_MINOR}, since it's also used in the package version file. :)<div class="im">
<br>
<br>
> Using link_directories/target_link_<u></u>libraries with a library name<br>
> will work but it will not hook up a dependency to cause the app<br>
> to re-link when the clang library file changes. To achieve that<br>
> you need full target export/import support:<br>
><br>
> <a href="http://www.cmake.org/Wiki/CMake/Tutorials/Exporting_and_Importing_Targets" target="_blank">http://www.cmake.org/Wiki/<u></u>CMake/Tutorials/Exporting_and_<u></u>Importing_Targets</a><br>
><br>
> That can be added later though. The basic find_package(libclang)<br>
> functionality will work without this.<br>
<br></div>
This would be nice and especially for people patching libclang, testing their custom build of libclang.<br>
<br>
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.<div class="im">
<br>
<br>
><br>
>> +++ b/tools/libclang/cmake/<u></u>modules/<a href="http://libclang-config.cmake.in" target="_blank">libclang-config.cmake.<u></u>in</a><br>
>> ...<br>
>> +message(STATUS "libclang version: ${LIBCLANG_PACKAGE_VERSION}")<br>
><br>
> Package configuration files should not display any messages.<br>
> If the project loading it wants to display that kind of verbose<br>
> information it can do so itself.<br>
><br>
<br></div>
Ihis makes sense, I didn't know, because often FindPackages file performs status outputs. Thanks. :)<div class="im"><br>
<br>
>> The other question is that I think that there are still<br>
>> many (basically all) package releases that use the<br>
>> configure-based build. We'll need a solution for those as<br>
>> well. Is the idea that we just copy the correctly versioned<br>
> Yes, you need to configure the two files<br>
><br>
> <a href="http://libclang-config.cmake.in" target="_blank">libclang-config.cmake.in</a><br>
> <a href="http://libclang-config-version.cmake.in" target="_blank">libclang-config-version.cmake.<u></u>in</a><br>
><br>
> with proper @...@ replacements and install them.<br>
><br>
>> files into <prefix>/share/clang/cmake?<br>
><br>
> Yes, though see above recommendation about install destination:<br>
><br>
> lib/cmake/libclang-<version><br>
><br>
<br></div>
@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.<br>
</blockquote><div><br></div><div style>I have no idea about the configure build, cc'ed echristo for ideas here...</div><div style><br></div><div style>Cheers,</div><div style>/Manuel</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Please find a new patch with the different modifications discussed as attachment.<br>
<br>
Cheers and thank you for your help,<div class="im"><br>
--<br>
Damien Buhl<br>
alias daminetreg<br>
<br></div>
P.S. The patch is also available on github here : <a href="https://github.com/daminetreg/clang/tree/libclang-cmake-package-config" target="_blank">https://github.com/daminetreg/<u></u>clang/tree/libclang-cmake-<u></u>package-config</a><br>
</blockquote></div><br></div></div>