[CMake] FindImageMagick: Rewrite to support all utilities.

[a.neundorf-work at gmx.net]

On Thursday 18 October 2007 21:25, Miguel A. Figueroa-Villanueva wrote:
> On 10/14/07, Alex Neundorf wrote:
> > On Saturday 13 October 2007 07:17, Miguel A. Figueroa-Villanueva wrote:
> > > Hello,
> > >
> > > I've been assigned the task of maintaining the ImageMagick find
> > > module. As such, I made a rewrite to support the complete toolchain
> > > (i.e., all command line executables) and would like to ask for user
> > > feedback before committing the changes. The new module is attached for
> > > reference.
> > >
> > > The pattern that I'm using for this find module comes up a few times,
> > > for example FindLATEX could use it. Instead of hard-coding the tools
> > > to search for I use the COMPONENTS argument in of the FIND_PACKAGE
> > > command. For example, if I need ImageMagick's convert I would do:
> > >
> > > FIND_PACKAGE(ImageMagick COMPONENTS convert)
> > >
> > > If found the following variables will be set:
> > >
> > > #  ImageMagick_FOUND - TRUE if all components are found.
> > > #  ImageMagick_EXECUTABLE_DIR - Full path to executables directory.
> > > #  ImageMagick_<component>_FOUND - TRUE if <component> is found.
> > > #  ImageMagick_<component>_EXECUTABLE - Full path to <component>
> > > executable.
> >
> > Why do you use FIND_PATH() instead of FIND_PROGRAM() ?
> > Then you wouldn't have to care for the different suffixes.
>
> Something like this should work:
>
> FIND_PROGRAM(ImageMagick_mogrify_EXECUTABLE mogrify ...)
> GET_FILENAME_COMPONENT(ImageMagick_EXECUTABLE_DIR
>        ImageMagick_mogrify_EXECUTABLE PATH
> )
>
> ...
>
> Then the only variable in the CACHE would be
> ImageMagick_mogrify_EXECUTABLE instead of ImageMagick_EXECUTABLE_DIR.
>
> Should I change ImageMagick_EXECUTABLE_DIR to ImageMagick_ROOT_DIR ??

Can you please post the new file before committing ?

> Since I haven't had any objections I'll go ahead and commit with the
> previous change (i.e., find_program thing above). Note also that I
> would like to make the following additions:
>
> - add XXX_EXECUTABLE_DIR to the variables in the readme.txt in the
> Modules directory, unless it is recommended to use the XXX_ROOT_DIR
> naming.

Please ask for that in an extra email with an obvious subject, so that the 
cmake developers will notice it and can give their opinion.

> - create a macro in the same line of
> FIND_PACKAGE_HANDLE_STANDARD_ARGS(...), and possibly in the same file,
> that checks if all components are found to set the main variable:
>
> FIND_PACKAGE_HANDLE_STANDARD_COMPONENTS(ImageMagick)
>
> would basically do a:
>
> # check if all components are found to set XXX_FOUND
> SET(XXX_FOUND TRUE)
> FOREACH (component ${XXX_FIND_COMPONENTS})
>   IF (NOT XXX_${component}_FOUND)
>     SET(XXX_FOUND FALSE)
>   ENDIF (NOT XXX_${component}_FOUND)
> ENDFOREACH (component)

Maybe this loop could also just be added to the  
FIND_PACKAGE_HANDLE_STANDARD_ARGS() macro ?
If no components are specified, then there will be no change in behaviour, 
right ?

> - Create a list of clean, standard compliant, non-patched cmake
> modules that can be used as example references for module developers.
> This could be added in the Module maintainer wiki page and/or the
> readme.txt and would add context to the specs in the readme.txt file.

Good idea :-)

Bye
Alex