[jira] [Commented] (THRIFT-6189) Add a GitHub Actions CI job for the Lua library

["Jens Geyer (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/THRIFT-6189?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18116677#comment-18116677 ] 

Jens Geyer commented on THRIFT-6189:
------------------------------------

The PR implements this with three departures from the description above:

* *The {{\-\-without-lua}} flags stay.* {{CONFIG_ARGS_FOR_LIBS}} in build.yml carries a {{\-\-without-X}} for every binding, and each job switches its own back on; the new {{lib-lua}} job does that for Lua. In sca.yml the list is the bindings without static analysis, and Lua is still one of them. Calling the flags stale was wrong.
* *All three scripts in {{lib/lua/test}} run under {{make check}},* not only {{test_recursion_depth.lua}}. {{test_http_headers.lua}} and {{test_frame_and_string_limits.lua}} landed after this was filed.
* *The Lua floor moves from 5.2 to 5.3.* The stand-ins those scripts preload for the C modules use 5.3's bitwise operators and {{string.pack}}, so on 5.2 none of them parses, and the new check target would have made {{make check}} fail on every 5.2 build. configure now requires 5.3, the focal image installs lua5.3 instead of lua5.2, and LANGUAGES.md gives 5.3 to 5.4 instead of 5.1.5 to 5.4. *This drops Lua 5.2 for autotools builds:* on a 5.2 system configure now reports the Lua library as not built.

Because the {{lib/lua/test}} scripts never load the compiled C modules, the job also runs the Lua client against the Lua server through {{test/test.py}}, over the three transports and three protocols {{test/tests.json}} lists for Lua.

_Drafted with AI assistance (Claude Opus 5); reviewed and posted by Jens Geyer._

> Add a GitHub Actions CI job for the Lua library
> -----------------------------------------------
>
>                 Key: THRIFT-6189
>                 URL: https://issues.apache.org/jira/browse/THRIFT-6189
>             Project: Thrift
>          Issue Type: New Feature
>          Components: Build Process, Lua - Library
>            Reporter: Jens Geyer
>            Priority: Major
>          Time Spent: 10m
>  Remaining Estimate: 0h
>
> No CI job builds or tests the Lua binding.
> .github/workflows/build.yml has jobs for php, go, java-kotlin, netstd, haxe, rust, python, nodejs, cpp and ruby, but none for Lua, and both build.yml (line 32) and sca.yml (line 29) pass --without-lua in their shared CONFIG_ARGS_FOR_LIBS. The docker.yml workflow only builds and validates the official images; it runs no language test suite. So a change to lib/lua gets no automated coverage at all.
> lib/lua ships enabled by default -- configure.ac line 276 has AX_THRIFT_LIB(lua, [Lua], yes) and lib/Makefile.am line 88 has SUBDIRS += lua -- so this is code that reaches users with no regression coverage behind it.
> h3. The historical reason for --without-lua no longer applies
> The three ubuntu Dockerfiles carry the comment "need to update our luasocket code, lua doesn't have luaL_openlib any more". That update happened: THRIFT-4386 (eb684d348, 2024-07-28) added the version switch, so the C sources now call luaL_setfuncs on Lua >= 5.2 and luaL_register only below it. The comment and the --without-lua flags are stale and should go with this change.
> h3. Verified in the thrift:jammy image before filing
> * All six C sources under lib/lua/src compile clean against Lua 5.4.4.
> * ./configure --with-lua reports "Building Lua Library ......... : yes", using /usr/bin/lua, Lua 5.4.4.
> * make -C lib/lua builds all four shared libraries (libluasocket, liblualongnumber, libluabpack, libluabitwise) without warnings.
> * lib/lua/test/test_recursion_depth.lua reports 30 passed, 0 failed, after generating its stubs.
> * test/lua/test_basic_server.lua and test_basic_client.lua run against each other successfully (client exits 0) over the default buffered/binary combination.
> h3. Most of what a job needs already exists
> * build/docker/ubuntu-{jammy,noble}/Dockerfile install lua5.4 and liblua5.4-dev; ubuntu-focal installs lua5.2 and lua5.2-dev. configure requires >= 5.2, so all three are usable, and /usr/bin/lua exists as an alternative (the launchpad bug referenced in the Dockerfile comment affected 5.3, not 5.4).
> * configure.ac already locates the interpreter, headers and libraries via AX_PROG_LUA, AX_LUA_HEADERS and AX_LUA_LIBS.
> * test/tests.json already carries a complete lua entry -- client, server, transports (buffered, framed, http) and protocols (binary, compact, json) -- so the cross-test wiring exists and has simply never been exercised. Its "TODO: Add dll to LUA_CPATH" note is also stale; the built libraries load from ../../lib/lua/.libs without further work.
> h3. One prerequisite, the same shape as THRIFT-6172 for Dart
> lib/lua/Makefile.am defines no check target. "make -C lib/lua check" answers "Nothing to be done for 'check-am'", which means lib/lua/test/test_recursion_depth.lua has never been executed by any build since it was added. A check-local that generates the stubs with $(THRIFT) and runs the test should land before, or with, the job -- otherwise the job goes green without running the one test the binding has.
> For the same reason the job should assert that configure actually enabled the binding, for instance by grepping the configure summary for "Building Lua Library ......... : yes". configure silently falls back to disabled when it cannot find Lua, so a job that only runs "make check" would pass while testing nothing.
> h3. Worth knowing when writing the job
> The Lua server socket's default timeout is 1000, and lib/lua/src/usocket.c treats it as milliseconds (socket_wait computes timeout/1000 seconds). So test_basic_server.lua exits with "Timeout" roughly one second after start if nothing connects. The cross-test runner is fine with this because test/crossrunner/run.py polls the port every 0.1 s and starts the client as soon as it is open, using the tests.json "delay" only as an upper bound -- but a fixed sleep before the client would kill the server first.
> Compare THRIFT-6002 (netstd), THRIFT-6003 (haxe), THRIFT-6170 (D), THRIFT-6171 (Erlang) and THRIFT-6172 (Dart), which are the same gap in other bindings.
> _Drafted with AI assistance (Claude Opus 5); reviewed and filed by Jens Geyer._



--
This message was sent by Atlassian Jira
(v8.20.10#820010)