On 03/07/2013 01:17 AM, Manuel Klimek wrote:
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 :)
In fact in my previous mail I was adressing both comments, but I sent the mail accidentally. :|

Thank you for taking time for this, it's nice Manuel that you cc'ed Brad King.

Brad it's really nice that you replied so fast and that you help me in a great and detailed way like this.

On 03/06/2013 05:26 PM, Brad King wrote:
>> +++ 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.

I'm aware of this. I tried to make something equal to llvm/cmake/modules/CMakeLists.txt which also declares a variable in the same way to provide package configuration file for llvm libraries.

>
>> +set(LLVM_INSTALL_PREFIX ${CMAKE_INSTALL_PREFIX})
>
> You don't seem to actually use this variable anywhere in
> the configure_file inputs.
>

Yes this is silly indeed to have it defined. :) It was to have the same as llvm/cmake/modules/CMakeLists.txt, I removed it in the new patch.

> Also CMake will not look in "clang" for a package called "libclang"
> without extra options to find_package.

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.

> 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)
>

It's nice to think to the future indeed, I changed this, but took LIBCLANG_LIBRARY_VERSION instead of ${CLANG_VERSION_MAJOR}.${CLANG_VERSION_MINOR}, since it's also used in the package version file. :)

> 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.

This would be nice and especially for people patching libclang, testing their custom build of libclang.

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.

>
>> +++ 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.
>

Ihis makes sense, I didn't know, because often FindPackages file performs status outputs. Thanks. :)

>> 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>
>

@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.

Please find a new patch with the different modifications discussed as attachment.

Cheers and thank you for your help,
--
Damien Buhl
alias daminetreg

P.S. The patch is also available on github here : https://github.com/daminetreg/clang/tree/libclang-cmake-package-config
>From 40e64690db58e100820aef48f3e753864bbadad7 Mon Sep 17 00:00:00 2001
From: "Damien Buhl (alias daminetreg)" <[email protected]>
Date: Thu, 7 Mar 2013 01:12:16 +0100
Subject: [PATCH] Adds a CMake package configuration file for libclang, so
 that library users from CMake based project can configure
 their dependency to libclang easily.

Signed-off-by: Damien Buhl (alias daminetreg) <[email protected]>
---
 tools/libclang/CMakeLists.txt                      |    2 ++
 tools/libclang/cmake/modules/CMakeLists.txt        |   16 ++++++++++
 .../cmake/modules/libclang-config-version.cmake.in |    9 ++++++
 .../cmake/modules/libclang-config.cmake.in         |   31 ++++++++++++++++++++
 4 files changed, 58 insertions(+)
 create mode 100644 tools/libclang/cmake/modules/CMakeLists.txt
 create mode 100644 tools/libclang/cmake/modules/libclang-config-version.cmake.in
 create mode 100644 tools/libclang/cmake/modules/libclang-config.cmake.in

