D17289: KDevelop/Shell: set dedicated TMPDIR

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added a comment.
This revision now requires changes to proceed.


  we use CXTranslationUnit_CreatePreambleOnFirstParse to get code completion results fast. otherwise the first code completion request would create the preamble, which felt much worse
  
  otherwise this patch has some issues, but it goes in the right direction. you'll have to audit our whole platform then for use of `[QK]Process` and ensure the original system env is used. I think these two greps should show you the places:
  
    ack '[QK]Process \w' # full object instances
    ack '[QK]Process::start' # static utilities
  
  that's ~50ish code locations... Quite a lot :-/

INLINE COMMENTS

> core.cpp:134
>      qCDebug(SHELL) << "Creating controllers";
> +#if defined(Q_OS_UNIX)
> +    auto tmpLocation = QStandardPaths::writableLocation(QStandardPaths::TempLocation);

imo this should also be handled on windows via `TMP` and `TEMP`, according to

https://superuser.com/questions/709953/temp-vs-tmp-in-environment-variables

> core.cpp:136
> +    auto tmpLocation = QStandardPaths::writableLocation(QStandardPaths::TempLocation);
> +    const QString tmpName(QStringLiteral("/kdevelop-tmp-%1-").arg(getuid()));
> +    const auto pos = tmpLocation.lastIndexOf(tmpName);

all of this code shouldn't be required - the system env used to launch subprocesses should not use the changed tmpdir after all, so it cannot be recursive

> core.cpp:150
> +    m_tmpDir->mkpath(tmpLocation);
> +    if (m_tmpDir->exists()) {
> +        const auto origTmpDir = qgetenv("TMPDIR");

it exists, you create the path in the line above

> core.cpp:151
> +    if (m_tmpDir->exists()) {
> +        const auto origTmpDir = qgetenv("TMPDIR");
> +        if (!origTmpDir.isEmpty()) {

instead of this code, you should do something like

  #if defined(Q_OS_UNIX)
  m_originalTmpDir = qEnvironmentVariable("TMPDIR");
  qputenv("TMPDIR", tmpLocation.toLatin1());
  #elif defined(Q_OS_WIN)
  m_originalTmpDir = qEnvironmentVariable("TEMP");
  if (m_originalTmpDir.isEmpty()) {
      m_originalTmpDir = qEnvironmentVariable("TMP");
  }
  qputenv("TEMP", tmpLocation.toLatin1());
  qputenv("TMP", tmpLocation.toLatin1());
  #else
  #error unhandled OS
  #endif

> core.cpp:341
>  
> +    if (m_tmpDir->exists()) {
> +        m_tmpDir->removeRecursively();

it must exist, no? we should always create it after all

> core.cpp:343
> +        m_tmpDir->removeRecursively();
> +        delete m_tmpDir;
> +    }

don't make this a ptr

> core.cpp:482
> +{
> +    env = QProcessEnvironment::systemEnvironment();
> +    if (!d->m_originalTmpDir.isEmpty()) {

this should essentially be

  auto env = QProcessEnvironment::systemEnvironment();
  env.insert(QStringLiteral("TMPDIR"), d->m_originalTmpDir);

> core.cpp:483
> +    env = QProcessEnvironment::systemEnvironment();
> +    if (!d->m_originalTmpDir.isEmpty()) {
> +        // restore the original TMPDIR value

don't check whether it's empty or not, always insert the original value. this will essentially remove the TMPDIR env var then and whatever system default is picked up then

> core.cpp:485
> +        // restore the original TMPDIR value
> +        env.remove(QStringLiteral("TMPDIR"));
> +        env.insert(QStringLiteral("TMPDIR" ), d->m_originalTmpDir);

insert will overwrite, so this line is not required

> core.h:100
> +     */
> +    void systemEnvironment(QProcessEnvironment& env) const;
> +

don't use out params, just return a QProcessEnvironment similar to how it's done in `QProcessEnvironment::systemEnvironment()`

> core_p.h:79
> +    QString m_originalTmpDir;
> +    QDir* m_tmpDir = nullptr;
>  };

no ptr

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D17289

To: rjvbb, #kdevelop, kfunk, mwolff
Cc: aaronpuchert, mwolff, pino, kfunk, kdevelop-devel, glebaccon, hase, antismap, iodelay, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20190127/578385e1/attachment-0001.html>