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
