WIP: Build without Lua scripting via USE_LUA (#1204) #1245

neomantra · 2024-11-01T15:45:34Z

Adds the ability to disable Lua scripting using the build
configuration USE_LUA. This will prevent the building
of the Lua static library and will keep Lua and most Lua
scripting internals out of the build.

This approach strived to minimize the change surface, stubbing out
keys requried functions and otherwise ifdefing out large swathes of code.

The base Lua scripting commands like EVAL remain intact, but reply
with an error message Lua scripting disabled. INFO commands
still include scripting statistics to prevent breaking any DevOps scripts, etc.

There are decisions to make:

What do with EVAL commands, etc? Currently, the command still exists but replies with an Error.
What to do with testing infrastructure? EVAL is sprinkled throughout. grep -or EVAL tests/unit | wc -l ==> 125. Do we break tests into files with EVAL and ones without?

neomantra · 2024-11-01T16:53:37Z

I see the Test Tags system now and am using that to constrain tests which utilize Lua.

zuiderkwast · 2024-11-01T21:29:38Z

What do with EVAL commands, etc? Currently, the command still exists but replies with an Error.

This sounds right to me.

What to do with testing infrastructure? EVAL is sprinkled throughout. grep -or EVAL tests/unit | wc -l ==> 125. Do we break tests into files with EVAL and ones without?

I see the Test Tags system now and am using that to constrain tests which utilize Lua.

Test tag sounds like the best solution. Is it feasible? (If not, maybe we can just run a few selected test suites in the CI for the build without Lua.)

codecov · 2024-11-01T21:31:20Z

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.65%. Comparing base (a102852) to head (51a4429).
Report is 6 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/object.c	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1245      +/-   ##
============================================
- Coverage     70.70%   70.65%   -0.05%     
============================================
  Files           114      114              
  Lines         63157    63159       +2     
============================================
- Hits          44653    44628      -25     
- Misses        18504    18531      +27

Files with missing lines	Coverage Δ
src/defrag.c	`84.34% <100.00%> (ø)`
src/eval.c	`57.15% <100.00%> (+0.09%)`	⬆️
src/function_lua.c	`99.19% <ø> (ø)`
src/functions.c	`95.71% <ø> (ø)`
src/lazyfree.c	`86.11% <ø> (ø)`
src/script_lua.c	`90.42% <ø> (ø)`
src/server.c	`87.70% <ø> (-0.04%)`	⬇️
src/server.h	`100.00% <ø> (ø)`
src/object.c	`79.15% <0.00%> (ø)`

... and 12 files with indirect coverage changes

neomantra · 2024-11-01T21:39:47Z

Although I perhaps wanted needs:lua, I stuck with the tag scripting. I went through and found all the spots where eval or call were used and where the tests were failing from needing Lua in any way.

It passes tests with both USE_LUA=on and USE_LUA=off on my Mac, but I was having problems with full make check on my Linux boxes... but I had the same problem on unstable : spewing Port <num> was already busy, trying another port.... Maybe related to my IPv4-only network, will look deeper.

See this commit for the tag changes.

zuiderkwast · 2024-11-01T22:21:48Z

Although I perhaps wanted needs:lua, I stuck with the tag scripting.

I guess needs:lua is better. Scripting is equivalent to Lua now, but we're considering more languages for scripting (such as WASM, #387) so scripting may not always imply Lua in the future.

neomantra · 2024-11-01T23:30:05Z

I hadn't re-run Lua-enabled after fixing some Lua-disabled tests.... this last commit corrects that with the proper empty results.

The expression `dictSize(evalScriptsDict())` is used a couple times for memory reporting. `dictSize` is a macro without NULL checking and used within a `FMTARG` macro in `server.c`. Overall, this makes it challenging to stub out when building without the Lua engine. This change lifts that expression up to its own function `evalScriptsDictSize`. Signed-off-by: Evan Wies <[email protected]>

Adds the ability to disable Lua scripting using the build conifguration `USE_LUA`. This will prevent the building of the Lua static library and will keep Lua and most Lua scripting internals out of the build. This approach strived to minimize the change surface, stubbing out keys requried functions and otherwise `ifdef`ing out large swathes of code. The base Lua scripting commands like `EVAL` remain intact, but reply with an error message `Lua scripting disabled`. `INFO` commands still include scripting statistics to prevent breaking any DevOps scripts, etc. Signed-off-by: Evan Wies <[email protected]>

Adds LUA_TEST_FLAGS to Makefile to propagate filtering of scripting tests. Expands "scripting" tag to all uses of Lua. Signed-off-by: Evan Wies <[email protected]>

Signed-off-by: Evan Wies <[email protected]>

* fix ci.yml whitespace, from yamlint * fix defrag.c, from -Werror Signed-off-by: Evan Wies <[email protected]>

neomantra · 2024-11-02T13:54:10Z

Sorry for the repeated build failures -- glad the CI catches it :)

