12.07.2012 18:41, Brad King пишет: > On 07/10/2012 06:12 PM, Aleksey Avdeev wrote: >> I maintainer of ALT Linux Team (see >> <http://en.altlinux.org/ALT_Linux_Team>). Packaging italc2 (see >> <https://sourceforge.net/projects/italc/>), I made a few modules to >> CMake. I think they will be useful not only to me:
I modified the proposed modules. > > Thanks for your interest in contributing! > > A few general comments: > > (1) Please convert the code in the modules to use lower-case commands. > We are considering a "flag day" to massively convert all modules in > CMake anyway, and your "git blame" credit will be preserved if the > modules start lower case. Done. > > (2) Please try putting the modules in CMake's source locally and check > the output of "bin/cmake --help-module $mod" for each module. Adjust > the comments and documentation at the top of them to make the output > look good. Done. Ask to check the result. > >> 1. FindIcotool (see >> <http://git.altlinux.org/people/solo/public/?p=cmake-modules.git;a=blob;f=Modules/FindIcotool.cmake;h=d6566f6f49678e3691381eb8b6c472208c8c6be2;hb=FindIcotool>) >> -- find icotool See <http://git.altlinux.org/people/solo/public/cmake-modules.git?p=cmake-modules.git;a=blob;f=Modules/FindIcotool.cmake;h=97543113756a72e59175c778eace0dc00220a1e0;hb=ea809d0c5b1cd36b83437043d916fd6eacc1dc5f>. > > Hard-coded paths like > > /bin > /usr/bin > /usr/local/bin > /sbin > /usr/sbin > /usr/local/sbin > > should not be necessary. The find_program() command already searches them. OK. Done. > > Also, Eike (CCed) did a wonderful sweep recently to add version support to > as many find modules as possible. Please consider adding support to this > module too, and update "Tests/CMakeOnly/AllFindModules/CMakeLists.txt" to > verify it. Done. Value ICOTOOL_VERSION_STRING up over the output: icotool --version > >> 2. FreedesktopIconsMacros (see >> <http://git.altlinux.org/people/solo/public/?p=cmake-modules.git;a=blob;f=Modules/FreedesktopIconsMacros.cmake;h=bb48f6dfe0898a83974c611154e04508e8f05684;hb=FreedesktopIconsMacros>) >> -- define freedesktop.org standard for icons installation See <http://git.altlinux.org/people/solo/public/cmake-modules.git?p=cmake-modules.git;a=blob;f=Modules/FreedesktopIconsMacros.cmake;h=b02d29fdef822cba1e7c1c12bd321d0c1ec83e2d;hb=53431ba5ddd8cf99578f9854242bccc5451a8d52>. > > It looks like this extends the concept of GNUInstallDirs: > > > http://cmake.org/gitweb?p=cmake.git;a=blob;f=Modules/GNUInstallDirs.cmake;hb=v2.8.8 Yes, variables and CMAKE_INSTALL_FREEDESKTOP_ICONS_BASE CMAKE_INSTALL_FREEDESKTOP_ICONS_FULL_BASE would be logical to move the module GNUInstallDirs. (Perhaps it is worth to use other, more standard loya module variable names.) But I think that this issue is addressed later, when the proposed unit will already be included in the code CMake. > > Nice. The documentation of the macros should mention their requirement > on things like ICOTOOL_FOUND to work. Done. The documentation module added a description of its dependencies. Well done: * Description MacOS FREEDESKTOP_INSTALL_ICONS moved to module documentation. * In macros added checking of arguments. > >> 3. MacroProcessManpages (see >> <http://git.altlinux.org/people/solo/public/?p=cmake-modules.git;a=blob;f=Modules/MacroProcessManpages.cmake;h=5530c0444acd074d5ab05b52d8185a475afa77df;hb=MacroProcessManpages>) >> -- find manpages in the given directory and install. (This module is >> modified >> <http://cmake-modules.googlecode.com/svn/trunk/Modules/Macros/Manpages/MacroProcessManpages.cmake>.) See <http://git.altlinux.org/people/solo/public/cmake-modules.git?p=cmake-modules.git;a=blob;f=Modules/InstallManpages.cmake;h=ea3ebb3df5e3f7522acaf6d62a95dcce8ef169fb;hb=f98a0679300901592d19fbe466e569838dbb8ae6>. > > Why do the names of the module and macro not match? > Is the module name meant to include future macros too? The module name was inherited from the code Andreas Schneider. Renamed InstallManpages.cmake. > > The documentation should mention that the install destination > is controlled by CMAKE_INSTALL_FULL_MANDIR from GNUInstallDirs. Done. Ask to check the result. > > The macro should complain if there are extra arguments. That will > allow extension in the future with additional options. Done. > > The GPL license block needs to be removed for distribution in CMake > under our pure BSD license. We'll need Andreas Schneider's permission. Done. Permission is obtained: 27.09.2012 16:45, Andreas Schneider wrote: > You have the permission to license it under BSD. -- Sincerely. Alex.
signature.asc
Description: OpenPGP digital signature
-- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
