Review Request: Attica will not be found if installed under /usr/local/

[Alexander Neundorf]

On Wednesday 22 September 2010, Guy Maurel wrote:
> > On 2010-09-17 18:52:17, Alexander Neundorf wrote:
> > > /usr/local is part of the default search directories, so it should be
> > > found if installed there. I just tested this here locally and it works
> > > for me.
> > >
> > > If it is not found when installed in /usr/local/, then we have to check
> > > what is going wrong there.
> > >
> > > Alex
> >
> > Guy Maurel wrote:
> >     mmmh!
> >     I found at http://www.kitware.com/news/home/browse/168:
> >     "FIND_LIBRARY and FIND_PATH now look in CMAKE_LIBRARY_PATH and
> > CMAKE_INCLUDE_PATH environment variables in addition to and before the
> > PATH environment variable. " It is what I need, but nothing about a
> > default value for CMAKE_LIBRARY_PATH and CMAKE_INCLUDE_PATH Is it
> > something the user *has* to do itself before running?
> >
> > Guy Maurel wrote:
> >     Thanks to Alex for the discussion and your questions/helps.
> >
> >     The CMAKE_INCLUDE_PATH give the order to search for:
> >     *First* at /usr/local
> >     *then*  at /usr
> >     This is done (I think) to give a priority to the packages one have
> > self installed.
> >
> >     At the FindLibAttica.cmake (and other more) there is some lines of
> > code such: IF (NOT WIN32)
> >        # use pkg-config to get the directories and then use these values
> >        # in the FIND_PATH() and FIND_LIBRARY() calls
> >        FIND_PACKAGE(PkgConfig)
> >        PKG_CHECK_MODULES(PC_LIBATTICA QUIET libattica)
> >        SET(LIBATTICA_DEFINITIONS ${PC_ATTICA_CFLAGS_OTHER})
> >     ENDIF (NOT WIN32)
> >
> >     I am using ArchLinux and the pacman-command installs (implicitly)
> > some packages I havn't take care. One of them was "attica", installed at
> > /usr . The search order is changed. The cmake works correctly with
> > FindLibAttica.cmake and told me: /usr/include .
> >
> >     *NOW* I would like to ask: What is the reason/strategy of changing
> > the search order in this way?
> >
> >     *MY* problem is now solved.
> >     Thanks
>
> I got the "problem" because I don't anderstand the message, generate bei
> ccmake: "* libattica (0.1.90 or higher)..."
>
> I propose to do the same way as FindPolkitQt-1.cmake to improve the message
> with a warning such as: "WARNING: LibAttica version 0.1.4 found at
> /usr/include is too old. Please install 0.1.90 or newer." this way:
> --- FindLibAttica.cmake (revision 1175580)
> +++ FindLibAttica.cmake (working copy)
> @@ -37,6 +37,7 @@
>      STRING(REGEX REPLACE "LIBATTICA_VERSION_STRING \"(.*)\"\n" "\\1"
> LIBATTICA_VERSION ${LIBATTICA_VERSION_MATCH}) IF(LIBATTICA_VERSION STRLESS
> "${LIBATTICA_MIN_VERSION}")
>        SET(LIBATTICA_VERSION_OK FALSE)
> +      MESSAGE("WARNING: LibAttica version ${LIBATTICA_VERSION} found at
> ${LIBATTICA_INCLUDE_DIR} is too old. Please install
> ${LIBATTICA_MIN_VERSION} or newer.") IF(LibAttica_FIND_REQUIRED)
>          MESSAGE(FATAL_ERROR "LibAttica version ${LIBATTICA_VERSION} is too
> old. Please install ${LIBATTICA_MIN_VERSION} or newer.")
> ELSE(LibAttica_FIND_REQUIRED)

No, please don't do it this way.
We have FindPackageHandleStandardArgs.cmake in kdelibs from current cmake, 
please have a look at its documentation.
It should be possible to remove the whole version checking chunk and just do 
something like this:

find_package_handle_standard_args(LibAttica 
                 REQUIRED_VARS LIBATTICA_LIBRARIES LIBATTICA_INCLUDE_DIR
                 VERSION_VAR LIBATTICA_VERSION_MATCH)

This should produce a much better error message without needing any special 
code.

> The next improvment I make soon would be
> IF(LIBATTICA_VERSION_OK)
>   # search for libraries
> ELSE
>   # do nothing
> ENDIF

I think this just complicates the code for no real gain.
The find_xxx() commands anyway only do something if the given variable is 
still empty or NOTFOUND, i.e. once LibAttica has been found they don't search 
again.

You could check wether pkg_check_modules() caches its results and if not, add 
that there.

Alex