[cmake-developers] Volunteering to maintain a new module: FindGSL.cmake

[Brad King]

On 12/02/2014 12:06 PM, Thompson, KT wrote:
> A copy of my proposed module follows.

Great, thanks.  Here are some comments.

> #=============================================================================

This line should be dropped.

> #  GSL_INCLUDE_DIR    - Location of GSL header files.
> #  GSL_LIBRARY        - The GSL library.
> #  GSL_CBLAS_LIBRARY  - The GSL CBLAS library.

Please read the section of the developer manual on variable names.
The singular names used for cache entries referring to individual
files should not be presented as results.  Instead they should be
presented in non-cached variables with plural names:

 GSL_INCLUDE_DIRS
 GSL_LIBRARIES
 GSL_CBLAS_LIBRARIES

> #  GSL_ROOT_DIR       - The top level directory of the discovered GSL 
> #                       install (useful if GSL is not in a standard location)

This and the other singular names should be documented as variables
that are used as hints or to record results.

> # It will also provide the CMake library target names gsl::gsl and
> # gsl::gslcblas. 

Please have a dedicated documentation section for the imported targets
and use a definition list for them.  Also the convention that has been
established in a few other find modules is to use the "<pkg>::" namespace
for modules where "<pkg>" matches what users put in "find_package()" calls.
That would be "GSL::" for FindGSL.

> # Warn about using with older versions of CMake...
> # - This module uses PkgConfig (since 2.8), import libraries (since
> #   2.6), but developed and only tested with CMake 3.0+
> if(CMAKE_MINIMUM_REQUIRED_VERSION VERSION_LESS 3.0.0)
>   message(AUTHOR_WARNING
>     "Your project should require at least CMake 3.0.0 to use FindGSL.cmake")
> endif()

This section is not needed when the module comes with CMake.

> include(FindPackageHandleStandardArgs)

Use a full path to include dependencies:

 include(${CMAKE_CURRENT_LIST_DIR}/FindPackageHandleStandardArgs.cmake)

> include(GetPrerequisites)

This module is used for packaging and should not be needed for find modules
in general.

> # http://www.cmake.org/cmake/help/v3.0/module/FindPkgConfig.html

Links like this should not be present for modules in upstream CMake.

>     exec_program( "${GSL_CONFIG}"
>       ARGS --version
>       OUTPUT_VARIABLE GSL_VERSION )

Please use execute_process instead.  It has options to strip whitespace too.

> mark_as_advanced( GSL_FOUND GSL_LIBRARY GSL_CBLAS_LIBRARY
>   GSL_INCLUDE_DIR GSL_ROOT_DIR GSL_VERSION GSL_LIBRARY_DEBUG
>   GSL_CBLAS_LIBRARY_DEBUG GSL_USE_PKGCONFIG GSL_CONFIG CACHE )

There is no CACHE option to mark_as_advanced.  GSL_FOUND should not
be cached so it does not need to be marked.

>       set_target_properties( gsl::gslcblas PROPERTIES
>         IMPORTED_LOCATION                 "${GSL_CBLAS_LIBRARY_DLL}"
>         IMPORTED_IMPLIB                   "${GSL_CBLAS_LIBRARY}"
>         INTERFACE_INCLUDE_DIRECTORIES     "${GSL_INCLUDE_DIR}"
>         IMPORTED_LINK_INTERFACE_LANGUAGES "C" )
>       set_target_properties( gsl::gsl PROPERTIES
>         IMPORTED_LOCATION                 "${GSL_LIBRARY_DLL}"
>         IMPORTED_IMPLIB                   "${GSL_LIBRARY}"
>         INTERFACE_INCLUDE_DIRECTORIES     "${GSL_INCLUDE_DIR}" 
>         IMPORTED_LINK_INTERFACE_LANGUAGES "C"
>         IMPORTED_LINK_INTERFACE_LIBRARIES gsl::gslcblas )

See CMP0022 and use INTERFACE_LINK_LIBRARIES instead of
IMPORTED_LINK_INTERFACE_LIBRARIES.

>   if( TARGET_SUPPORTS_SHARED_LIBS )

This is not a variable, but a global property that needs to be read
with get_property.  Also there is not much reason not to just use
UNKNOWN all the time.

>     set_target_properties( gsl::gslcblas PROPERTIES
>       IMPORTED_LOCATION                 "${GSL_CBLAS_LIBRARY}"
>       INTERFACE_INCLUDE_DIRECTORIES     "${GSL_INCLUDE_DIR}"
>       IMPORTED_LINK_INTERFACE_LANGUAGES "C" )
>     set_target_properties( gsl::gsl PROPERTIES
>       IMPORTED_LOCATION                 "${GSL_LIBRARY}"
>       INTERFACE_INCLUDE_DIRECTORIES     "${GSL_INCLUDE_DIR}" 
>       IMPORTED_LINK_INTERFACE_LANGUAGES "C"
>       IMPORTED_LINK_INTERFACE_LIBRARIES gsl::gslcblas )

See above comment about CMP0022.

> include( FeatureSummary )
> set_package_properties( GSL PROPERTIES
>   DESCRIPTION "Gnu Scientific Library"
>   URL "www.gnu.org/software/gsl"
>   PURPOSE "The GNU Scientific Library (GSL) is a numerical library for C and C++ programmers."
>   )

The FeatureSummary module is for applications, not find modules.

Thanks,
-Brad