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:
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. (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. > 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 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. 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. > 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 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 Nice. The documentation of the macros should mention their requirement on things like ICOTOOL_FOUND to work. > 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>.) Why do the names of the module and macro not match? Is the module name meant to include future macros too? The documentation should mention that the install destination is controlled by CMAKE_INSTALL_FULL_MANDIR from GNUInstallDirs. The macro should complain if there are extra arguments. That will allow extension in the future with additional options. The GPL license block needs to be removed for distribution in CMake under our pure BSD license. We'll need Andreas Schneider's permission. Thanks, -Brad -- 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
