Refactor to embrace a cmake server import backend
ClosedPublic

Authored by apol on Jan 12 2017, 1:03 AM.

Details

Summary

Allow using cmake -E server rather than the compilecommands json files we use
nowadays.

Test Plan

Opened few projects locally successfully. Needs a patch I submitted to upstream
cmake and won't be released until 3.7.2 is out.

Diff Detail

Repository
R32 KDevelop
Branch
apol/cmakeserver
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes
mwolff added inline comments.Jan 22 2017, 8:53 PM
projectmanagers/cmake/cmakemanager.cpp
143

for PODs, you don't need Q_GLOBAL_STATIC. simply make this a

namespace {
static bool s_serverSupported = false;
}

on a conceptual level - this depends on the cmake binary that can be selected for every project, no? So this should not be a static at all, but rather a per-project property?

171

make this a warning? also, do we want to show the user that? this can be serious and render the whole cmake support useless after all, right?

195

warning, and see above? actually the whole code is duplicated - move this into a proper slot? or at least share the lambda body?

projectmanagers/cmake/cmakeprojectdata.cpp
3

the future is now! it's 2017 :P

31

missing parent?

33

empty line at end please

projectmanagers/cmake/cmakeprojectdata.h
61

& next to type names

71

simply make it public like the stuff above and remove the getter?

projectmanagers/cmake/cmakeserver.cpp
4

2017

25

unused?

39

keep it around, otherwise the file gets removed again, no? or is that OK?

46

QStringLiteral, and I hope cmake supports "--pipe", "path" as two args? i.e. instead of --pipe=path, can you pass --pipe path?

63

can you add a comment saying why this is required? is there really no "good" way to wait for the server to start up? what is creator doing here? have you talked to thunger about this? looks really brittle

68

I'd simplify the above a bit (imo) by doing something like:

connect(...started, this, [this, path] () {
            // please document me
            QTimer::singleShot(100, this, [this, path] () {
                m_localSocket->connectToServer(path, QIODevice::ReadWrite);
            });
        });
82

QByteArrayLiteral, wrap in functions returning QByteArray to get rid of global static initialization

91

qCDebug(...) << "writing..." << object;

92

spaces around operators

99
const auto openTag = ::openTag(); // once you added the getter
const auto oepnTagSize = openTag.size();
// use both below
100

this can go into an infinite-loop, no?

if m_buffer.size() == openTag.size() + 1, then we go into the loop, but we won't find the close tag and thus cannot ever get out of the loop

see below

104

spaces around operators

107

else break?

118

since we don't generate the response data, I wouldn't assert on it. if there's a cmake bug it would kill kdevelop...

129

here and below: QStringLiteral

projectmanagers/cmake/cmakeserver.h
4

dito, 2017

30

this is actually a CMakeServerClient, right?

projectmanagers/cmake/cmakeserverimportjob.cpp
4

really?

31

you only use it with T == Path::List, so why not hardcode that?

40

please add it as a getter to the MakefileResolver API, which is used elsewhere in CMake already. That way we don't duplicate this magic

44

QStringLiteral

70

this could probably be part of a helper function in the MakefileResolver (see above), since that does something like that already elsewhere, I guess?

84

spaces around operators

projectmanagers/cmake/cmakeserverimportjob.h
2–3

really?

34

indent

This revision now requires changes to proceed.Jan 22 2017, 8:53 PM
apol marked 27 inline comments as done.Jan 24 2017, 1:33 AM

That was one thorough review! Thanks a lot milian!!

Will be submitting a new version with the fixes. Then if there's no big issue I'll merge to master by the end of the week, so we can start testing.

projectmanagers/cmake/cmakemanager.cpp
195

As is we can't, they should inherit from the same class. I don't think it's a big deal at the moment...

projectmanagers/cmake/cmakeprojectdata.cpp
31

Hm? It's not a QObject.

projectmanagers/cmake/cmakeserver.cpp
39

No, actually we don't want the file at all. We just want a temporary and unique path but no file.

46
apol@oliver:~$ cmake -E server --experimental --pipe=fu
^C
apol@oliver:~$ cmake -E server --experimental --pipe fu
CMake Error: Unknown argument for server mode
63

Could be a good idea to talk with him about this, yes.

107

Yep, actually I added it yesterday when testing. Good catch! :D

118

Yeah... I added it when cmake was crashing. I'll remove for now.

129

