[PATCH] bug 174806

[Thiago Macieira]

Michael Witten wrote:
>2009/4/12 Thiago Macieira <thiago at kde.org>:
>> 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.
[snip]
>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.

I see. I hadn't seen that part of the patch.

>> 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.

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

>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!

Yes.

You already have to set a few variables, so what's one more?

	PATH
	KDEDIRS
	XDG_CONFIG_DIRS
	XDG_DATA_DIRS
	PKG_CONFIG_PATH
	LD_LIBRARY_PATH
	etc.

Note that, for some of those, KDE takes care of setting them for you or 
suitably changing the defaults behind the scenes. But it's also a surprise 
factor: since XDG_DATA_DIRS defaults are changed without notice, non-KDE 
applications don't see it and will behave unpredictably.

>> 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.

>    * Put documentation at the top of the file, rather than in-file.

Do it in a separate patch. Really.

>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.

Unfortunately it does. I'm trying to understand what the behaviour will be 
and I have to understand first if the change in question is related to 
where the files will be installed or not.

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.

>> 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.

Which reinforces the point: you're trying to do too much in one single 
patch. Please split it into multiple patches, each one doing one specific 
thing.

-- 
  Thiago Macieira  -  thiago (AT) macieira.info - thiago (AT) kde.org
    PGP/GPG: 0x6EF45358; fingerprint:
    E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/kde-buildsystem/attachments/20090412/e68efab0/attachment.sig