KDevelop/Shell: set dedicated TMPDIR
Needs ReviewPublic

Authored by rjvbb on Sat, Dec 1, 7:56 PM.

Details

Reviewers
kfunk
Group Reviewers
KDevelop
Summary

This is a simple patch implementing a dedicated KDevelop temporary directory (on Unix). With this, every session has its own directory for temporary files under the default QSP::TempLocation directory, set via the TMPDIR env. variable. The clang parser's preamble* files go here, for instance, but also any files the session creates under QSP::TempLocation itself.

The entire directory is removed during a clean exit, and also at startup. This takes care of preventing the accumulation of (large) old temporary files I tend to get on Mac.

Test Plan

Tested on Mac and Linux. Processes launched by KDevelop will also see the new directory as their TempLocation but I haven't noticed any unwanted side-effects in my workflow. Processes spawned by the session and outliving it may run into issues but how common are those? (The only such processes I create are other KDevelop sessions which are evidently not concerned.)

Setting TEMPDIR (or TEMP) instead of TMPDIR may have the same on MS Windows(?)

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 5666
Build 5684: arc lint + arc unit
rjvbb requested review of this revision.Sat, Dec 1, 7:56 PM
rjvbb created this revision.
kfunk requested changes to this revision.Sun, Dec 2, 9:16 AM
kfunk added a subscriber: kfunk.

Not usable as-is.

  • You're changing behavior for all of KDevelop, while you only want to fix the Clang backend's behavior. This can lead to subtle side effects. I'm not sure we want this. Not sure how to implement this properly the way we use libclang right now either, without further thought.
  • Why not using QTemporaryDir in the first place? Also see patch in QtCreator (https://codereview.qt-project.org/#/c/142566). (Yes, I know QtC usesd libclang out-of-process, we don't. Not relevant here. The interesting bits are the env vars and the QTemporaryDir usage.)

This needs more thought.

This revision now requires changes to proceed.Sun, Dec 2, 9:16 AM
kfunk added a comment.Sun, Dec 2, 9:23 AM

Forgot: Also needs documentation *in* source code why were are doing this. Noone can figure out that the temp dir is changed due to issues with the Clang backend here...

rjvbb added a comment.Sun, Dec 2, 9:26 PM

Actually, issues with clang temp files is why I started to think about this kind of change but ultimately I realised it didn't seem such a bad idea at all to put all KDevelop-related temporary files in a dedicated location. I often clean out a bunch of KDevelop's own temp files that were left behind, e.g. after a crash. I just didn't mention them because they're negligible in size (and I purge before their numbers really start to grow).

I'm not sure what QTemporaryDir would improve here; I use a deterministic name for a reason here. The relevant Qt Creator code I've seen also sets TMPDIR. Probably because it gets picked up by QSP, and most non-Qt applications also seem to recognise TMPDIR as the variable that defines the (user) location for storing temporary files.

I'm curious what kind of subtle side effects others here can think of. The only one I could think of is what could happen with processes that outlive their parent KDevelop process. For the rest, every application spawned by KDevelop should see the same TMPDIR location and I'm guessing (hoping) that none expect to exchange files with other processes under the assumption that everyone uses the same temp. directory.
Files in TMPDIR are usually private, or else their full path is handed to external apps that need to use them (like sockets), no?

Note that there is a design oversight in the initial version: if the QSP::TempLocation ends with /kdevelop-temp-session-XXXX that substring should be clipped to avoid recursion when launching another session through KDevelop.

kfunk added a comment.Mon, Dec 3, 6:01 AM

Okay, trying to keep this short.

I'm not sure what QTemporaryDir would improve here;

It would implement this feature with around 3 lines of code instead of 18, because it is /exactly/ suited for this use-case.

I use a deterministic name for a reason here.

You can instruct QTemporaryDir to use hard-coded names; just do not use "XXXXXX" in the template path passed to it. Less code is preferrable.

The relevant Qt Creator code I've seen also sets TMPDIR.

That's *after* creating the directory. You'd still have to do it here as well.

pino added a subscriber: pino.Mon, Dec 3, 6:10 AM

Also, considering that "session" in Core::initialize is user-specified and defaults to an empty string, please do not use a 100% deterministic name, otherwise two different users will have a temporary directory conflict when launching kdevelop. Please use the "XXXXXX" variable part.

rjvbb added a comment.Mon, Dec 3, 10:21 AM
Also, considering that "session" in `Core::initialize` is user-specified and defaults to an empty string, please do not use a 100% deterministic name,

From what I can see the function is only called from main() and the session variable will not ever be an empty string in practice. I just tested with a newly created session: even there a new UUID was created before calling Core::initialize().

I can still add the user ID to the dirname just in case session is ever empty after all. That's actually useful if TMPDIR isn't set to start with and the directory is created in /tmp .

I just tested QTemporaryDir. It only relieves me from handling the directory creation and deletion because I still have to create the absolute path (and thus prune it): giving just the directory name (instead of a full path) means the directory will be created in the current directory.

So I end up with a slightly cleaner snippet

#if defined(Q_OS_UNIX)
    auto tmpPath = QStandardPaths::writableLocation(QStandardPaths::TempLocation);
    const QString tmpName(QStringLiteral("/kdevelop-tmp-%1-").arg(getuid()));
    const auto pos = tmpPath.lastIndexOf(tmpName);
    if (pos >= 0) {
        // we must have been started by another KDevelop session,
        // restore the default TempLocation
        tmpPath = tmpPath.left(pos);
    }
    tmpPath += tmpName + session;
    m_tmpLocation = new QTemporaryDir(tmpPath);
    if (m_tmpLocation->isValid()) {
        qputenv("TMPDIR", tmpPath.toLatin1());
        qCDebug(SHELL) << "temp. directory:" << m_tmpLocation->path();
    }
#endif

and still don't have a deterministic name because QTemporaryDir will always add something random to the name even if you don't include an "XXXXXX" pattern in the template. From the docs:

If the templatePath ends with XXXXXX it will be used as the dynamic portion of the directory name, otherwise it will be appended.

and this is obligatory on Unix as mkdtemp() is used underneath. So the snippet above is actually broken, the temp dir is not at tmpLocation.
Come to think of it, the directory is also not emptied on creation as I want it to be; the class guarantees it never reuses an existing directory.

rjvbb updated this revision to Diff 46779.Mon, Dec 3, 11:01 AM

FIxed and streamlined version.

rjvbb set the repository for this revision to R32 KDevelop.Mon, Dec 3, 11:02 AM