My brain wants to do it, but then it becomes an unreadable blob... :(

projectmanagers/cmake/cmakeserver.h
4

xD well I wrote this code in 2016.

Ah, time, thou art a heartless bitch,

30

Well, whoever interacts with this is interacting with the server after all.

projectmanagers/cmake/tests/test_cmakemanager.cpp
283

Sh... at least we are testing a bit more than we used to...

apol updated this revision to Diff 10471.Jan 24 2017, 1:34 AM
apol edited edge metadata.
apol marked 2 inline comments as done.

Addressed issues pointed by milian

apol updated this revision to Diff 10472.Jan 24 2017, 1:37 AM
apol edited edge metadata.

Adapt milian changes

mwolff requested changes to this revision.Jan 24 2017, 8:31 AM
mwolff edited edge metadata.

lgtm to me mostly, but some stuff still needs to be ironed out, imo

projectmanagers/cmake/cmakemanager.cpp
143

this is not used anywhere now?

167–191

Who will delete the server when it is available? this looks like you are piling up servers until the CMakeManager is deleted at shutdown.

You probably want to use a unique_ptr here and transfer ownership into the import job when the server is available

projectmanagers/cmake/cmakeserver.cpp
46

:-S

64

you don't need the full timer object anymore, do you? Simply use QTimer::singleShot?

65

what if the first connect fails - we simply get an error and disconnect and don't try thereafter, right? couldn't we retry for up to N times (say every 100ms for up to 1s?) to make this a bit less brittle?

118

missing curly braces

129

think of all the little allocations :P

projectmanagers/cmake/cmakeserver.h
30

right, it's a client to the server, I'd welcome the rename personally

This revision now requires changes to proceed.Jan 24 2017, 8:31 AM
apol marked 4 inline comments as done.Jan 30 2017, 1:38 AM
apol added inline comments.
projectmanagers/cmake/cmakeserver.cpp
65

FWIW, this is the code in QtCreator:

m_connectionTimer.setInterval(100);
connect(&m_connectionTimer, &QTimer::timeout, this, &ServerMode::connectToServer);

I'd prefer doing the same as them and discussing with Tobias how to improve it, as he did cmake-server implementation as well as qtcreator.

mwolff added inline comments.Jan 30 2017, 9:52 AM
projectmanagers/cmake/cmakeserver.cpp
65

ah ok, yes that makes sense.

apol updated this revision to Diff 11447.Feb 17 2017, 1:49 PM
apol edited edge metadata.
  • Change cmake setup logic slightly
  • Don't try to connect if already connected. Otherwise it errors out
apol updated this revision to Diff 11473.Feb 17 2017, 11:33 PM
apol edited edge metadata.
  • Don't try to connect if already connected. Otherwise it errors out and everything crashes
mwolff accepted this revision.Feb 18 2017, 9:31 PM

minor nitpicks, otherwise lgtm

please fix those and then feel free to commit

projectmanagers/cmake/cmakemanager.cpp
225

these shouldn't be QPointer, or?

projectmanagers/cmake/cmakeserverimportjob.cpp
72

space after for

75

add braces and spaces around operator

148

add braces to all conditionals here

157

braces

projectmanagers/cmake/tests/test_cmakeserver.cpp
30

indent

This revision was automatically updated to reflect the committed changes.
antonanikin added a comment.EditedMar 6 2017, 5:48 AM

I have problems with the patch and cmake 3.7.2:

unhandled message QJsonObject({"supportedProtocolVersions":[{"isExperimental":true,"major":1,"minor":0}],"type":"hello"})
error!! QJsonObject({"cookie":"","errorMessage":"Failed to activate protocol version: \"CMAKE_GENERATOR\" is set but incompatible with configured generator value.","inReplyTo":"handshake","type":"error"})
kdevelop.projectmanagers.cmake: couldn't load json successfully "kdev-valgrind"
kdevelop.projectmanagers.cmake: couldn't load json successfully "kdev-valgrind"

Background parser now does nothing :(.

cmake 3.8.0-rc2 + full-rebuild kdevplatform/kdevelop - still breaks on project parsing :(

zhigalin reopened this revision.Mar 6 2017, 7:54 PM
zhigalin added a subscriber: zhigalin.
zhigalin added inline comments.
projectmanagers/cmake/cmakemanager.cpp
168

Somewhy is not working:

kdevelop.projectmanagers.cmake: cmake server socket error: QLocalSocket::ServerNotFoundError "/tmp/kdevelopcmake-.H17014"
CMake Error: cmake version 3.5.1
Usage: /usr/bin/cmake -E <command> [arguments...]
Available commands:
chdir dir cmd [args...] - run command in a given directory
compare_files file1 file2 - check if file1 is same as file2
...

zhigalin accepted this revision.Mar 7 2017, 11:44 AM

Please close this.

apol added a comment.Mar 18 2017, 8:04 PM
In D4095#95855, @kfunk wrote:

Please close this.

I can't do that... :( I can only abandon it.

bcooksley added a subscriber: bcooksley.

The revision is still pending acceptance by Kevin, which is why you can't close it.
Removing him as a Reviewer should fix this - which i've just done.

This revision is now accepted and ready to land.Mar 18 2017, 8:46 PM
bcooksley closed this revision.Mar 18 2017, 8:46 PM
kfunk added a comment.Mar 19 2017, 9:46 AM

The revision is still pending acceptance by Kevin, which is why you can't close it.
Removing him as a Reviewer should fix this - which i've just done.

Ah, that's why! Thanks!