On Wed, Mar 6, 2013 at 11:33 AM, Damien Buhl <[email protected]> wrote:
> 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 ? 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 :) Cheers, /Manuel > > > > > > 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<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<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<http://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<http://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<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
