FindRuby.cmake

[Alexander Neundorf]

Hi,

On Tuesday 15 March 2011, Michael Jansen wrote:
> On Monday 14 March 2011 22:23:54 Alexander Neundorf wrote:
> > On Monday 14 March 2011, Alexander Neundorf wrote:
> > > On Monday 14 March 2011, Michael Jansen wrote:
> >
> > ...
> >
> > > 0002-Reorder-stuff-in-the-hope-it-is-more-readable-and-un.patch
> > > I don't think this patch improves readability.
> > >
> > > 0003-More-commenting.patch
> > > IMHO this one doesn't really improve the comments.
> > >
> > > -# if ruby > 1.8 is required or if ruby > 1.8 was found, search for the
> > > config.h dir +#
> > > +### FIND THE CONFIG.H FILE (IF > RUBY 1.8)
> > > +#
> > > IMO not better than before.
> > >
> > > +#
> > > +### CALL FindPackageHandleStandardArgs
> > > +#
> > >
> > >  INCLUDE(FindPackageHandleStandardArgs)
> > >  SET(_RUBY_REQUIRED_VARS RUBY_EXECUTABLE RUBY_INCLUDE_DIR RUBY_LIBRARY)
> > >
> > > This comment doesn't add any information.
> > >
> > > And why are they all UPPERCASE ?
> >
> > Or to put it in other words: I kind-of maintain (i.e. fix bugs from time
> > to time) the FindRuby.cmake in CMake, and I don't feel like merging these
> > two patches into cmake, since they do not really improve the file IMO.
>
> The rational is that the script is more or less a about 200 whatever
> listing of cmake commands with some comments sprinkled inbetween. That what
> i call spaghetti code and refactor. In every language. In C++ i would
> factor it out into small helper functions with a description and one duty.
> In cmake i can't. So i try to put comments into the file that are easily
> visible distuingishable from code that explain the steps and try to
> minimize cross dependencies between the blocks.
>
> I for one did only notice that RUBY_ARCH is not cached after that commits.
> I was only able to see at a glimpse which variable belongs to which
> installation are after those patches. And what other vars are set.

How to put it.
The structure currently is:
IF(RUBY_EXECUTABLE  AND NOT  RUBY_MAJOR_VERSION)
 * call ruby a few times to get some variables
 * put those variables into the cache
 * mark them as advanced
ENDIF()

and IMO this is quite obvious.
Doesn't look too spaghetti to me.

> It is your decision but i would like to assure that i would have never
> finished the task without them. I btw. do not care the slightest bit about
> upper/lowercase or wording.  If you can improve anything feel free. The

> FindPackageHandleStandardArgs comment is there because i have no better
> explanation what this statement and the surrounding code does.

I consider a comment which doesn't contain information (as in "additional 
information") useless at best.
I'm all for comments which improve the documentation.
Your patch only changes the comments, but to me it's really not obvious that 
it actually *improves* the commenting.

> Because i do not know what it does.

Here goes the documentation from the man page:
FindPackageHandleStandardArgs: 
 
FIND_PACKAGE_HANDLE_STANDARD_ARGS(<name> ... )
This function is intended to be used in FindXXX.cmake modules files. It 
handles the REQUIRED, QUIET and version-related arguments to FIND_PACKAGE(). 
It also sets the <UPPERCASED_NAME>_FOUND variable. The package is considered 
found if all variables <var1>... listed contain valid results, e.g. valid 
filepaths.
There are two modes of this function. The first argument in both modes is the 
name of the Find-module where it is called (in original casing).
The first simple mode looks like this:
    FIND_PACKAGE_HANDLE_STANDARD_ARGS(<name> (DEFAULT_MSG|"Custom failure 
message") <var1>...<varN> )
If the variables <var1> to <varN> are all valid, then <UPPERCASED_NAME>_FOUND 
will be set to TRUE. If DEFAULT_MSG is given as second argument, then the 
function will generate itself useful success and error messages. You can also 
supply a custom error message for the failure case. This is not recommended.
The second mode is more powerful and also supports version checking:
    FIND_PACKAGE_HANDLE_STANDARD_ARGS(NAME [REQUIRED_VARS <var1>...<varN>]
                                           [VERSION_VAR   <versionvar>
                                           [CONFIG_MODE]
                                           [FAIL_MESSAGE "Custom failure 
message"] )

As above, if <var1> through <varN> are all valid, <UPPERCASED_NAME>_FOUND will 
be set to TRUE. After REQUIRED_VARS the variables which are required for this 
package are listed. Following VERSION_VAR the name of the variable can be 
specified which holds the version of the package which has been found. If 
this is done, this version will be checked against the (potentially) 
specified required version used in the find_package() call. The EXACT keyword 
is also handled. The default messages include information about the required 
version and the version which has been actually found, both if the version is 
ok or not. Use the option CONFIG_MODE if your FindXXX.cmake module is a 
wrapper for a find_package(... NO_MODULE) call, in this case all the 
information provided by the config-mode of find_package() will be evaluated 
automatically. Via FAIL_MESSAGE a custom failure message can be specified, if 
this is not used, the default message will be displayed.
Example for mode 1:
    FIND_PACKAGE_HANDLE_STANDARD_ARGS(LibXml2  DEFAULT_MSG  LIBXML2_LIBRARY 
LIBXML2_INCLUDE_DIR)

LibXml2 is considered to be found, if both LIBXML2_LIBRARY and 
LIBXML2_INCLUDE_DIR are valid. Then also LIBXML2_FOUND is set to TRUE. If it 
is not found and REQUIRED was used, it fails with FATAL_ERROR, independent 
whether QUIET was used or not. If it is found, success will be reported, 
including the content of <var1>. On repeated Cmake runs, the same message 
won't be printed again.
Example for mode 2:
    FIND_PACKAGE_HANDLE_STANDARD_ARGS(BISON  REQUIRED_VARS BISON_EXECUTABLE
                                             VERSION_VAR BISON_VERSION)
In this case, BISON is considered to be found if the variable(s) listed after 
REQUIRED_VAR are all valid, i.e. BISON_EXECUTABLE in this case. Also the 
version of BISON will be checked by using the version contained in 
BISON_VERSION. Since no FAIL_MESSAGE is given, the default messages will be 
printed.
Another example for mode 2:
    FIND_PACKAGE(Automoc4 QUIET NO_MODULE HINTS /opt/automoc4)
    FIND_PACKAGE_HANDLE_STANDARD_ARGS(Automoc4  CONFIG_MODE)
In this case, FindAutmoc4.cmake wraps a call to FIND_PACKAGE(Automoc4 
NO_MODULE) and adds an additional search directory for automoc4. The 
following FIND_PACKAGE_HANDLE_STANDARD_ARGS() call produces a proper 
success/error message.

> In the end it is your call. But always remember that not everyone
> understands 250 lines of cmake code without any visible grouping or
> explanation what each part does.

FindRuby.cmake as it is in cmake is not unreadable, and your patches 2 and 3 
don't improve it, so I object to these two patches.

The actual logic for determining the install directories looks fine.

Alex