[cmake-developers] [PATCH] New module: FindIce.cmake

[Brad King]

Hi Roger,

Thanks for the revisions!

On 08/12/2014 05:59 PM, Roger Leigh wrote:
>> Regarding the Windows Registry, I've taken a look and it looks
>> like there might be some usable keys from the installer which
>> could be used, but I'll need to do further digging with all
>> the different versions to see what's most usable here.
> 
> This turned out to be fairly simple at least for Ice versions
> 3.4.0 - 3.5.1, which all have the same naming convention.
> Earlier versions have odd naming conventions and are in
> Wow6432Node so I've not included them (they are obsolete in
> any case, and ICE_HOME can be set to use them).

The Wow6432Node is for distinguishing between 32-bit and 64-bit
views of the registry.  Just omit it from any registry key path.
When CMake find_* commands evaluate registry values they take
into account whether the target architecture is 32-bit or 64-bit
and prefer the matching registry view.

So, please go ahead and add the registry entries for the earlier
versions.  Just convert any "\Wow6432Node\" to "\" in the keys.

Other comments:

> +# Components can include any of: Freeze Glacier2 Ice IceBox IceDB
> +# IceGrid IcePatch IceSSL IceStorm IceUtil IceXML Slice.

I think each name in the list should be ``quoted`` to clarify
that it is a name to appear in code.

> +#   ICE_LIBRARIES - component libraries to be linked

Is that a typo in the docs?  It should be Ice_LIBRARIES.

> +#   ICE_<C>_LIBRARY - component library

Although the individual find result is a singular-named cache entry
the results presented to the project should still be plural-named
normal variables:

 Ice_<C>_LIBRARY - cache entry
 Ice_<C>_LIBRARIES - results (even if only one entry)

> +set(ICE_HOME NOTFOUND
> +    CACHE PATH "Location of the Ice installation")
> +mark_as_advanced(FORCE ICE_HOME)
> +
> +set(ICE_BINARYDIR NOTFOUND
> +    CACHE PATH "Location of the Ice programs")
> +mark_as_advanced(FORCE ICE_BINARYDIR)
> +
> +set(ICE_INCLUDEDIR NOTFOUND
> +    CACHE PATH "Location of the Ice headers")
> +mark_as_advanced(FORCE ICE_INCLUDEDIR)
> +
> +set(ICE_SLICEDIR NOTFOUND
> +    CACHE PATH "Location of the Ice slice interface definitions")
> +mark_as_advanced(FORCE ICE_SLICEDIR)
> +
> +set(ICE_LIBRARYDIR NOTFOUND
> +    CACHE PATH "Location of the Ice libraries")
> +mark_as_advanced(FORCE ICE_LIBRARYDIR)

These hints should not need to be explicitly cached with such
granularity.  The find_* commands create cache entries for
their results both to save them persistently and to allow users
to set them granularly.  The CMAKE_PREFIX_PATH variable allows
users to add custom prefixes without a per-package _HOME or
_ROOT variable.

> +  list(APPEND ice_slice_paths
> +    "/usr/local/share/Ice-${Ice_VERSION_SLICE2CPP_FULL}/slice"
> +    "/usr/local/share/Ice-${Ice_VERSION_SLICE2CPP_SHORT}/slice"
> +    "/usr/local/share/Ice/slice"
> +    "/usr/share/Ice-${Ice_VERSION_SLICE2CPP_FULL}/slice"
> +    "/usr/share/Ice-${Ice_VERSION_SLICE2CPP_SHORT}/slice"
> +    "/usr/share/Ice/slice")

It looks like a lot of the explicit search location construction
could be simplified using PATH_SUFFIXES options to the find
commands.  There should not need to be any explicit copies of
paths for every common prefix.

Another option you might be interested in exploring, but not
as a requirement for our acceptance of this module, is creation
of imported targets to hold the find results with usage reqs.
See the cmake-buildsystem(7) manual:

 http://www.cmake.org/cmake/help/v3.0/manual/cmake-buildsystem.7.html

for more info.  See the FindQt4 module for an example.  This
part could be added as a follow-up change once the main module
is in though.

-Brad