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

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

Details

Reviewers
kfunk
Group Reviewers
KDevelop
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

Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 5571
Build 5589: arc lint + arc unit
rjvbb requested review of this revision.Fri, Nov 30, 11:38 AM
rjvbb created this revision.
kfunk requested changes to this revision.Fri, Nov 30, 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.Fri, Nov 30, 12:04 PM
kfunk added inline comments.Fri, Nov 30, 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.Fri, Nov 30, 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.Fri, Nov 30, 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.Fri, Nov 30, 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.Fri, Nov 30, 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.Sat, Dec 1, 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.Sat, Dec 1, 11:04 AM
rjvbb marked 5 inline comments as done.
kfunk added a subscriber: apol.Sun, Dec 2, 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

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.Sun, Dec 2, 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.Sun, Dec 2, 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.Sun, Dec 2, 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.Mon, Dec 3, 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.Mon, Dec 3, 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.