D17289: KDevelop/Shell: set dedicated TMPDIR

[Milian Wolff]

mwolff added a comment.


  In D17289#401264 <https://phabricator.kde.org/D17289#401264>, @rjvbb wrote:
  
  > >   we use CXTranslationUnit_CreatePreambleOnFirstParse to get code completion results fast. otherwise the first code completion request would create the preamble, which felt much worse
  >
  > Shall we keep that discussion to D18551 <https://phabricator.kde.org/D18551>?
  
  
  Yes.
  
  >>   that's ~50ish code locations... Quite a lot :-/
  > 
  > Erm, yes, so many that it begs the question if "this is worth it"... Is it?
  
  Exactly.
  
  > I only looked at places where the existing env. profile feature is used. There were already enough of those that I didn't feel like changing them all potentially for nothing, but I don't think there were this many.
  > 
  > Why would we not limit the change to those locations, or in other words, why is the selected env. profile not used for every process KDevelop spawns?
  
  Because someone needs to write the code for that. And there are probably places where the simpler QProcess API was used instead. One way or another, someone needs to refactor all of the code.
  
  > Would it be feasible to reduce the footprint of this overhaul by using a wrapper around writableLocation(QSP::TempLocation) instead?
  
  I don't see how that would change anything, considering we need to change TMPDIR for libclang, which isn't using Qt internally?
  
  > I'd have to check in how many locations libclang reads TMPDIR and so if it's feasible to change that variable temporarily around the parser's entrypoints into libclang.
  
  You cannot, since env vars are process-wide and thus changing them from a background thread would lead to race conditions.
  
  > (replying by email)
  > 
  >>> +#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
  > 
  > Aren't those  used in the MSWin version of `TempLocation`? I can try to implement this for MSWin too but only if someone promises to test it (I can't).
  > 
  > BTW, cf. https://codereview.qt-project.org/#/c/239952
  > 
  >> 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
  > 
  > Evidently this can go once that restored system env. is used.
  > 
  >>> +    m_tmpDir->mkpath(tmpLocation);
  >> 
  >> it exists, you create the path in the line above
  > 
  > Except when mkpath failed because tmpLocation was messed up. KDevelop does NOT like that. That happened to me during development and I decided to leave the check in.
  
  Then handle mkpath's return value properly and error out
  
  >>> +    void systemEnvironment(QProcessEnvironment& env) const;
  >> 
  >> don't use out params, just return a QProcessEnvironment similar to how it's done in `QProcessEnvironment::systemEnvironment()`
  > 
  > This was out of a concern that certain spawned processes might be performance critical. I guess the overhead of the extra copy can be neglected then?
  
  the value is implicitly shared, returning it is cheap

REPOSITORY
  R32 KDevelop

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

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