[
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)