diff --git a/tools/libclang/CMakeLists.txt b/tools/libclang/CMakeLists.txt
index dd4eccf..31ab8c4 100644
--- a/tools/libclang/CMakeLists.txt
+++ b/tools/libclang/CMakeLists.txt
@@ -1,3 +1,5 @@
+add_subdirectory(cmake/modules)
+
 set(LLVM_LINK_COMPONENTS
   ${LLVM_TARGETS_TO_BUILD}
   asmparser
diff --git a/tools/libclang/cmake/modules/CMakeLists.txt b/tools/libclang/cmake/modules/CMakeLists.txt
new file mode 100644
index 0000000..ed077d5
--- /dev/null
+++ b/tools/libclang/cmake/modules/CMakeLists.txt
@@ -0,0 +1,16 @@
+set(libclang_cmake_builddir "${CMAKE_BINARY_DIR}/share/cmake/libclang-${LIBCLANG_LIBRARY_VERSION}")
+
+configure_file (
+    libclang-config.cmake.in
+    ${libclang_cmake_builddir}/libclang-config.cmake
+    @ONLY)
+
+configure_file (
+    libclang-config-version.cmake.in
+    ${libclang_cmake_builddir}/libclang-config-version.cmake
+    @ONLY)
+
+install(FILES
+    ${libclang_cmake_builddir}/libclang-config.cmake
+    ${libclang_cmake_builddir}/libclang-config-version.cmake
+    DESTINATION share/cmake/libclang-${LIBCLANG_LIBRARY_VERSION})
diff --git a/tools/libclang/cmake/modules/libclang-config-version.cmake.in b/tools/libclang/cmake/modules/libclang-config-version.cmake.in
new file mode 100644
index 0000000..50dffd3
--- /dev/null
+++ b/tools/libclang/cmake/modules/libclang-config-version.cmake.in
@@ -0,0 +1,9 @@
+set(PACKAGE_VERSION @LIBCLANG_LIBRARY_VERSION@)
+if("${PACKAGE_FIND_VERSION_MAJOR}" EQUAL @CLANG_VERSION_MAJOR@)
+    set(PACKAGE_VERSION_COMPATIBLE 1)
+
+    if("${PACKAGE_FIND_VERSION_MINOR}" EQUAL @CLANG_VERSION_MINOR@)
+        set(PACKAGE_VERSION_EXACT 1)
+    endif("${PACKAGE_FIND_VERSION_MINOR}" EQUAL @CLANG_VERSION_MINOR@)
+
+endif("${PACKAGE_FIND_VERSION_MAJOR}" EQUAL @CLANG_VERSION_MAJOR@)
diff --git a/tools/libclang/cmake/modules/libclang-config.cmake.in b/tools/libclang/cmake/modules/libclang-config.cmake.in
new file mode 100644
index 0000000..52460c6
--- /dev/null
+++ b/tools/libclang/cmake/modules/libclang-config.cmake.in
@@ -0,0 +1,31 @@
+# libclang @LIBCLANG_LIBRARY_VERSION@ - CMake Package Configuration File
+#
+#   To use libclang in your software, simply add the following to your 
+#   CMakeLists.txt :
+#
+#       find_package(libclang @LIBCLANG_LIBRARY_VERSION@ REQUIRED)
+#       link_directories(${LIBCLANG_LIBRARY_DIRS}) 
+#       include_directories(${LIBCLANG_INCLUDE_DIRS})
+#       add_definitions(${LIBCLANG_DEFINITIONS})
+#
+#   Once this done you can simply add *clang* to your target_link_libraries, e.g:
+#       target_link_libraries(yourTarget clang)
+#       
+#   In case you have installed an LLVM version which was compiled without 
+#   defining REQUIRES_RTTI=1, then you are probably getting problems with
+#   missing  typeid symbols (i.e. of the mangled form : _ZTIN5[...]E) when the 
+#   dynamic linker resolve symbols in your binary. 
+#
+#   To solve the issue, find a version of llvm and clang compiled with rtti 
+#   enabled, because this should be the case for released clang versions.
+#   (c.f. llvm/docs/Packaging.rst)
+#
+
+set(LIBCLANG_VERSION_MAJOR @CLANG_VERSION_MAJOR@)
+set(LIBCLANG_VERSION_MINOR @CLANG_VERSION_MINOR@)
+set(LIBCLANG_PACKAGE_VERSION @LIBCLANG_LIBRARY_VERSION@)
+
+set(LIBCLANG_INSTALL_PREFIX "@CMAKE_INSTALL_PREFIX@")
+set(LIBCLANG_INCLUDE_DIRS ${LIBCLANG_INSTALL_PREFIX}/include)
+set(LIBCLANG_LIBRARY_DIRS ${LIBCLANG_INSTALL_PREFIX}/lib)
+set(LIBCLANG_DEFINITIONS "-D__STDC_LIMIT_MACROS" "-D__STDC_CONSTANT_MACROS")
-- 
1.7.9.5

_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Reply via email to