FindR.cmake needs some work

[Alexander Neundorf]

On Saturday 01 May 2010, Alexander Rieder wrote:
> On Tuesday 27 April 2010 21:13:38 you wrote:
> > Hi Alex,
> >
> > somebody reported a problem with FindR.cmake, so I had a look at it.
> > It has some issues and needs some work.
> > Can you please have a look at them ?
> > I'll happily provide guidance on how to fix them if you have questions.
> >
> > So here we go (numbered so it's easier to refer to them):
> >
> > 1) The check IF(R_EXECUTABLE-NOTFOUND) is wrong.
> > I guess you want to check whether R_EXECUTABLE has been set to something
> > useful. To do this, do IF(R_EXECUTABLE).
> >
> > 2) A big part of the code below uses R_EXECUTABLE and is executed also if
> > R_EXECUTABLE has not been found. So all that code below should be put
> > into a IF(R_EXECUTABLE)
> >
> > 3) Is there a reason why you do
> > FIND_FILE(R_H
> > 	R.h
> > 	PATHS ${R_INCLUDEDIR}
> > 	NO_DEFAULT_PATH)
> > ...
> > 	GET_FILENAME_COMPONENT(R_INCLUDEDIR ${R_H}
> > 				PATH)
> >
> > instead of
> > FIND_PATH(R_INCLUDEDIR R.h PATHS ${R_INCLUDEDIR} NO_DEFAULT_PATH)
> > ?
> >
> > This should do the same and save code, i.e. make the file easier to read.
> >
> > 4) Why do you use the NO_DEFAULT_PATH as last argument ? Is this to make
> > sure that cmake first looks in R_INCLUDEDIR ?
> > A better way to do this is to use the "HINTS" keyword instead the "PATHS"
> > keyword. The directories listed after HINTS are searched before the
> > default directories, not after them. So then there should be no reason to
> > use NO_DEFAULT_PATH anymore.
> >
> > 5) The documentation at the top says that R_INCLUDE_DIR is provided by
> > the file, but it seems that instead R_INCLUDEDIR is set. This should be
> > changed so that indeed R_INCLUDE_DIR is provided.
> >
> > 6) R_USED_LIBS should be named R_LIBRARIES.
> >
> > 7) All the other library variables should be named "R_FOO_LIBRARY", e.g.
> > R_R_LIBRARY, R_LAPACK_LIBRARY etc.
> >
> > 8) All variables which hold the result of a
> > find_file/path/library/program() call should be marked as advanced at the
> > bottom of the file (using MARK_AS_ADVANCED() ) The only exception may be
> > R_EXECUTABLE itself.
> >
> > 9) To check whether FindR.cmake was successful, you should use the macro
> > find_package_handle_standard_args, provided by
> > FindPackageHandleStandardArgs.cmake, something like
> > include(FindPackageHandleStandardArgs)
> > find_package_handle_standard_args(R  DEFAULT_MSG
> >                                      R_EXECUTABLE R_INCLUDE_DIR
> > R_R_LIBRARY)
> >
> >
> > Alex
>
> Hi,
> it took me a bit longer than I thought, but I had a look at the
> findR.cmake, and improved it according to your suggestions. (And I again
> got reminded that I really suck at cmake). Could you please look at the
> attached patch, see if it is actually better, and if it fixes all you
> wanted? It correctly detects R on my machine, and cantor compiles.

Much better already :-)

Still some comments:
1) please decide whether you want to use spaces or tabs indentation. 
FindR.cmake is mixed now (mainly spaces, some tabs left)

2) FIND_PATH(R_INCLUDE_DIR R.h PATHS ${R_INCLUDE_DIR}) 

You can simply do 
FIND_PATH(R_INCLUDE_DIR R.h)
This command will only actually do something at all if R_INCLUDE_DIR doesn't 
already have a valid value (i.e. only if R_INCLUDE_DIR is empty at this point 
or contains "NOTFOUND"). So if it already has a good value, FIND_PATH() 
shouldn't do anything at all anyway.

3)  FIND_LIBRARY(R_R_LIBRARY
    R
    PATHS ${R_HOME}/lib ${R_SHARED_LIB_DIR} ${R_HOME}/bin
    NO_DEFAULT_PATH)

I'd suggest you remove the "NO_DEFAULT_PATH" and replace "PATHS" with "HINTS". 
This will have the effect that the given directories will be searched first. 
This should be what was originally intended I think.
Similar for the other libraries.

4)
  FIND_LIBRARY(R_LAPACK_LIBRARY
    Rlapack
    PATHS ${R_SHARED_LIB_DIR}
    NO_DEFAULT_PATH)
  IF(NOT R_LAPACK_LIBRARY)
    #MESSAGE(STATUS "No, it does not exist in ${R_SHARED_LIB_DIR}")
  ELSE(NOT R_LAPACK_LIBRARY)
    #MESSAGE(STATUS "Yes, ${R_LAPACK_LIBRARY} exists")
    SET(R_LIBRARIES ${R_LIBRARIES} Rlapack)


This last line is wrong. At this point, find_library() has put the full path 
to the Rlapack library into the R_LAPACK_LIBRARY variable, 
e.g. "/usr/lib/libRlapack.so".
This should be added to R_LIBRARIES instead of just "Rlapack".
SET(R_LIBRARIES ${R_LIBRARIES} ${R_LAPACK_LIBRARY} )

This way also later on it will not be necessary to call LINK_DIRECTORIES() and 
all the messing around with R_LIBDIR.

5) If you find all the libraries using find_library() with their full path, 
and add them with their full path to R_LIBRARIES, I think all that R_LIBDIR 
code can simply be removed. The link directory should then not be necessary 
anymore (since all libraries are given with their full path).


> As for your last suggestion I don't really know where/for what to use this
> macro (is it inside findR or inside Cantor?), and I couldn't find any
> example for its use grepping through KDE. Could you give me an example on
> this?

It's used a lot in the Find*.cmake files in kdelibs/cmake/modules/.
It's also documented, just see the cmake man page, which is also available 
online:
http://www.cmake.org/cmake/help/cmake-2-8-docs.html#module:FindPackageHandleStandardArgs

Short version: the macro basically takes a number of variables as arguments, 
checks whether they are all valid, and if so, sets the <package>_FOUND 
variable to TRUE (and prints some status message).
So you probably want something like the following:

include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(R  DEFAULT_MSG 
                                  R_EXECUTABLE R_INCLUDE_DIR R_R_LIBRARY)

This will set R_FOUND to TRUE if these three things have been found 
(R_EXECUTABLE R_INCLUDE_DIR R_R_LIBRARY). The macro also handles the QUIET 
and REQUIRED keywords of find_package().

Alex

P.S. please keep kde-buildsystem on CC, so others can read that too for later 
reference.