I was missing some targets and hence some Lua coverage. I still cannot do a full test on Linux with unstable. [I hoped #1243 would help, but it didn't. Looked a little at the find_available_port etc and it does feel like a port race.]

* Make clang linter happy in defrag.c * Tag a test as scripting in memefficiency.tcl Signed-off-by: Evan Wies <[email protected]>

madolson · 2024-11-04T15:50:35Z

We discussed this in an online meeting. The core team was OK with being able to remove LUA from the server, but we would like to make it more pluggable and modular. Instead of just not installing LUA, we would like to make it so that LUA is not added as an engine (similar to how functions allows you to specify the engine type) and so when you try to run EVAL you get an error like "no default engine loaded". In the future folks can use either Lua 5.4 or LuaJIT.

Signed-off-by: Evan Wies <[email protected]>

neomantra · 2024-11-04T17:15:59Z

I made that error message change per your suggestion. Some of that work (modular engine system) is beyond the scope of this PR, but I think it is a sensible approach. At least the USE_LUA injections and scripting test tagging here helps show the surface and some cross-cutting concerns.

If you have specifics to add that are low-hanging, I could explore it.

zuiderkwast · 2024-11-04T17:52:21Z

There is partial logic for a modular engine system. EVAL currently accepts a shebang that is intended to be able to select the scripting engine in the future.

eval.c-365-        /* Verify lua interpreter was specified */
eval.c:366:        if (strcmp(parts[0], "#!lua")) {
eval.c-367-            if (err) *err = sdscatfmt(sdsempty(), "Unexpected engine in script shebang: %s", parts[0]);
eval.c-368-            sdsfreesplitres(parts, numparts);
eval.c-369-            return C_ERR;
eval.c-370-        }

Here we want the module API to provide a way for modules to provide a scripting engine. I created this issue: #1261.

hpatro

Looks like we allow loading of functions in case the engine doesn't support any engine. Would that cause any confusion? Same thing applies for functions arriving via replication stream.

hpatro · 2024-11-04T20:56:17Z

src/eval.c

+void evalCommand(client *c) {
+    addReplyError(c, "No default engine loaded");
+}
+
+void evalRoCommand(client *c) {
+    addReplyError(c, "No default engine loaded");
+}
+
+void evalShaCommand(client *c) {
+    addReplyError(c, "No default engine loaded");
+}
+
+void evalShaRoCommand(client *c) {
+    addReplyError(c, "No default engine loaded");
+}
+
+void scriptCommand(client *c) {
+    addReplyError(c, "No default engine loaded");
+}


We would need test to cover these scenarios. So, we might need separate test suite/tag for this particular flow.

hpatro · 2024-11-04T21:00:28Z

src/defrag.c

    dictDefragFunctions defragfns = {
        .defragAlloc = activeDefragAlloc,
        .defragKey = (dictDefragAllocFunction *)activeDefragSds,
-        .defragVal = (val_type == DEFRAG_SDS_DICT_VAL_IS_SDS       ? (dictDefragAllocFunction *)activeDefragSds
-                      : val_type == DEFRAG_SDS_DICT_VAL_IS_STROB   ? (dictDefragAllocFunction *)activeDefragStringOb
-                      : val_type == DEFRAG_SDS_DICT_VAL_VOID_PTR   ? (dictDefragAllocFunction *)activeDefragAlloc
+        .defragVal = (val_type == DEFRAG_SDS_DICT_VAL_IS_SDS     ? (dictDefragAllocFunction *)activeDefragSds
+                      : val_type == DEFRAG_SDS_DICT_VAL_IS_STROB ? (dictDefragAllocFunction *)activeDefragStringOb
+                      : val_type == DEFRAG_SDS_DICT_VAL_VOID_PTR ? (dictDefragAllocFunction *)activeDefragAlloc
+#ifdef USE_LUA
                      : val_type == DEFRAG_SDS_DICT_VAL_LUA_SCRIPT ? (dictDefragAllocFunction *)activeDefragLuaScript
+#endif
                                                                   : NULL)};


Might be just me. But ifdef within the struct declaration gives a hit to the readability. We can consider refactoring this.

Maybe like?

dictDefragFunctions defragfns = { .defragAlloc = activeDefragAlloc, .defragKey = (dictDefragAllocFunction *)activeDefragSds, .defragVal = (val_type == DEFRAG_SDS_DICT_VAL_IS_SDS ? (dictDefragAllocFunction *)activeDefragSds : val_type == DEFRAG_SDS_DICT_VAL_IS_STROB ? (dictDefragAllocFunction *)activeDefragStringOb : val_type == DEFRAG_SDS_DICT_VAL_VOID_PTR ? (dictDefragAllocFunction *)activeDefragAlloc : NULL)}; #ifdef USE_LUA if (val_type == DEFRAG_SDS_DICT_VAL_LUA_SCRIPT) dictDefragFunctions.defragVal = (dictDefragAllocFunction *)activeDefragLuaScript; #endif

neomantra force-pushed the nm-1204 branch from 00ecb20 to 7853592 Compare November 1, 2024 15:49

neomantra mentioned this pull request Nov 1, 2024

[NEW] Disable LUA integration during build #1204

Open

neomantra force-pushed the nm-1204 branch from a8cc030 to 7605730 Compare November 2, 2024 01:24

neomantra added 7 commits November 2, 2024 08:44

Update test coverage for non-Lua builds

7f60c99

Adds LUA_TEST_FLAGS to Makefile to propagate filtering of scripting tests. Expands "scripting" tag to all uses of Lua. Signed-off-by: Evan Wies <[email protected]>

Add test-without-lua build to ci.yml

309a758

Signed-off-by: Evan Wies <[email protected]>

Fix test results from previous commit

536a63f

Signed-off-by: Evan Wies <[email protected]>

Fix test results for runtest-moduleapi

ce8efa3

Signed-off-by: Evan Wies <[email protected]>

Fix errors found in CI

489db63

* fix ci.yml whitespace, from yamlint * fix defrag.c, from -Werror Signed-off-by: Evan Wies <[email protected]>

neomantra force-pushed the nm-1204 branch from 719f9e7 to 489db63 Compare November 2, 2024 12:45

More CI-discovered fixes

dc246d9

* Make clang linter happy in defrag.c * Tag a test as scripting in memefficiency.tcl Signed-off-by: Evan Wies <[email protected]>

zuiderkwast linked an issue Nov 4, 2024 that may be closed by this pull request

[NEW] Disable LUA integration during build #1204

Open

Update Lua-less EVAL error message per review

51a4429

Signed-off-by: Evan Wies <[email protected]>

hpatro reviewed Nov 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Build without Lua scripting via USE_LUA (#1204) #1245

WIP: Build without Lua scripting via USE_LUA (#1204) #1245

neomantra commented Nov 1, 2024

neomantra commented Nov 1, 2024

zuiderkwast commented Nov 1, 2024

codecov bot commented Nov 1, 2024 •

edited

Loading

neomantra commented Nov 1, 2024 •

edited

Loading

zuiderkwast commented Nov 1, 2024

neomantra commented Nov 1, 2024

neomantra commented Nov 2, 2024

madolson commented Nov 4, 2024

neomantra commented Nov 4, 2024

zuiderkwast commented Nov 4, 2024

hpatro left a comment

hpatro Nov 4, 2024 •

edited

Loading

hpatro Nov 4, 2024

neomantra Nov 5, 2024

WIP: Build without Lua scripting via USE_LUA (#1204) #1245

Are you sure you want to change the base?

WIP: Build without Lua scripting via USE_LUA (#1204) #1245

Conversation

neomantra commented Nov 1, 2024

neomantra commented Nov 1, 2024

zuiderkwast commented Nov 1, 2024

codecov bot commented Nov 1, 2024 • edited Loading

Codecov Report

neomantra commented Nov 1, 2024 • edited Loading

zuiderkwast commented Nov 1, 2024

neomantra commented Nov 1, 2024

neomantra commented Nov 2, 2024

madolson commented Nov 4, 2024

neomantra commented Nov 4, 2024

zuiderkwast commented Nov 4, 2024

hpatro left a comment

Choose a reason for hiding this comment

hpatro Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

hpatro Nov 4, 2024

Choose a reason for hiding this comment

neomantra Nov 5, 2024

Choose a reason for hiding this comment

codecov bot commented Nov 1, 2024 •

edited

Loading

neomantra commented Nov 1, 2024 •

edited

Loading

hpatro Nov 4, 2024 •

edited

Loading