KDevelop: support for installing into a non-standard parallel prefix
ClosedPublic

Authored by rjvbb on Nov 30 2018, 11:38 AM.

Details

Summary

This is a proper patch containing the changes I have been making to make KDevelop behave better in a situation where it is installed into a non-standard parallel prefix. In my case this concerns /opt/local as used by MacPorts but I think the changes could be useful elsewhere too.

The patch adds a preference for the cmake utility and the manpages from the install prefix, and replaces the hardcoded python interpreter specification with the more usual /usr/bin/env python line.

Test Plan

Has proven to work as intended over the last few years, on Mac and Linux.

I may have overlooked places where the default standard locations (/usr and /usr/local) are computed rather than hardcoded.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
rjvbb requested review of this revision.Nov 30 2018, 11:38 AM
rjvbb created this revision.
kfunk requested changes to this revision.Nov 30 2018, 12:04 PM
kfunk added a subscriber: kfunk.
kfunk added inline comments.
plugins/cmake/cmakeutils.cpp
361–362

This does not make sense. The default install prefix *is* /usr/local.

https://cmake.org/cmake/help/v3.0/variable/CMAKE_INSTALL_PREFIX.html

plugins/qmljs/3rdparty/qtcreator-libs/qmljs/parser/changeLicense.py
1 ↗(On Diff #46546)

I think these changes (here and below, in .py files) are fine.

Please commit them separately (no further review) required. Master branch maybe, to not break the next stable release.

This revision now requires changes to proceed.Nov 30 2018, 12:04 PM
kfunk added inline comments.Nov 30 2018, 12:10 PM
plugins/cmake/cmakeutils.cpp
325

This is also cumbersome, it would make sense in case CMake is shipped (or bundled with) KDevelop, but it isn't.

333

I'd be fine with a fallback:

#elif Q_OS_MAC
if (cmake.isEmpty())
    cmake = QStandardPaths::findExecutable(QStringLiteral("cmake"), {"/opt/local/bin"}) // for CMake from MacPorts

... here, though.

flherne added inline comments.
plugins/cmake/cmakeutils.cpp
361–362

I presume the idea is that if KDevelop itself has been built locally and installed to some non-standard path, the user will want to use the same path to install their other projects.

This seems a bad idea to me; it's only helpful in very specific circumstances and will be very confusing if the behaviour isn't desired.

flherne added inline comments.Nov 30 2018, 12:18 PM
plugins/cmake/cmakeutils.cpp
361–362

On further thought -- won't this cause all distro-built versions to try to install projects under /usr rather than /usr/local? That would *really* be a bad idea.

rjvbb added inline comments.Nov 30 2018, 1:55 PM
plugins/cmake/cmakeutils.cpp
333

For MacPorts it would have to be the 1st option.

I think though that MacPorts isn't the only packaging system that installs to a parallel prefix and intends everything from there to override whatever the host already provides. Gentoo Prefix and pkgsrc come to mind.

361–362

Indeed, this is for when you maintain a software install in a parallel prefix, whether that be through the aforementioned projects like Gentoo Prefix or MacPorts, or manually. There also used to be a Project Neon5 which had the same approach.

The change doesn't change the default, but why would using '/usr' (instead of '/usr/local') be worse than using 'c:\Program Files' under MS Windows?
That said, filtering out '/usr' won't be hard.

Or maybe the default install dir should simply be empty in order not to assume anything over whatever default the local cmake installation is set to?

kfunk added inline comments.Nov 30 2018, 2:20 PM
plugins/cmake/cmakeutils.cpp
333

Then the system-wide PATH should already contain that prefix; this should not be within KDevelop's responsibility.

Further note: We'd basically need to patch every QStandardPaths::findExecutable() call in order to make this a consistent behavior for all command-line tools inside KDevelop.

361–362

Nope, it still does not make sense. defaultInstallDir here is resembling CMake's default install prefix, which is not dependent on KDevelop's install prefix. Why would it...

The change doesn't change the default, but why would using '/usr' (instead of '/usr/local') be worse than using 'c:\Program Files' under MS Windows?

Nope. It *does* change the default if KDevelop's install prefix is not /usr/local. The second point is moot. It's what CMake chose as default for these platforms.

That said, filtering out '/usr' won't be hard.

So we're back on applying hacks/work-arounds on half-baked solutions?

rjvbb added a comment.Nov 30 2018, 2:57 PM

The change doesn't change the default, but why would using '/usr' (instead of '/usr/local') be worse than using 'c:\Program Files' under MS Windows?

Nope. It *does* change the default if KDevelop's install prefix is not /usr/local.

That's also what I meant.

That said, the install location field is always empty in the cmake import wizard when I create a new project. Which is probably fine because why would KDevelop bother hardcoding a default cmake setting esp. if that setting is dubious (argue as you might, c:\Program Files *is* the equivalent of /usr and I gather from Francis' comment that /usr would not have been used here if that had been CMake's choice on Unix).

But maybe a proper cmake option to configure the default would raise less eyebrows?
If there is an issue with setting the default to /usr then /usr/local should be equally off-limits on systems where that tree is under distribution control (like FreeBSD).

rjvbb updated this revision to Diff 46601.Dec 1 2018, 11:04 AM

The python script changes were committed to the master branch as requested (db05710cb6931a7b44d7fc70bfcfe75c7ccc9f4a).

I've removed the change to the cmake executable lookup; I'll keep it as a distribution patch.

I've tested the idea of NOT specifying an install prefix default when none has been configured. To the best I can see this does not lead to invalid cmake invocations (aka -DCMAKE_INSTALL_PREFIX=). Instead, this does exactly what you'd expect: cmake will use whatever is its default for the current platform. The result will be obtained from the CMakeCache later on.
In other words, there doesn't appear to be any reason to hardcode assumptions about CMake's defaults, and not doing that should be a suitable compromise for everyone. It also removes the need for conditional code.

rjvbb set the repository for this revision to R32 KDevelop.Dec 1 2018, 11:04 AM
rjvbb marked 5 inline comments as done.
kfunk added a subscriber: apol.Dec 2 2018, 9:25 AM

I've tested the idea of NOT specifying an install prefix default when none has been configured. To the best I can see this does not lead to invalid cmake invocations (aka -DCMAKE_INSTALL_PREFIX=). Instead, this does exactly what you'd expect: cmake will use whatever is its default for the current platform. The result will be obtained from the CMakeCache later on.
In other words, there doesn't appear to be any reason to hardcode assumptions about CMake's defaults, and not doing that should be a suitable compromise for everyone. It also removes the need for conditional code.

@apol Do you think that makes sense?

plugins/manpage/manpageplugin.cpp
98 ↗(On Diff #46601)

I wonder if we just want to remove that complete if-branch altogether... Not entirely sure what it is trying to work-around.

kfunk added a comment.Dec 2 2018, 9:28 AM

Just some general remarks: Storing the application's install prefix inside the binary itself almost always smells like bad design. Thus the concerns here. You rather want to do this dependent on the kdevelop binary's application path (cf. http://doc.qt.io/qt-5/qcoreapplication.html#applicationDirPath)

Easily explained with: On Windows this will just fail (as the install prefix can be anything, as the *user* chooses where KDevelop will end up during installation).

brauch added a subscriber: brauch.Dec 2 2018, 10:06 AM

On Linux quite possibly too, I think many packaging systems install into some temporary dir and then copy the deployment out of there for archiving.

rjvbb added a comment.Dec 2 2018, 9:13 PM
On Linux quite possibly too, I think many packaging systems install into some temporary dir and then copy the deployment out of there for archiving.

That uses make install DESTDIR=/path/to/distdir; MacPorts does the same. All this does is prepending DESTDIR to PREFIX.
Using the install prefix during the build process isn't that uncommon with autoconf projects, and it is in basically what happens when you build using full rpaths.

The question is moot if the manpage plugin stops referring to hardcoded paths. But is that plugin even of much use on MS Windows?

arrowd added a subscriber: arrowd.Dec 3 2018, 4:45 AM

If the problem this patch solves is to have CMAKE_INSTALL_PREFIX to be set for newly-created projects, why not just add a configuration option to the CMake plugin?

rjvbb added a comment.Dec 3 2018, 9:02 AM

No, it went beyond that: the original underlying principle was "don't assume or hardcode /usr/local anywhere, use whatever install prefix the user specified".

The default location for new projects used by the cmake plugin is a bit in a category of its own as evident from early feedback on my patch, which is why I now propose NOT to set any default in the code.

Adding cmake option for this could be of interest but I think it would be even better to add a (toplevel or session-specific) configuration option so the user can indicate where new projects should be installed. That could go right into the Settings/Projects pane, under the "Projects base directory" setting.
Would be a different diff though.

mwolff requested changes to this revision.Jan 12 2019, 12:26 PM
mwolff added a subscriber: mwolff.
mwolff added inline comments.
plugins/cmake/cmakeutils.h
120

make sure to add this information to the dialog that asks for the install dir and ensure that it shows something like 'default' in it's click message and something more elaborate in the tooltip

plugins/manpage/manpageplugin.cpp
98 ↗(On Diff #46601)
commit 194b97e1d14681d2470193beb746819a38e3ecee
Author: David nolden <david.nolden.kde@art-master.de>
Date:   Thu Feb 3 11:39:24 2011 +0100

    - Don't show manpage documentation for declarations that are not in /usr/
    - Don't show manpage documentation for declarations that are in the current project
    It was very annoying to see manpage documentation just because the function-name accidentally matches something that has a manpage (for example "test")

it sounds like a major cause for annoyance, but I agree that the current workaround might be a bit too much?

but then again, why should we un-ignore stuff in the KDEVELOP_INSTALL_PREFIX?!

This revision now requires changes to proceed.Jan 12 2019, 12:26 PM
rjvbb updated this revision to Diff 49335.Jan 12 2019, 3:03 PM

Adds tooltip to the import dialog's "Installation Prefix" entrie widget.

rjvbb set the repository for this revision to R32 KDevelop.Jan 12 2019, 3:03 PM
rjvbb added inline comments.
plugins/cmake/cmakeutils.h
120

What do you mean with "its click message"? Accepting that dialog immediately gives the next import wizard dialog where you enter the project name.

plugins/manpage/manpageplugin.cpp
98 ↗(On Diff #46601)

This extends David's logic to the situation where you have an entire parallel install in KDEVELOP_INSTALL_PREFIX, and thus a potentially large number of duplicate (or newer versions of) manpages in there.

The manpage plugin is mostly an annoyance to me so I can drop this hunk (I usually turn the plugin off). I've been thinking lately that KDevelop c/should just interface with khelpcentre to handle manpage display (and probably only care about sections 2 and 3*).

mwolff requested changes to this revision.Jan 13 2019, 1:24 PM
mwolff added inline comments.
plugins/cmake/cmakeutils.h
120
plugins/manpage/manpageplugin.cpp
98 ↗(On Diff #46601)

yes, let's drop this hunk (and then also the change to config-kdevelop.h.cmake
since it becomes unused) and we can think about a proper solution to this in a separate patch. if at all, then these whitelisted paths should be user configurable in the manpage plugin configuration

This revision now requires changes to proceed.Jan 13 2019, 1:24 PM
rjvbb updated this revision to Diff 49415.Jan 13 2019, 10:30 PM
rjvbb marked 3 inline comments as done.

placeholder text + dropped manpageplugin hunk.

rjvbb added inline comments.Jan 13 2019, 10:30 PM
plugins/cmake/cmakeutils.h
120

Strange, now I see a "clickMessage" property in the Designer property editor. I'm pretty certain I have the KF5 designer plugin installed on my other system too though.

rjvbb set the repository for this revision to R32 KDevelop.Jan 13 2019, 10:30 PM
mwolff accepted this revision.Jan 15 2019, 1:36 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jan 15 2019, 7:28 PM
This revision was automatically updated to reflect the committed changes.