-
Notifications
You must be signed in to change notification settings - Fork 29
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
Refactor the CLI so it can be used by other libraries #11
Conversation
OK if you want to see how this works look at https://github.com/baconpaul/sst-elementary-bindings where i build a custom CLI which adds my callbacks (and works!) to build you need to check out and submodule that but then you need to pull this PR branch for the libs/elementary submodule. But the key is: sst-elementary-bindings makes its own CLI application, but the only code I needed was #include "CLIEngine.h"
#include "sst-elementary-bindings.h"
int main(int argc, char **argv)
{
return ElementaryCLIMain(argc, argv, [](auto &a){
sst::elementary::registerSSTElementaryBindings<float>(a);
});
} |
Oh I want to make a small cmake change. The cli library should be static |
Alright pushed up that change! Let me know what you think With this I can definitely pull in the filters and wave shapers from surge easily. |
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.
Yea love this idea, and it's really a clean change to enable this flexibility for devs.
One high level thought: do you think it'd be worth nudging the Benchmark functionality into the same structure while we're here? The benchmark cli tool is the same thing but instead of writing to the audio driver it loops the Runtime::process
step on the main thread so you can profile it. Or do you think it's sufficient just to have the realtime output?
To your bigger question : now I look at it yes it certainly looks like benchmark should get the same change! I hadn’t even read that code tbh. But it indeed seems symmetric. |
Hey @baconpaul, just pushed some changes to fix up the rest of the benchmark split as we chatted about in Discord, and I tweaked a bit else to satisfy my own aggressive nitpicking, I hope that's ok! Let me know if this looks good on your end. |
The CLI is a useful tool and if you are developing external nodes you want to use it. This commit makes it so that is easier. It takes the following approach 1. Split the CLI into a libcli which exports a function previously known as main with an added callback, and a new executable which just calls that 2. Have the new function have a registerCallback which you can pass 3. Add cmake targets for the library only 4. Add a couple of inlines which avoid duplicated symbols from headers so you can get this all to work As a result, a library can write a 'test' cli which uses the entire current CLI with one function injected with a couple of lines of cmake, and the internal CLI still works just with one extra initialization time call to an empty function.
a52acca
to
f82b812
Compare
🙌 thank you @baconpaul! Elementary's first public contributor :D |
The CLI is a useful tool and if you are developing external nodes you want to use it. This commit makes it so that is easier.
It takes the following approach
As a result, a library can write a 'test' cli which uses the entire current CLI with one function injected with a couple of lines of cmake, and the internal CLI still works just with one extra initialization time call to an empty function.