FindR.cmake needs some work
Alexander Rieder
alexanderrieder at gmail.com
Sun May 2 16:04:34 CEST 2010
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:FindPackageH
> > > 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.
Alexander
-------------- next part --------------
A non-text attachment was scrubbed...
Name: improve_findr.diff
Type: text/x-patch
Size: 10082 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/kde-buildsystem/attachments/20100502/27033424/attachment-0001.diff
More information about the Kde-buildsystem
mailing list