<div dir="ltr">On Wed, Mar 6, 2013 at 11:33 AM, 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">Hi Manuel,<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
On 03/06/2013 10:01 AM, Manuel Klimek wrote:<br></div><div class="im">
for the future: it'll be easier to get your patches reviewed if you<br>
don't directly address a single dev (in this case Chandler, whose review<br>
queue is constantly over-subscribed anyway).<br>
</div></blockquote>
<br>
Thank you for the tip ;), on other projects I usually address any developer, but the code owner file brought me to address it only to Chandler.<div class="im"><br>
<br>
On 03/06/2013 10:01 AM, Manuel Klimek wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Looking at your patch, it generally looks good if the high level direction is right...<br>
</blockquote>
<br></div>
Do you mean that libclang shouldn't be so easily accessible for user code ? Or am I missing something ?</blockquote><div><br></div><div style>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 :)</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"><div class="HOEnZb"><div class="h5"><br>
<br>
<br>
<br>
<br>
On 03/06/2013 05:26 PM, Brad King wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On 03/06/2013 10:01 AM, Manuel Klimek wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Looking at your patch, it generally looks good if the high<br>
level direction is right... I'm cc'ing Brad King, who has<br>
responded to your original pull request, in the hope that he<br>
can confirm that this looks good from a cmake perspective.<br>
</blockquote>
<br>
I just looked at the patch as posted here:<br>
<br>
<a href="http://thread.gmane.org/gmane.comp.compilers.clang.scm/68052" target="_blank">http://thread.gmane.org/gmane.<u></u>comp.compilers.clang.scm/68052</a><br>
<br>
though I have not actually tried building with it.<br>
<br>
It is a great start. Thanks for working on it.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+++ b/tools/libclang/cmake/<u></u>modules/CMakeLists.txt<br>
...<br>
+set(libclang_cmake_builddir "${CMAKE_BINARY_DIR}/share/<u></u>clang/cmake")<br>
</blockquote>
<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>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+set(LLVM_INSTALL_PREFIX ${CMAKE_INSTALL_PREFIX})<br>
</blockquote>
<br>
You don't seem to actually use this variable anywhere in<br>
the configure_file inputs.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+++ b/tools/libclang/cmake/<u></u>modules/CMakeLists.txt<br>
...<br>
+install(FILES<br>
+ ${libclang_cmake_builddir}/<u></u>libclang-config.cmake<br>
+ ${libclang_cmake_builddir}/<u></u>libclang-config-version.cmake<br>
+ DESTINATION share/clang/cmake)<br>
</blockquote>
<br>
In the future the file may contain architecture-specific information.<br>
Also CMake will not look in "clang" for a package called "libclang"<br>
without extra options to find_package. 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>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+# target_link_libraries(<u></u>youTarget clang)<br>
</blockquote>
<br>
s/you/your/ ?<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>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+++ 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>
</blockquote>
<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>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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>
</blockquote>
<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>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
files into <prefix>/share/clang/cmake?<br>
</blockquote>
<br>
Yes, though see above recommendation about install destination:<br>
<br>
lib/cmake/libclang-<version><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
If yes, how does cmake find them there?<br>
</blockquote>
<br>
The CMake find_package command:<br>
<br>
<a href="http://www.cmake.org/cmake/help/v2.8.10/cmake.html#command:find_package" target="_blank">http://www.cmake.org/cmake/<u></u>help/v2.8.10/cmake.html#<u></u>command:find_package</a><br>
<br>
will search in <prefix>/lib/cmake/<name>* where <name> is the<br>
package to be found (case insensitive) and "*" globs any suffix<br>
on the directory name (like "-<version>"). The command documents<br>
how it constructs the list of possible <prefix>es.<br>
<br>
-Brad<br>
<br>
</blockquote>
</div></div></blockquote></div><br></div></div>