-
Notifications
You must be signed in to change notification settings - Fork 119
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
Disable Server Protobuf Fixes #1724
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1724 +/- ##
=======================================
Coverage 24.31% 24.31%
=======================================
Files 166 166
Lines 16603 16603
=======================================
Hits 4037 4037
Misses 12566 12566 Continue to review full report at Codecov.
|
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.
One comment; make sure you have a plan.
optional resources.Settings settings = 1; ///< The settings that you want to apply including API (e.g, "Direct3D9" for the graphics system). | ||
} | ||
|
||
message SyntaxCheckRequest { |
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.
Where is the request message used if not in the RPC? I don't think there's a reason to separate request/response messages from the service definition. What's the point of building the request if there's nowhere to send it?
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.
The RPC stubs just call out to the compileEGMf functions. Also it doesn't matter, you said to use the Protocol Buffer interfaces directly for new stuff. That means in order to compile the compiler you'd need GRPC, but we don't want that. Emake could be built without server support but still do command line syntax checking.
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.
The new compile method doesn't accept a CompileRequest
—it takes a Project
proto and converts it to a GameData
struct. To my knowledge, only the RPC service would accept a CompileRequest
.
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.
I am sorry, I was talking about the new logging methods.
enigma-dev/CommandLine/emake/Server.cpp
Line 140 in 4d62bf0
SyntaxError GetSyntaxError(syntax_error* err) { |
void CallBack::ProcessOutput() { |
You are correct. Regardless, ideally, we'd just add the wrapper accepting SyntaxCheckRequest in compileEGMf, then have the old version call the new one. So again, we want to use these buffers in the compiler now, but don't want to make the compiler depend on GRPC by default.
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.
Okay, let's not get too carried away, here. Remember, you can't just pass the protos themselves across DLL bounds, anyway; you have to serialize them, first. I think we have emake just calling external methods accepting and returning protos all willy-nilly, and that works because emake and ENIGMA are built by the same compiler at the same time. But RGM and LGM cannot use that interface. They have to use an overload that accepts byte[]
(or char*
) and performs the serialization in the C++.
If that's where you want to use the request/response messages, then okay. I'd just as soon accept (string, SomeSyntaxCheckMetadataProto)
(as bytes, not C++ types) and then use optional string
and SomeSyntaxCheckMetadataProto
as fields in the Request proto.
And before you ask, if you want to use threads with the non-RPC compiler API, you will want to use a ring buffer of string of serialized bytes, and a counting semaphore. Otherwise, just use a single static string.
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.
Ok, that all sounds about right and I had expected as much. I don't know why I don't think I actually considered the DLL bounds until you said that much though, but that would be extremely useful. The same should be done for libEGM, then people could redistribute it. Hell, they could make a GMS extension using libEGM to load GMKs. The more third party tools that use our stuff, the more developers we will have that want to help maintain our stuff.
Update of the ENIGMA submodule which includes fixes for the default build that does not contain GRPC. enigma-dev/enigma-dev#1724
This pull request attempts to address issues like #1723 which stem from #1292. Old protocol buffer's compiler can not even handle the RPC syntax. Since we aren't requiring GRPC yet in the default LGM/ENIGMA, we should not run protoc on the server proto.
What I've done here is readd my filter-out that fundies had removed in #1292. I also split the server RPC interface's compiler buffers out into a new
compiler.proto
file since emake does depend on that (just not the RPC stuff).I tested locally myself to make clean with and without
CLI_ENABLE_SERVER=TRUE
defined. When it's not passed,server.proto
is not passed to protoc at all, and no codegen is produced. However, emake does continue to work andcompiler.proto
is built. When server mode is enabled, thenserver.proto
is passed to protoc, and the codegen is generated. I tested the emake server against the latest RGM release executable. Also, in both cases, LateralGM was able to communicate with compileEGMf. So this should be good to go once it passes.