FindR.cmake needs some work

[Alexander Neundorf]

On Saturday 01 May 2010, Alexander Rieder wrote:
> On Saturday 01 May 2010 21:03:01 you wrote:
> > 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:FindPackageHan
> >dl eStandardArgs
> >
> > 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.
>
> Hi,
> thanks for your explanations.
> Here's another try of cleaning it up. Again it seems to detect R correctly,
> and Cantor compiles with it.
> Please have a look and tell me any errors I made/things I missed.

We're almost there.

1) MARK_AS_ADVANCED(R_INCLUDE_DIR, R_R_LIBRARY, R_LAPACK_LIBRARY,
 R_BLAS_LIBRARY)

The commas are wrong, just remove them. The arguments are separated simply by 
the spaces.

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

This must be at the end of the file, e.g. directly before the 
mark_as_advanced(), because at the top, where they are now, R_EXECUTABLE, 
R_INCLUDE_DIR and R_R_LIBRARY are empty (at least on the first run, on 
following runs they are probably already in the cache at that point).

3)
ELSE( R_EXECUTABLE)
  MESSAGE(STATUS "Could NOT find R executable")

I think this is not necessary, find_package_handle_standard_args() should 
already report that.

Beside that, I think it looks good :-)

Alex