[PATCH] bug 174806

[Alexander Neundorf]

On Wednesday 25 March 2009, Michael Witten wrote:
> Hello again!
>
> I've updated the page for the following bug:
>
>     https://bugs.kde.org/show_bug.cgi?id=174806
>
> with this attachment:
>
>     https://bugs.kde.org/attachment.cgi?id=32379
>
> and this comment:
>
> Sorry for getting back WAY too late on this one. I've got excuses, but
> who cares... ;-)
>
> I've attached a tarball that contains 3 pataches: kdelibs.patch,
> kdebase.patch, kdebindings.patch.
>
> The main patch is kdelibs.patch, which cleans up some cruft and adds 3
> new CMake commands: PARSE_ARGS(), ADD_PYTHON(), and INSTALL_PYTHON().
>
> If I may boast, these are very cool and they do The Right Thing.
>
> ADD_PYTHON() creates a target for compiling python modules (as well as
> whole packages) and INSTALL_PYTHON() allows for easily installing the
> results. Whereas before, there was just one always-out-of-date target
> to which all compilation commands (everywhere) were appended, it is
> now possible to create any number of named targets with outputs that
> only need to be rebuilt when necessary. More interesting is that the
> handling of packages and the preservation of relative paths might
> allow for a more pythonic organization of the python source code. In
> short, It's Awesome.

Generic comments: 
* make sure that everything works with cmake 2.6.2, since this is the required 
CMake version for building KDE 4.2/4.3.

* if you are using execute_process() and you use only "OUTPUT_VARIABLE", then 
the error output of the process will be visible during the cmake run. In 
general this is not wanted. So if you are not interested in the error output, 
please use ERROR_QUIET, otherwise capture it usinmg ERROR_VARIABLE.

* else, endif: please use the full version with the arguments repeated, that's 
how it is done in all other places in KDE's cmake files.

ParseArgs.cmake looks cool ! :-)
Just a few comments:
can you please add tests for it ?
E.g. write a cmake file, which includes ParseArgs.cmake, and then calls it on 
several examples. This can be done via cmake -P
The copyright notice should be below the actual documentation, because 
otherwise only the copyright notice ends up in the generated documentation, 
but not the actual documentation.
The colons in the documentation are a bit confusing to me, they are not used 
that way in any other place in the cmake documentation.

FindPyKDE4.cmake:
PYKDE4_INSTALL_PYTHON_FILES(): why doesn't it byte-compile the files anymore ?

FindPythonLibrary.cmake:
the variables found via FIND_FILE/PATH/LIBRARY/PROGRAM() should be marked as 
advanced so they don't clutter up the "simple view" of ccmake/cmake-gui

You have a big IF(DEFINED PYTHON_LIBRARY) in the file.
Doesn't this exclude all the other code from being executed when it runs the 
second time ? This would mean that e.g. PYTHON_SITE_PACKAGES_DIR etc. are 
empty in this case. It is probably a good idea either to not exclude that 
part of the file from being executed, or use the cache. I.e. store these 
variables in the cache and run execute_process() only if these variables are 
not false/empty/NOTFOUND.

Please add a bit documentation on how PYTHON_SITE_PACKAGES_INSTALL_DIR is 
determined.

PythonMacros.cmake:
The macro PYTHON_INSTALL() must not be removed, since we shipped it at least 
with 4.2, so removing it is a source incompatible change.

And please use the prefix "PYTHON_" for all macros/functions/variables in this 
file.

The "SEE BELOW for more details." doesn't help.
The comments at the top of the file will end up in the generated documentation 
(up to the first empty line), so there will be no "below" ;-)

Looking forward to your improved patches :-)

Alex