[PATCH] bug 174806

[Michael Witten]

2009/4/12 Thiago Macieira <thiago at kde.org>:
> Michael Witten wrote:
>>> 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.
>
> Sorry, it has to be. If you don't want to set PYTHONPATH, then you should
> pass the argument to cmake and install as root.
>
> Like I said: configure --prefix=/usr --sysconfigdir=/etc

I agree with this policy as a matter of theory. However, I think
installing to a known site-packages is much more practical; after all,
if you really want things in a nonstandard location (from python's
view) like the KDE prefix, you're free to use
-DPYTHON_SITE_PACKAGES_INSTALL_DIR=...

By default, I suggest it's better to work with python rather than make
python work with KDE; at least it would be useful to provide titular
modules that can find the real stuff in the KDE prefix.

Of course, the problem with using site-packages is that an upgrade of
python will require PYTHONPATH anyway to point to the old
site-packages (unless people are smart enough to compile python to
look there too), or the KDE python bindings will need to be
reinstalled for the new site-packages. However, it makes sense that an
extraordinary upgrade should require extraordinary measures.

In any case, I conceed. I don't really care where these things are
installed or what variables must be setup. In fact, the more I think
about it, I don't even mind if .py files are byte-compiled to .pyc
outside of versioned site-packages, because they will be ignored if
they aren't the right version (though I would suggest KDE alert the
user of the need to run python's compileall with privileges [or some
other setuid tool KDE could provide]).

>>> 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.
>
>>    * 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.
>
> Not your decision to make. While it's mandated, you follow it. But that's
> not my point.

I care more about progress than I do about worshipping bad policies.
But that's not my point.

> How about the changes to regexp, changing \\. to [.] ? That's unnecessary.
> I'm not saying you shouldn't do it, but do it in a separate patch... you're trying
> to do too much in one single patch. Please split it into multiple patches, each
> one doing one specific thing.

Your request is completely reasonable and is aligned with my modus operandi.

However, your request is unreasonable until KDE starts using a real
version control system like git. In my case, I wanted to look into a
completely different issue, so I checked out the svn trunk and got
stalled on this bug; I hadn't intended to provide any extensive
patches until I was steeped in this problem, so I never bothered to
convert the working directory to a sane system.

My patches are pretty clean. As for your example of patch noise, I had
already rewritten 70% of the file, so I decided to make the rest of
that one file match with my new code by capitalizing the cmake
commands (this also matches most KDE cmake code and as well as the
cmake code used for cmake itself). Because I had already modified the
line, why not make the regular expression a little more sane too?
Should I have done this in a separate patch? I would have, if I hadn't
been tricked into using something as subpar as svn.

>>> 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.
>
> It's not in the patch you attached.

You must be looking at the wrong patch; though, you missed the python
code too...

Sincerely,
Michael Witten