Generate grpc_server.h/grpc_server.cpp, support selectively building plugins #1776
Conversation
| builder.RegisterService(&_camera_service); | ||
| #endif | ||
|
|
||
| #ifdef CAMERA_SERVER_ENABLED |
There was a problem hiding this comment.
It looks a bit not elegant, maybe the 3 lines can be wrapped into some macro Call like REGISTER_IF_ENABLE(…) but it’s auto generated so also does not matter so much.
Or I wonder if it’s possible to somehow move it within the plugin header🤔
There was a problem hiding this comment.
Right, it's not pretty but it's just auto-generated, and at least it's not out of sync like it often was in the past because someone would add a plugin and they and us maintainers would both forget to update it.
The plugin header includes are not there at all, so not sure how it would be there.
There was a problem hiding this comment.
Yeah I agree. The only way i could imagine moving this into the plugin would be through lazy plugin somehow(or passing builder around), am not sure if that’s possible, is also rather cosmetic and not so important.
There was a problem hiding this comment.
That might actually work. Any reason why builder is not a member variable? then each service could take a pointer to the builder in their constructor, and register/stop within the plugins like we do with server plugins impl. Anyways, can come in later if we want to do it.
There was a problem hiding this comment.
I don't quite understand how you imagine it. Feel free to make a PR or code example to show it. This can also happen later, of course.
There was a problem hiding this comment.
Something like this https://diffy.org/diff/43d4582733066
There was a problem hiding this comment.
@devbharat I think what you suggest makes sense, thanks!
@JonasVautherin can you check that change in diffy as well? You know this part of the code.
There was a problem hiding this comment.
Looks clean. Just wondering: is it fine to register a service in its own constructor?
There was a problem hiding this comment.
I was also not sure of that, but the unit test thing seems to work. Worst case we can have like a two step constructor that creates the service and registers it and then returns the object.
Perhaps not the best idea to put it directly in the constructor(they construct another service with it on the call here), although its in the body so everything else should already have been constructed.
Edit: Ah no, they just take a pointer to it, so its not 'so bad', but perhaps still better to avoid.
JonasVautherin
force-pushed
the
pr-plugin-list
branch
from
03b98a1 to
a78a2ed
Compare
This is refactoring around the way the plugins are built into the mavsdk library as well as the mavsdk_server executable. The changes contain the following: - Auto-generation of grpc_server.h and grpc_server.cpp from jinja2 templates. This means that the mavsdk_server is always in sync and new plugins don't have to be added to it manually anymore. - As a side effects this removes the ugly rewriting of CMakeLists.txt based on the auto-generated plugins and instead uses a separate plugins.txt file that is used throughout. - The cmake variable ENABLED_PLUGINS can be used to manually select the plugins to be built for users requiring to do so.
julianoes
force-pushed
the
pr-plugin-list
branch
from
a78a2ed to
5ded541
Compare
cmake/tools: generate grpc_server, list plugins
This is refactoring around the way the plugins are built into the mavsdk
library as well as the mavsdk_server executable.
The changes contain the following:
To use this, the enabled plugins can be passed at configure time:
FYI @devbharat