On Sunday 15 March 2009, Allen Winter wrote:
> Howdy,
>
> I'd like to get a review of the attached patch to improve Python discovery.
>
> This is Michael Witten's patch posted to
> https://bugs.kde.org/show_bug.cgi?id=174806. I'm just the messenger.
>
> Also, this patch provides a new $PYTHON_SITE_PACKAGES_INSTALL_DIR
> where python packages can be installed (i.e. in a non-root-owned location,
> if desired).

Just some comments:

+# PYTHON_SITE_PACKAGES_INSTALL_DIR - Multiplexer between 
PYTHON_{,USER_}SITE_PACKAGES_DIR

This doesn't really explain what it does.

+    # What seemed like error checking was worthless, so it has been removed;
+    # In fact, error checking is so nontrivial in these cases, that it has
+    # mostly not been attempted.

I think comments about something which was here earlier are not useful, this 
should just be in the commit message.

+    if(PYTHON_SITE_PACKAGES_INSTALL_DIR)
+                # This variable may be supplied by the user.
+      message( "User supplied ...

This should be "message(STATUS ...")

+    elseif(NOT CMAKE_INSTALL_PREFIX MATCHES "^$ENV{HOME}")
+      set(PYTHON_SITE_PACKAGES_INSTALL_DIR ${PYTHON_SITE_PACKAGES_DIR})      
...
+    elseif (PYTHON_USER_SITE_PACKAGES_DIR)
+      set(PYTHON_SITE_PACKAGES_INSTALL_DIR ${PYTHON_USER_SITE_PACKAGES_DIR}) 
...

This is quite some magic, changing the python install destination depending on 
whether CMAKE_INSTALL_PREFIX points inside $HOME or not. Not sure I like 
that.

+    else(PYTHON_SITE_PACKAGES_INSTALL_DIR)
+      message(SEND_ERROR

What SEND_ERROR and not FATAL_ERROR ?
I'd prefer FATAL_ERROR, because this aborts immediately, so it's easier to see 
why the cmake run aborted (compared to scrolling up a few pages and looking 
for the error).

+"You have selected a CMAKE_INSTALL_PREFIX that is under \${HOME}. Usually
+this means you are a KDE developer, 

This is too much guessing, and FindPythonLibrary.cmake is usable independent 
from KDE, so a message which says that you are probably a KDE developer is 
out of place here.

Alex
_______________________________________________
Kde-buildsystem mailing list
Kde-buildsystem@kde.org
https://mail.kde.org/mailman/listinfo/kde-buildsystem