[cmake-developers] I: I want to become a maintainer of several modules.

[Aleksey Avdeev]

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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 900 bytes
Desc: OpenPGP digital signature
URL: <http://public.kitware.com/pipermail/cmake-developers/attachments/20121005/5cd5009a/attachment.sig>