Scripting Engine Debugger Support
This PR introduces the support for implementing remote debuggers in scripting engines modules.
The module API is extended with scripting engines callbacks and new functions that can be used by scripting engine modules to implement a remote debugger.
Most of the code that was used to implement the Lua debugger, was refactored and moved to the scripting_engine.c file, and only the code specific to the Lua engine, remained in the debug_lua.c file.
The SCRIPT DEBUG (YES|NO|SYNC) command was extend with an optional parameter that can be used to specify the engine name, where we want to enable the debugger. If no engine name is specified, the Lua engine is used to keep backwards compatibility.
In src/valkeymodule.h we see the module API changes. And in the helloscripting.c file we can see how to implement a simple debugger for the dummy HELLO scripting engine.
Codecov Report
:x: Patch coverage is 46.84685% with 354 lines in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 72.37%. Comparing base (7e0b3bb) to head (28c9b2e).
:warning: Report is 3 commits behind head on unstable.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| src/scripting_engine.c | 48.35% | 236 Missing :warning: |
| src/lua/debug_lua.c | 43.07% | 74 Missing :warning: |
| src/module.c | 20.83% | 19 Missing :warning: |
| src/eval.c | 50.00% | 9 Missing :warning: |
| src/valkey-cli.c | 0.00% | 9 Missing :warning: |
| src/lua/engine_lua.c | 76.00% | 6 Missing :warning: |
| src/server.c | 66.66% | 1 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## unstable #1701 +/- ##
============================================
- Coverage 72.58% 72.37% -0.22%
============================================
Files 128 128
Lines 71326 71626 +300
============================================
+ Hits 51772 51839 +67
- Misses 19554 19787 +233
| Files with missing lines | Coverage Δ | |
|---|---|---|
| src/commands.def | 100.00% <ø> (ø) |
|
| src/server.c | 88.43% <66.66%> (ø) |
|
| src/lua/engine_lua.c | 86.48% <76.00%> (-1.64%) |
:arrow_down: |
| src/eval.c | 87.91% <50.00%> (-1.33%) |
:arrow_down: |
| src/valkey-cli.c | 54.53% <0.00%> (-1.69%) |
:arrow_down: |
| src/module.c | 9.83% <20.83%> (+0.04%) |
:arrow_up: |
| src/lua/debug_lua.c | 37.11% <43.07%> (-2.17%) |
:arrow_down: |
| src/scripting_engine.c | 53.60% <48.35%> (-19.83%) |
:arrow_down: |
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
Postponing until 9.0.
We talked about this in a core team meeting. We said that nobody uses the Lua debugger and that we could basically deprecate it. No meed to make this large change to extend the debugger support. The main reason to do it is to avoid breaking it if we move Lua to a module?
Correct. This is required if we want to keep the debugger feature for Lua when moving Lua to a module. I'm also fine to deprecate the debugger feature, but if that is the case we should deprecate it in 9.0 and raise somekind of warning to the user when he tries to use the debugger.
@zuiderkwast I've updated the PR, and addressed your comments. Added new tests and fixed some bugs that appeared in the tests.
@zuiderkwast I've updated this PR with the current unstable branch. We should get this PR merged so I can open the PR that moves the lua engine into a module.
@madolson do you want to review the module API additions?
I believe we can merge this without waiting for the others to review it, so we can unblock the other work. If there are any concerns regarding the module API, we have time to change it until 9.1.
It needs some rebase now and bump the scripting engine API version to 3?
@zuiderkwast yes, I've fixed the merge conflicts and updated the code accordingly. Run a few more tests and looks good. Will merge this now.