[
https://issues.apache.org/jira/browse/THRIFT-6189?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Jens Geyer updated THRIFT-6189:
-------------------------------
Description:
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._
was:
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.
> 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
>
> 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)
