Hi Manuel,
On 03/06/2013 10:01 AM, Manuel Klimek wrote:
for the future: it'll be easier to get your patches reviewed if you
don't directly address a single dev (in this case Chandler, whose review
queue is constantly over-subscribed anyway).
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.
On 03/06/2013 10:01 AM, Manuel Klimek wrote:
Looking at your patch, it generally looks good if the high level direction is
right...
Do you mean that libclang shouldn't be so easily accessible for user
code ? Or am I missing something ?
On 03/06/2013 05:26 PM, Brad King wrote:
On 03/06/2013 10:01 AM, Manuel Klimek wrote:
Looking at your patch, it generally looks good if the high
level direction is right... I'm cc'ing Brad King, who has
responded to your original pull request, in the hope that he
can confirm that this looks good from a cmake perspective.
I just looked at the patch as posted here:
http://thread.gmane.org/gmane.comp.compilers.clang.scm/68052
though I have not actually tried building with it.
It is a great start. Thanks for working on it.
+++ b/tools/libclang/cmake/modules/CMakeLists.txt
...
+set(libclang_cmake_builddir "${CMAKE_BINARY_DIR}/share/clang/cmake")
FYI, the location inside the build tree is just a staging area
for install(FILES) and the files should never be loaded from
there.
+set(LLVM_INSTALL_PREFIX ${CMAKE_INSTALL_PREFIX})
You don't seem to actually use this variable anywhere in
the configure_file inputs.
+++ b/tools/libclang/cmake/modules/CMakeLists.txt
...
+install(FILES
+ ${libclang_cmake_builddir}/libclang-config.cmake
+ ${libclang_cmake_builddir}/libclang-config-version.cmake
+ DESTINATION share/clang/cmake)
In the future the file may contain architecture-specific information.
Also CMake will not look in "clang" for a package called "libclang"
without extra options to find_package. Finally, there could be more
than one version of libclang in the same prefix (someday). I suggest
installing the package configuration file and package version file to
lib/cmake/libclang-<version>
where <version> is "${CLANG_VERSION_MAJOR}.${@CLANG_VERSION_MINOR}".
+# target_link_libraries(youTarget clang)
s/you/your/ ?
Using link_directories/target_link_libraries with a library name
will work but it will not hook up a dependency to cause the app
to re-link when the clang library file changes. To achieve that
you need full target export/import support:
http://www.cmake.org/Wiki/CMake/Tutorials/Exporting_and_Importing_Targets
That can be added later though. The basic find_package(libclang)
functionality will work without this.
+++ b/tools/libclang/cmake/modules/libclang-config.cmake.in
...
+message(STATUS "libclang version: ${LIBCLANG_PACKAGE_VERSION}")
Package configuration files should not display any messages.
If the project loading it wants to display that kind of verbose
information it can do so itself.
The other question is that I think that there are still
many (basically all) package releases that use the
configure-based build. We'll need a solution for those as
well. Is the idea that we just copy the correctly versioned
Yes, you need to configure the two files
libclang-config.cmake.in
libclang-config-version.cmake.in
with proper @...@ replacements and install them.
files into <prefix>/share/clang/cmake?
Yes, though see above recommendation about install destination:
lib/cmake/libclang-<version>
If yes, how does cmake find them there?
The CMake find_package command:
http://www.cmake.org/cmake/help/v2.8.10/cmake.html#command:find_package
will search in <prefix>/lib/cmake/<name>* where <name> is the
package to be found (case insensitive) and "*" globs any suffix
on the directory name (like "-<version>"). The command documents
how it constructs the list of possible <prefix>es.
-Brad
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
