Improve Lua 5.1 compatibility, lint for 5.1 issues #101
Conversation
| @@ -21,8 +21,8 @@ local utils = require("packages.markdown.utils") | |||
| local function checkAstSemver(version) | |||
| -- We shouldn't care the patch level. | |||
| -- The Pandoc AST may change upon "minor" updates, though. | |||
| local major, minor = table.unpack(version) | |||
| local expected_major, expected_minor = table.unpack(Pandoc.API_VERSION) | |||
| local major, minor = pl.utils.unpack(version) | |||
There was a problem hiding this comment.
There are subtle differences between table.unpack and pl.utils.unpack (which, last time I looked, called the former under the hood), but it doesn't seem we are in such a situation, are we?
| @@ -84,7 +84,7 @@ local function pandocAstParse(element) | |||
| if element.t then | |||
| if Pandoc[element.t] then | |||
| if HasMultipleArgs[element.t] then | |||
| addNodes(out, Pandoc[element.t](table.unpack(element.c))) | |||
| addNodes(out, Pandoc[element.t](pl.utils.unpack(element.c))) | |||
There was a problem hiding this comment.
This PR raises a conflict somewhere near these lines -- I realize it was made on a former version, probably 1.4.0 from June 2023. Can you possibly rebase your changes on current master (= 1.5.0 at this date).
No urge - I still have to manage having a SILE build based on Lua 5.1.
There was a problem hiding this comment.
Just to confuse the issue, since we're loading compat53 the table.pack() function is actually being patched and/or provided behind the scenes. I've been trying not to rely on that where possible and explicitly using pl.utils.<func> or other explicit calls where possible in the hopes that someday we don't need a monkey-patched set of Lua internal functions. The side-effects of compat53 are not nearly as egregious as std were (so glad we got rid of that!) but sometimes they are still unexpected, especially when 3rd party dependencies don't expect it.
There was a problem hiding this comment.
Noted.
Here I suspect (without having checked) that it would perhaps work even if we didn't have compat53, as we load jgm's lunamark or djot library, and they also have their own compatibility shims...
There was a problem hiding this comment.
Welcome to Lua. It's shims all the way down.
| @@ -15,6 +15,7 @@ description = { | |||
| } | |||
| dependencies = { | |||
| "lua >= 5.1", | |||
| "penlight >= 1.13.0", -- use of kpairs requires newer Penlight than transitive dependency possibly provided by SILE | |||
There was a problem hiding this comment.
As noted in #78, the markdown.sile module does not use kpairs directly in its code, so this shouldn't be needed here.
As an aside note, modules preinstalled with SILE (penlight, lpeg, etc.), as of 0.14.x, do no show in luarocks list. It implies that any additional dependency added here will always be installed regardless of what SILE provides. I am not sure this is the way to go (if so, I'd need to add lpeg, but version ranges are problematic then, as it would mean other parts of SILE would have to be compatible with whatever version the module overloaded...)
There was a problem hiding this comment.
As an aside note, modules preinstalled with SILE (penlight, lpeg, etc.), as of 0.14.x, do no show in
luarocks list. It implies that any additional dependency added here will always be installed regardless of what SILE provides.
This is only partially true. It will be true if SILE was configured and built --without-system-luarocks in which case it bundled vendored copies of them in it's own path. Otherwise --with-system-luarocks should interact nicely with luarocks including luarocks list. Just remember you do have to specify the Lua interpreter version to list for if it is not the default one LuaRocks is configured to use.
There was a problem hiding this comment.
We should probably add some sort of SILE subcommand for running LuaRocks with a matching version. sile-typesetter/sile#1947
There was a problem hiding this comment.
Interesting, thanks for the explanation.
For the record, the route I took here was:
FROM docker.io/library/archlinux:latest
RUN pacman --needed --noconfirm -Syu
RUN pacman --noconfirm -S luarocks git
RUN pacman --noconfirm -Suy sile # it installed SILE v0.14.14 (LuaJIT 2.1.1702233742)
RUN luarocks list --lua-version=5.1
Typically, lpeg and penlight did not show as installed.
It might not be obvious for people installing pre-packaged versions of SILE as here (i.e. not building it themselves), how it was configured and built.
Usually I build my own version of SILE, so I'll try the above-mentioned option at some point.
(Here I just wanted a quick'n fresh ready-to-go docker image for testing)
There was a problem hiding this comment.
So the Dockerfile you show in that comment will show LPEG and Penlight because the Arch LInux system package for sile does use --with-system-luarocks. For convenience of people not knowing how to install LuaRocks to their system or not having a matching VM or whatever else, the default ./configure option is actually --without-system-luarocks. That default is also what the 'official' SILE Docker image uses since it is a source build and does not use the Arch Linux packaged Lua libraries (even though it is using Arch as a base too). It uses the exact versions specified in the rockspec installed in the vendored location.
We probably do need to rethink how that interacts with 3rd party modules like yours. As an example, if you used LPEG in your extension directly it would seem to make sense to add it to your rockspec, but that would actually make it harder to install or result in a duplicate installation where one of them is unnecessary. SILE will always provide LPEG whether on the system where LuaRocks also know about it or locally vendored where your module will be able to use it anyway. That isn't very evident or documented however.
There was a problem hiding this comment.
Note to self: rebuilding wholly the docker image clean (i.e. real latest stuff, no previous cached artifacts from earlier docker builds, all pruned) = it worked indeed. Prolly I had some stuff cached.
There was a problem hiding this comment.
Also note since I've been reminding of this myself recently: the Dockerfile copies the entire source tree from your current Git project into the builder container. This can bite you if you have something configured and/or built or the host system. Ideally building Docker images would involve a pretty aggressive git clean -dxff before configuring just with enabling developer but not actually building anything on the host system before make docker.
I've wondered after this bit me a couple times if the Dockerfile should be forcing this cleanup after it copies the sources...
See related commits in Omikhleia/markdown.sile#101 Co-authored-by: Caleb Maclennan <caleb@alerque.com>
See related commits in Omikhleia/markdown.sile#101 Co-authored-by: Caleb Maclennan <caleb@alerque.com>
|
I think I have taken into account the minimal things from this PR in #102 |
|
Closing -- Tests ok with #102 and SILE 0.14.14 running on LuaJIT (from SILE docker images) |
I was trying to use this for something and ran into trouble. It turns out at the very least one issue was my SILE runs on LuaJIT and this library was making assumptions about access to Lua 5.4 interfaces. SILE provides shims for things like that it wants to use that were introduced to Lua later, and also grantees access to Penlight and some other libraries. By calling those libraries we know SILE provides we should be able to make this 5.1 compatible.
Also Luacheck provides upstream support for SILE's builtin variables. This makes use of that and then lints against 5.1 (min) instead of 5.4 (max).