Skip to content
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

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

Open
wants to merge 7 commits into
base: unstable
Choose a base branch
from

Conversation

neomantra
Copy link

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
Copy link
Author

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

@zuiderkwast
Copy link
Contributor

  • 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.)

Copy link

codecov bot commented Nov 1, 2024

Codecov Report

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

Project coverage is 70.66%. Comparing base (0d7b234) to head (7605730).

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.66%   -0.05%     
============================================
  Files           114      114              
  Lines         63157    63159       +2     
============================================
- Hits          44654    44629      -25     
- Misses        18503    18530      +27     
Files with missing lines Coverage Δ
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% <ø> (ø)
src/server.h 100.00% <ø> (ø)
src/object.c 79.15% <0.00%> (ø)

... and 14 files with indirect coverage changes

@neomantra
Copy link
Author

neomantra commented Nov 1, 2024

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
Copy link
Contributor

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
Copy link
Author

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]>
 * fix ci.yml whitespace, from yamlint
 * fix defrag.c, from -Werror

Signed-off-by: Evan Wies <[email protected]>
@neomantra
Copy link
Author

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.]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants