FindR.cmake needs some work

[Alexander Neundorf]

On Tuesday 27 April 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)
>
> Hi,
> thanks for looking at this. The answer to most of your questions is simply
> "because I blindly copied it from the RKWard project".

If that's the case, maybe you can report your changes back to RKward once 
we're through ? 

> I will look at the issues you pointed out tomorrow.

Cool :-)
Alex