Differential D4095 Diff 12149 projectmanagers/cmake/cmakeserver.cpp

Changeset View

Standalone View

View Options

projectmanagers/cmake/cmakeserver.cpp

  • This file was added.
1​/* KDevelop CMake Support
2​ *
3​ * Copyright 2017 Aleix Pol <aleixpol@kde.org>
4​ *
mwolffUnsubmitted
Done

2017

mwolff: 2017
5​ * This program is free software; you can redistribute it and/or
6​ * modify it under the terms of the GNU General Public License
7​ * as published by the Free Software Foundation; either version 2
8​ * of the License, or (at your option) any later version.
9​ *
10​ * This program is distributed in the hope that it will be useful,
11​ * but WITHOUT ANY WARRANTY; without even the implied warranty of
12​ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
13​ * GNU General Public License for more details.
14​ *
15​ * You should have received a copy of the GNU General Public License
16​ * along with this program; if not, write to the Free Software
17​ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
18​ * 02110-1301, USA.
19​ */
20​
21​#include "cmakeserver.h"
22​#include "cmakeprojectdata.h"
23​#include "cmakeutils.h"
24​#include <QDir>
25​#include <QJsonDocument>
mwolffUnsubmitted
Done

unused?

mwolff: unused?
26​#include <QJsonObject>
27​#include <QJsonArray>
28​#include <QTimer>
29​#include <QTemporaryFile>
30​#include "debug.h"
31​
32​CMakeServer::CMakeServer(QObject* parent)
33​ : QObject(parent)
34​ , m_localSocket(new QLocalSocket(this))
35​{
kfunkUnsubmitted
Not Done

QTemporaryFile

kfunk: `QTemporaryFile`
36​ QString path;
37​ {
38​ QTemporaryFile file(QDir::tempPath() + "/kdevelopcmake-");
39​ file.open();
mwolffUnsubmitted
Not Done

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

mwolff: keep it around, otherwise the file gets removed again, no? or is that OK?
apolAuthorUnsubmitted
Not Done

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

apol: No, actually we don't want the file at all. We just want a temporary and unique path but no…
40​ file.close();
41​ path = file.fileName();
42​ }
43​
44​ m_process.setProcessChannelMode(QProcess::ForwardedChannels);
45​
46​ connect(&m_process, &QProcess::errorOccurred, this, [this, path](QProcess::ProcessError error) {
mwolffUnsubmitted
Not Done

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

mwolff: QStringLiteral, and I hope cmake supports "--pipe", "path" as two args? i.e. instead of `…
apolAuthorUnsubmitted
Not Done
apol@oliver:~$ cmake -E server --experimental --pipe=fu
^C
apol@oliver:~$ cmake -E server --experimental --pipe fu
CMake Error: Unknown argument for server mode
apol: ``` apol@oliver:~$ cmake -E server --experimental --pipe=fu ^C apol@oliver:~$ cmake -E server…
mwolffUnsubmitted
Not Done

:-S

mwolff: :-S
47​ qCWarning(CMAKE) << "cmake server error:" << error << path << m_process.readAllStandardError() << m_process.readAllStandardOutput();
48​ });
49​ connect(&m_process, static_cast<void(QProcess::*)(int)>(&QProcess::finished), this, [](int code){
50​ qCDebug(CMAKE) << "cmake server finished with code" << code;
51​ });
52​
53​ connect(m_localSocket, &QIODevice::readyRead, this, &CMakeServer::processOutput);
54​ connect(m_localSocket, static_cast<void(QLocalSocket::*)(QLocalSocket::LocalSocketError)>(&QLocalSocket::error), this, [this, path](QLocalSocket::LocalSocketError socketError) {
55​ qCWarning(CMAKE) << "cmake server socket error:" << socketError << path;
56​ setConnected(false);
57​ });
58​ connect(m_localSocket, &QLocalSocket::connected, this, [this]() { setConnected(true); });
59​
60​ connect(&m_process, &QProcess::started, this, [this, path](){
61​ //Once the process has started, wait for the file to be created, then connect to it
62​ m_localSocket->connectToServer(path, QIODevice::ReadWrite);
63​ QTimer::singleShot(100, this, [this, path]() {
mwolffUnsubmitted
Not Done

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

mwolff: can you add a comment saying why this is required? is there really no "good" way to wait for…
apolAuthorUnsubmitted
Not Done

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

apol: Could be a good idea to talk with him about this, yes.
64​ if (!m_localSocket->isOpen())
mwolffUnsubmitted
Done

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

mwolff: you don't need the full timer object anymore, do you? Simply use QTimer::singleShot?
65​ m_localSocket->connectToServer(path, QIODevice::ReadWrite);
mwolffUnsubmitted
Not Done

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?

mwolff: what if the first connect fails - we simply get an error and disconnect and don't try…
apolAuthorUnsubmitted
Not Done

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.

apol: FWIW, this is the code in QtCreator: ``` m_connectionTimer.setInterval(100); connect…
mwolffUnsubmitted
Not Done

ah ok, yes that makes sense.

mwolff: ah ok, yes that makes sense.
66​ });
67​ });
68​ m_process.start(CMake::findExecutable(), {"-E", "server", "--experimental", "--pipe=" + path});
mwolffUnsubmitted
Done

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);
            });
        });
mwolff: I'd simplify the above a bit (imo) by doing something like: ``` connect(...started, this…
69​}
70​
71​CMakeServer::~CMakeServer()
72​{
73​ m_process.kill();
74​ m_process.waitForFinished();
75​}
76​
77​void CMakeServer::setConnected(bool conn)
kfunkUnsubmitted
Done

Rename to sendCommand?

kfunk: Rename to `sendCommand`?
78​{
79​ if (conn == m_connected)
80​ return;
81​
82​ m_connected = conn;
mwolffUnsubmitted
Done

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

mwolff: QByteArrayLiteral, wrap in functions returning QByteArray to get rid of global static…
83​ if (m_connected)
84​ Q_EMIT connected();
85​ else
86​ Q_EMIT disconnected();
87​}
88​
89​bool CMakeServer::isServerAvailable()
90​{
91​ return m_localSocket->isOpen();
mwolffUnsubmitted
Done

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

mwolff: qCDebug(...) << "writing..." << object;
92​}
mwolffUnsubmitted
Done

spaces around operators

mwolff: spaces around operators
93​
94​static QByteArray openTag() { return QByteArrayLiteral("\n[== \"CMake Server\" ==[\n"); }
95​static QByteArray closeTag() { return QByteArrayLiteral("\n]== \"CMake Server\" ==]\n"); }
96​
97​void CMakeServer::sendCommand(const QJsonObject& object)
98​{
99​ Q_ASSERT(isServerAvailable());
mwolffUnsubmitted
Done
const auto openTag = ::openTag(); // once you added the getter
const auto oepnTagSize = openTag.size();
// use both below
mwolff: ``` const auto openTag = ::openTag(); // once you added the getter const auto oepnTagSize =…
100​
mwolffUnsubmitted
Done

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

mwolff: this can go into an infinite-loop, no? if m_buffer.size() == openTag.size() + 1, then we go…
101​ const QByteArray data = openTag() + QJsonDocument(object).toJson(QJsonDocument::Compact) + closeTag();
102​ auto len = m_localSocket->write(data);
103​// qCDebug(CMAKE) << "writing...\n" << QJsonDocument(object).toJson();
104​ Q_ASSERT(len > 0);
mwolffUnsubmitted
Done

spaces around operators

mwolff: spaces around operators
105​}
106​
107​void CMakeServer::processOutput()
mwolffUnsubmitted
Done

else break?

mwolff: else break?
apolAuthorUnsubmitted
Not Done

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

apol: Yep, actually I added it yesterday when testing. Good catch! :D
108​{
109​ Q_ASSERT(m_localSocket);
110​
111​ const auto openTag = ::openTag();
112​ const auto closeTag = ::closeTag();
113​
114​ m_buffer += m_localSocket->readAll();
115​ for(; m_buffer.size() > openTag.size(); ) {
116​
117​ Q_ASSERT(m_buffer.startsWith(openTag));
118​ const int idx = m_buffer.indexOf(closeTag, openTag.size());
mwolffUnsubmitted
Done

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

mwolff: since we don't generate the response data, I wouldn't assert on it. if there's a cmake bug it…
apolAuthorUnsubmitted
Not Done

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

apol: Yeah... I added it when cmake was crashing. I'll remove for now.
mwolffUnsubmitted
Done

missing curly braces

mwolff: missing curly braces
119​ if (idx >= 0) {
120​ emitResponse(m_buffer.mid(openTag.size(), idx - openTag.size()));
121​ m_buffer = m_buffer.mid(idx + closeTag.size());
122​ } else {
123​ break;
124​ }
125​ }
126​}
127​
128​void CMakeServer::emitResponse(const QByteArray& data)
129​{
mwolffUnsubmitted
Not Done

here and below: QStringLiteral

mwolff: here and below: QStringLiteral
apolAuthorUnsubmitted
Not Done

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

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

think of all the little allocations :P

mwolff: think of all the little allocations :P
130​ QJsonParseError error;
131​ auto doc = QJsonDocument::fromJson(data, &error);
132​ if (error.error) {
133​ qCWarning(CMAKE) << "error processing" << error.errorString() << data;
134​ }
135​ Q_ASSERT(doc.isObject());
136​ Q_EMIT response(doc.object());
137​}
138​
139​void CMakeServer::handshake(const KDevelop::Path& source, const KDevelop::Path& build)
140​{
141​ Q_ASSERT(!source.isEmpty());
142​ Q_ASSERT(!build.isEmpty());
143​
144​ sendCommand({
145​ {"cookie", ""},
146​ {"type", "handshake"},
147​ {"major", 1},
148​ {"protocolVersion", QJsonObject{{"major", 1}} },
149​ {"sourceDirectory", source.toLocalFile()},
150​ {"buildDirectory", build.toLocalFile()},
151​ {"generator", "Ninja"} //TODO: make it possible to keep whatever they have ATM
152​ });
153​}
154​
155​void CMakeServer::configure(const QStringList& args)
156​{
157​ sendCommand({
158​ {"type", "configure"},
159​ {"cacheArguments", QJsonArray::fromStringList(args)}
160​ });
161​}
162​
163​void CMakeServer::compute()
164​{
165​ sendCommand({ {"type", "compute"} });
166​}
167​
168​void CMakeServer::codemodel()
169​{
170​ sendCommand({ {"type", "codemodel"} });
171​}