FindR.cmake needs some work
Alexander Neundorf
neundorf at kde.org
Sun May 2 18:32:16 CEST 2010
On Sunday 02 May 2010, Alexander Rieder wrote:
> On Sunday 02 May 2010 14:34:57 you wrote:
> > 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:FindPackag
> > > >eH an
> > > >
> > > >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
>
> Ok, next try. Hopefully this will be the last one.
> please have a look.
If the rest of the file is all-uppercase, then please also use all-uppercase
for the find_package_handle_standard_args() lines.
Also, I think these lines:
IF( R_HOME AND R_INCLUDE_DIR AND R_R_LIBRARY AND R_SHARED_LIB_DIR )
MESSAGE(STATUS "Found R at ${R_HOME}: using libraries ${R_LIBRARIES}")
SET(R_FOUND TRUE)
ELSE ( R_HOME AND R_INCLUDE_DIR AND R_R_LIBRARY AND R_SHARED_LIB_DIR )
MESSAGE(STATUS "Could NOT find R ")
ENDIF( R_HOME AND R_INCLUDE_DIR AND R_R_LIBRARY AND R_SHARED_LIB_DIR )
can also just be removed, find_package_handle_standard_args() should print
some output.
Beside that, ok to commit.
Alex
More information about the Kde-buildsystem
mailing list