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

feat(core): Add lua modules #273

Merged
merged 2 commits into from
Sep 8, 2022
Merged

Conversation

dranikpg
Copy link
Contributor

@dranikpg dranikpg commented Sep 7, 2022

Add the following modules to the lua interpreter:

  • cjson
  • cmsgpack
  • struct
  • bit

and registers them in the globals table.

Helps with #184

@dranikpg
Copy link
Contributor Author

dranikpg commented Sep 7, 2022

I've chosen to embed the module sources at src/redis/lua because:

  • the build process of cjson/cmsgpack would have to be patched significantly
  • bit/struct are standalone and would require a build solution on their own

I guess, a single target with a single build file at src/redis/lua would be enough? The other option would be fully subdividing everything and listing all the modules in a cmake variable.

@romange
Copy link
Collaborator

romange commented Sep 8, 2022

Looks good.

  1. A single target is fine.
  2. We need tests inside interpreter_test that cover the functionality of all modules
  3. If you had to change the original files - please add a readme explaining in a few lines what had to be changed or provide a patch snippets (whatever is simpler).

@romange romange self-requested a review September 8, 2022 04:47
@dranikpg
Copy link
Contributor Author

dranikpg commented Sep 8, 2022

I've fixed the registration process, because lua 5.2 changed the way modules are registered. (lual_register became deprecated https://www.lua.org/manual/5.2/manual.html, the new luaL_newlib doesn't make it global upon registration to be used with require instead)

The modules show quite a lot of warnings (about 20) during compile (mainly sign-compare, implicit-fallthrough, misleading-identation). Should I fix them or just suppress them in the sub-build?

@romange
Copy link
Collaborator

romange commented Sep 8, 2022

just opress them - see target_compile_options in redis CMakeLists

@dranikpg dranikpg marked this pull request as ready for review September 8, 2022 10:21
@romange
Copy link
Collaborator

romange commented Sep 8, 2022

do you mind squashing the commits into a single one?

src/redis/lua/CMakeLists.txt Outdated Show resolved Hide resolved
@dranikpg
Copy link
Contributor Author

dranikpg commented Sep 8, 2022

forgot to pull my merge 😓

@romange romange merged commit a43badf into dragonflydb:main Sep 8, 2022
@dranikpg dranikpg deleted the lua-modules branch December 25, 2022 12:00
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