2009/4/12 Thiago Macieira <thiago@kde.org>:
> Michael Witten wrote:
>> Did nobody read my patches?
>
> I have done that now.
>
>> I have been too busy to to clean up the asthaetic issues that have
>> kept it out of trunk, but I honestly believe that my solution is the
>> fullest and most superior suggestion (and implementation) yet.
>
> You're right, your solution is by far the most complete one.
>
> However, there are still problems.
>
> If a user ran cmake with a path that doesn't exist *yet*, it'll still try
> to install to the system prefix. This is a reasonable use-case for the first
> time you try to build KDE. Not to mention the surprise factor of that
> changing in the second run.

The algorithm for determining writability is actually more general than that,
and will not fail in this way.

In kdelibs/cmake/modules/FindLibPython.py:

    def writable(path):
        while True:
            if access(path, F_OK):            # Exists?
                return access(path, W_OK)     # Is Writable?
            else:                             # Check ancestors
                ancestor = dirname(path)
                if ancestor == path:
                    return False              # No more ancestors
                path = ancestor               # Check ancestor

    ...

    if writable(install_prefix):
        if writable(system_site):
            w(system_site)
        else:
            w(user_site_packages_dir())
    else:
        w(system_site)

The writable() function recurses back down the filesystem hiearchy to
determine if the path is ULTIMATELY writable (as noted in the comments).
That is, if the path doesn't yet exist, then some ancestor is used
to determine whether the current user will even be able to create the
final path.

See? Didn't I do an awesome job? :-D

> Also, if root is installing to /opt/kde4, the files will by default end up
> in /usr/lib/python2.6, which is outside the prefix.

That's the way it has always been already; python dictates that user-importable
modules should be placed in site-packages directories.

> I repeat once again: unless the user tells us so in the command-line, do
> not install anything outside the prefix.

I understand this sentiment, and I'm inclined to agree with it, but setting
PYTHONPATH is not really a suitable answer, because it is searched before
the default paths, which could be problematic, in general.

Also, as an end-user I haven't had to set any variables explicitly. Now you
want me to set PYTHONPATH all the time? No way!

The only people, who really care, are the developers. If they have 2.6, it's
handled transparently for probably (100% - epsilon) of the cases out there.
If they have < 2.6, then it prompts them to specify on the command line where
to install things and tells them to use PYTHONPATH.

Your solution is a subset of my solution.

In any case, not all of the python modules are currently stored in site-packages,
and more could probably be moved out. However, there needs to be at least some
top-level modules in site-packages to setup up sys.path properly for finding the
others, etc.

> As for the cosmetic changes, it's necessary to address them, since there
> are a lot of them, which means reviewing your patches is hard.

Not really;

    http://mail.kde.org/pipermail/kde-buildsystem/2009-March/005714.html

Basically:

    * UNSET has to be changed to SET (for cmake 2.6.2 compatibility),
    * ERROR_QUIET for EXECUTE_PROCESS()
    * Repeated args for ELSE, ENDIF, ENDFOR, ENDWHILE, etc. (This is
      a huge mistake in my opinion. I think the entire codebase should
      have that nonsense filtered out; I will one day write a Perl
      script to do exactly that.
    * Add tests for ParseArgs.cmake.
    * Put documentation at the top of the file, rather than in-file.
    * Remove the colons from the documentation (I assume before code
      examples, but it's weird that this would be so distracting; I'm
      not entirely moved to remove them, as I find it more readable).
    * Mark some variables as ADVANCED; this was frankly a problem before
      my patches, I believe (though I probably added to it).
    * Make FindPythonLibrary.cmake handle an existing CMakeCache.txt
      better. This is actually not my fault; in fact, I made what was there
      work better.
    * Write more documentation on how PYTHON_SITE_PACKAGES_INSTALL_DIR is
      chosen; I thought I already had, but I can just copy the comments
      from FindLibPython.py.
    * PYTHON_INSTALL must be put back, because others might depend on it;
      I agree, but I'll put a warning that it's deprecated. I will also
      change the behavior, because it doesn't make sense to precompile
      python modules that are not installed as *versioned* site-packages;
      this is also why PYKDE4_INSTALL_PYTHON_FILES() no longer byte-compiles.
    * Mangle the cmake-compatible naming in order to namespace.

This is all straightforward stuff and in no way makes the patches harder to review.
In fact, I think I'll try to do this by at least the end of Tuesday, so that we can
get these patches in trunk and kill this trouble.

> example, there's a new file called ParseArgs.cmake which adds a macro that
> is not used anywhere.

It is used in PythonMacros.cmake's INSTALL_PYTHON, and it was written with the
intention of being used more.

Sincerely,
Michael Witten

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