-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Restore GRPC #90
Restore GRPC #90
Changes from 3 commits
c635f7d
be75b37
8563719
cc34f3d
0414557
ca41ec0
a0b5539
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -240,11 +240,6 @@ void CompilerClient::UpdateLoop() { | |
} | ||
|
||
ServerPlugin::ServerPlugin(MainWindow& mainWindow) : RGMPlugin(mainWindow) { | ||
/* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't seem to be checking the new macros you created. This code's just always uncommented. Are you just relying on it not being called? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am checking the define |
||
|
||
FIXME: gRPC currently causes infinite hang on linux and segfault on MinGW | ||
disabling until fixed. Uncomment all methods below when fixed. | ||
|
||
// create a new child process for us to launch an emake server | ||
process = new QProcess(this); | ||
|
||
|
@@ -255,14 +250,15 @@ ServerPlugin::ServerPlugin(MainWindow& mainWindow) : RGMPlugin(mainWindow) { | |
const QDir dir(path); | ||
QDir::Filters filters = QDir::Filter::Executable | QDir::Filter::Files; | ||
// we use a wildcard because we want it to find emake.exe on Windows | ||
auto entryList = dir.entryInfoList(QStringList("emake*"), filters, QDir::SortFlag::NoSort); | ||
auto entryList = dir.entryInfoList(QStringList({"emake","emake.exe"}), filters, QDir::SortFlag::NoSort); | ||
if (!entryList.empty()) { | ||
emakeFileInfo = entryList.first(); | ||
break; | ||
} | ||
} | ||
|
||
// use the closest matching emake file we found and launch it in a child process | ||
qDebug() << emakeFileInfo.fileName(); | ||
process->setWorkingDirectory(emakeFileInfo.absolutePath()); | ||
QString program = emakeFileInfo.fileName(); | ||
QStringList arguments; | ||
|
@@ -287,27 +283,28 @@ ServerPlugin::ServerPlugin(MainWindow& mainWindow) : RGMPlugin(mainWindow) { | |
// without us needing any threading or blocking the main thread | ||
QTimer* timer = new QTimer(this); | ||
connect(timer, &QTimer::timeout, compilerClient, &CompilerClient::UpdateLoop); | ||
timer->start(); | ||
// timer delay larger than one so we don't hog the CPU core | ||
timer->start(1); | ||
|
||
// update initial keyword set and systems | ||
compilerClient->GetResources(); | ||
compilerClient->GetSystems(); | ||
*/ | ||
|
||
} | ||
|
||
ServerPlugin::~ServerPlugin() { /*process->close();*/ } | ||
ServerPlugin::~ServerPlugin() { process->close(); } | ||
|
||
void ServerPlugin::Run() { /*compilerClient->CompileBuffer(mainWindow.Game(), CompileRequest::RUN);*/ }; | ||
void ServerPlugin::Run() { compilerClient->CompileBuffer(mainWindow.Game(), CompileRequest::RUN); }; | ||
|
||
void ServerPlugin::Debug() { /*compilerClient->CompileBuffer(mainWindow.Game(), CompileRequest::DEBUG);*/ }; | ||
void ServerPlugin::Debug() { compilerClient->CompileBuffer(mainWindow.Game(), CompileRequest::DEBUG); }; | ||
|
||
void ServerPlugin::CreateExecutable() { | ||
/*const QString& fileName = | ||
const QString& fileName = | ||
QFileDialog::getSaveFileName(&mainWindow, tr("Create Executable"), "", tr("Executable (*.exe);;All Files (*)")); | ||
if (!fileName.isEmpty()) | ||
compilerClient->CompileBuffer(mainWindow.Game(), CompileRequest::COMPILE, fileName.toStdString());*/ | ||
compilerClient->CompileBuffer(mainWindow.Game(), CompileRequest::COMPILE, fileName.toStdString()); | ||
}; | ||
|
||
void ServerPlugin::SetCurrentConfig(const resources::Settings& settings) { | ||
/*compilerClient->SetCurrentConfig(settings);*/ | ||
compilerClient->SetCurrentConfig(settings); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're linking in all the code regardless of whether you intend to use it, this could as well be a runtime check.
Also, we're leaking that pluginServer ref. It should probably be a
unique_ptr
owned by this class (can be left null when disabled).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting making it a dll? Yeah that's how Qt Creator has its own plugin interface, I just wasn't ready to set it up yet.
And no, we're not, Qt has its own ownership policy. The server plugin reference is owned by the main window. This is because the server plugin inherits from my generic RGM plugin abstraction which itself is based on QObject. All RGM plugins are informed of the main window during construction in the current implementation.
RadialGM/Plugins/RGMPlugin.h
Line 31 in 6d626d7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I'm just suggesting being consistent about whether we're eliminating code at compile time or branching over it at runtime. My complaint above is related.
You are building an entire unused class into RGM in this case. I suggest either making it a runtime check (e.g, a flag), or removing the class from the codegen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which class is the one you are saying is unused? Are you saying RGMPlugin is unused or ServerPlugin is unused? The former is clearly true, but the latter is not always true. I am in fact filtering out the ServerPlugin class entirely in both the pro qmake and the CMakeLists.
https://github.com/enigma-dev/RadialGM/pull/90/files#diff-af3b638bc2a3e6c650974192a53c7291R162-R166
https://github.com/enigma-dev/RadialGM/pull/90/files#diff-bad885b9516edcb9eac2d0c8c57a7ebdR33-R40
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if you remove the
#if
from just MainWindow, it will generate link errors because the Qt project file excludes the class?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, assuming you haven't uncommented
rgm_enable_grpc_server
in the pro or CMakeLists.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit awkward, but okay. Proceed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silly me though I forgot to wrap the include in the main window.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok though, as I said later we'll move plugins to dlls.