FindR.cmake needs some work

[Alexander Rieder]

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:FindPackageHandl
> 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.

Alexander
-------------- next part --------------
A non-text attachment was scrubbed...
Name: improve_findr.diff
Type: text/x-patch
Size: 10157 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/kde-buildsystem/attachments/20100501/288c35cf/attachment-0001.diff