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

LuaJIT 5.2 compatibility discussion #239

Open
CoffeeFlux opened this issue Dec 26, 2024 · 12 comments
Open

LuaJIT 5.2 compatibility discussion #239

CoffeeFlux opened this issue Dec 26, 2024 · 12 comments

Comments

@CoffeeFlux
Copy link
Member

CoffeeFlux commented Dec 26, 2024

I'm creating this issue to try and centralize discussion on the topic, as it's become spread across a bunch of issues and PRs in multiple repos.

Current state of affairs

First, a summary of the situation as best I understand it:

  • Aegisub 3.2.2 uses LuaJIT with 5.2 compatibility enabled when running automation scripts
  • Over the past decade, many scripts have been written that all assume LuaJIT with 5.2 compat as the Lua engine
    • This is not a theoretical concern; most scripts are explicitly or implicitly dependent on 5.2-specific features or LuaJIT FFI
      • The most important 5.2 feature is probably the __ipairs metatable entry
        • Many of the built-in scripts rely on 5.2, via ipairs(subs), and many third party scripts have copied this pattern
      • DependencyControl (henceforth DepCtrl) depends on FFI to function, as well as 5.2 compat, and many of the most useful automation scripts assume DepCtrl is present
        • Outside of the AUR, to my knowledge no Linux distro ships DepCtrl, and how it interacts with system packaging is an unsolved problem
    • These scripts, for all practical purposes, will not be rewritten
  • Many Linux distros do not compile system LuaJIT with 5.2 compatibility and have patched 3.2.2 to remove the check
    • I'm unclear as to the particulars of why distros will not use a separate build for Aegisub; some clarity here would be helpful
    • This means automation scripts are almost all broken, which has resulted in bugs filed against Debian and Aegisub over the years
  • Aegisub 3.4.0 includes a build rewrite to Meson, which preserves the LuaJIT compatibility check
    • If a suitable LuaJIT version is not found, it builds against upstream as a subproject with a custom wrap
  • Multiple distros are looking to get 3.4.0 packaged but this issue is potentially a blocker

Current *nix distro status (to be updated)

Note: anything that says 3.3.x is probably based on the wangqr fork

  • Arch: 3.4.0 with the LuaJIT subproject
  • Debian: stable ships 3.2.2 with many patches, maintainer is looking to get 3.4.0 into unstable
  • OpenSUSE: 3.3.3
  • Fedora: Unknown
  • Ubuntu: 3.2.2
  • NixOS: ships 3.3.3

Competing priorities

In deciding how to handle this, I have a few different concerns:

  1. Making it as easy as reasonably possible for distros to package Aegisub
  2. Not shipping known-broken builds of Aegisub
  3. Not supporting known-broken builds of Aegisub
  4. Preserving, to the degree possible, functionality of the existing automation script ecosystem
  5. Not making it impossible to rely more heavily on DepCtrl in the future
  6. Not having issues opened against Aegisub due to distro patches

Possible paths forward

There is no perfect solution here, but a few options that have been brought up:

  1. Remove the 5.2 compatibility check on Linux
  2. Remove the LuaJIT check on Linux and support building against vanilla Lua 5.2
  3. Update all automation scripts to work with non-5.2 LuaJIT
  4. Disable automation scripts on Linux if a suitable LuaJIT isn't available
  5. Ship an AppImage/Flatpak and forget about distros (this isn't meant to be rude; it was someone's suggestion!)
  6. Try to make https://github.com/lunarmodules/lua-compat-5.3 work and update scripts accordingly
  7. Add a polyfill for __ipairs specifically and pray it doesn't break too many scripts

My view

This view is my own, not representative of arch1t3cht or anyone else

Any proposal that requires updating all automation scripts is dead in the water. There are god-knows how many scripts written in other communities, and even for the main TypesettingTools ones I'm functionally the only person who can commit changes, and I simply don't have the time or energy to put them all through the probably significant necessary rewrites. The only interesting subset of that, to me, is getting rid of DepCtrl's FFI usage by offering the functionality in Aegisub itself, especially now that at least on Windows we already have to bundle curl for the update checker.

My inclination is to say we should add a flag to allow building against either LuaJIT without the 5.2 flag or vanilla 5.2 (I haven't thought through which I prefer) on Linux, but disable user automation scripts if this is done, with the dropdown offering a dialog explaining why they are disabled. Hopefully, most distros will either offer a compatible system LuaJIT or use the subproject (Arch being an example here). In addition, we should build an AppImage or a Flatpak with full automation functionality on CI. I don't know much about the tradeoffs between the formats, so I'd love input on that front as well.

This preserves the current state of affairs (automation scripts are almost all broken on Debian) but makes clearer to the user what's going on, and makes the program easier for distros to package. This satisfies priorities 1, 3, 4, and 6. Unfortunately, it means we'd be shipping a build that is missing a large feature, even if we're clear as to why. It also more or less punts on the DepCtrl issue, but given the uncertain status of the sqlite rewrite I'm inclined to say that's okay for now and we can reassess when that becomes relevant.

I'm eager to hear other perspectives before I sit down with arch1t3cht and try to reach a decision on how to move forward here (and cut 3.4.1). I'll update this issue as relevant info about the current state comes in.

@guijan
Copy link
Contributor

guijan commented Dec 26, 2024

I'm unclear as to the particulars of why distros will not use a separate build for Aegisub; some clarity here would be helpful

Most package systems have a hard rule against vendoring dependencies that only the most popular and difficult to work with projects (e.g. Firefox) are allowed to violate. Some package systems are understaffed enough that they tolerate it because they'd rather have the package (e.g. OpenBSD). Vendoring libraries defeats the benefits of shared libraries, and the issue package maintainers care about the most is having to apply fixes (specially security fixes) to many copies of the same library and having to integrate and test them when fixing one shared library should be enough. Most package maintainers simply won't bother packaging a program that doesn't meet their (very much reasonable) requirements and which has alternatives in their eyes.

Also, most Linux-centric software doesn't even compile on Linux, something with more Windows roots like Aegisub already didn't compile almost anywhere until 3.4.0, including because of vendored dependencies. Other, more obscure systems will break most of the time. Fixing that in vendored libraries is even worse: if I fix luajit's Makefile, that doesn't fix the meson.build Aegisub wrote for luajit, and if I fix luajit on the Aegisub side, that doesn't fix upstream luajit.

FreeBSD, OpenBSD

Aegisub doesn't compile on these currently. I forgot what the issue was on FreeBSD, but on OpenBSD it's all related to Lua. I have Aegisub compiling on OpenBSD with some changes, but it's useless as it is because ffms2 and avisynth don't compile either and I have them disabled, so my build can only open y4m files.

Disable automation scripts if a suitable LuaJIT isn't available

Disabling automation scripts when the Lua implementation isn't suitable is fine, I've participated on many projects using Aegisub and never used any of them. 3rd party scripts that depend on luajit features will still error in ways less tech savvy users won't understand.

@petzku
Copy link
Contributor

petzku commented Dec 26, 2024

3rd party scripts that depend on luajit features will still error in ways less tech savvy users won't understand.

If I correctly understand the maintainer stance on this, the idea is not to have scripts detect whether they have a suitable Lua environment to work with, but rather for Aegisub itself to simply not load any Lua automations if LuaJIT (with 5.2 compat) isn't available. This would mean that no scripts would work (which is less than ideal, as there are internal tools that have been planned to migrate to first-party Lua automations instead), which in turn means that 3rd party scripts couldn't error in such situations either.

@aniolm9
Copy link

aniolm9 commented Dec 27, 2024

I've done some investigation and it seems that this Lua 5.2 compat issue has been there for almost 10 years: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=781728.

As @guijan pointed out, having bundled libraries is not recommended and brings extra work, but in some special cases it can be allowed. For instance, Aegisub already has some bundled dependencies like luabins, so I guess that LuaJIT could be added as well. On the other hand, downloading during build is forbidden, so the wrap is not an option for Debian.

I think that we should proceed as follows:

  1. Add the LuaJIT source code to the subprojects directory (i.e. as a git submodule) so that the release tarball contains all the code without needing to download anything during build.
  2. Remove the LuaJIT check on Linux and support building against vanilla Lua 5.2. This would be a fallback option if LuaJIT 5.2 is not found. To improve UX, maybe automation could be enabled with some kind of "nice" warning when a script crashes due to compatibility?

From my side I'll add a reply to the Lua 5.2 compat bug in Debian and ask if LuaJIT could be bundled with Aegisub (as source code already available in the tarball).

@FichteFoll
Copy link
Contributor

FichteFoll commented Dec 27, 2024

It was already briefly asked in the OP, but what prevents distros from adding a separate luajit-with-5.2-compat package to their repositories that Aegisub could dynamically link against?

@arch1t3cht
Copy link
Member

To improve UX, maybe automation could be enabled with some kind of "nice" warning when a script crashes due to compatibility?

Note that it's not possible to determine whether a script crashes due to compatibility, or due to some other bug in the script itself. Any such warning could end up being confusing or being ignored.

Can't you instead just build two libraries, libluajit-5.1.so.2 and libluajit-5.1-lua52compat.so.2?

To me, this seems like the perfect solution to this problem, are there any problems with this that I am missing?

Add the LuaJIT source code to the subprojects directory (i.e. as a git submodule) so that the release tarball contains all the code without needing to download anything during build.

This also sounds feasible to me. Making LuaJIT a git submodule would be inconsistent with all the other dependencies, but meson can be used to generate tarballs (which I'm currently working on getting to work for Aegisub - this would also fix your issue in #220), and can also be configured to include subprojects (Although there are some issues with that too, see mesonbuild/meson#14005). Releasing a tarball that only includes the wrap subproject for luajit is somewhat inconsistent as well, but it makes sense to some degree since that's (currently) the only wrap subproject that distros may not provide in their system packages.

@aniolm9
Copy link

aniolm9 commented Dec 27, 2024

Having LuaJIT with 5.2 compat packaged on Debian would be the best solution and I don't know why it hasn't been done. The problem is that it is out of our reach, so for now we should focus on how to make it packageable. In the meanwhile I'll try to get more information about this (I've already replied to the bug I linked before).

Note that it's not possible to determine whether a script crashes due to compatibility, or due to some other bug in the script itself. Any such warning could end up being confusing or being ignored.

Okay, I was not aware of that. I think that losing the automation feature is a big deal, but if you don't see any other options we should go for it. I also don't like getting bug reports about scripts breaking the program :P

This also sounds feasible to me. Making LuaJIT a git submodule would be inconsistent with all the other dependencies, but meson can be used to generate tarballs (which I'm currently working on getting to work for Aegisub - this would also fix your issue in #220), and can also be configured to include subprojects (Although there are some issues with that too, see mesonbuild/meson#14005). Releasing a tarball that only includes the wrap subproject for luajit is somewhat inconsistent as well, but it makes sense to some degree since that's (currently) the only wrap subproject that distros may not provide in their system packages.

On my side I just need the source code to be in the tarball. I can remove files from upstream if, for instance, they don't follow the DFSG but I can't add new files.

@guijan
Copy link
Contributor

guijan commented Dec 27, 2024

On the other hand, downloading during build is forbidden, so the wrap is not an option for Debian.

meson dist has an --include-subprojects flag to include the source of wrap subprojects.
Might be a good idea to switch to the wrapdb luajit (CTRL+F luajit) and include it in source tarballs, actually.

@kasper93
Copy link
Contributor

Might be a good idea to switch to the wrapdb luajit (CTRL+F luajit) and include it in source tarballs, actually.

I'm made a try about using upstream wrap of luajit, seems to work. See #242 Although I see nothing wrong from shipping custom meson wrap that is currently vendored.

My inclination is to say we should add a flag to allow building against either LuaJIT without the 5.2 flag or vanilla 5.2

LuaJIT is whatever form is fine. I'd advice against endorsing "vanilla" 5.2 which is EOL for a long time and currently we should consider LuaJIT as defacto the implementation to use.

We actually face similar dilemma in mpv. We do support linking with all sort of lua versions. Which basically currently forces scripts to be written in 5.1/5.2 compatible way. I personally think that we should support luajit only at this point... and in default configuration. I understand that Aegisub is in slightly more difficult situation because of 5.2 requirement.

Remove the 5.2 compatibility check on Linux

I think loosening requirements at this point is not best idea as it would break a lot of existing scripts in often not easy to understand ways for end users.

Disable automation scripts on Linux if a suitable LuaJIT isn't available

This is possible, but in practice this would mean that Aegisub is shipped without automation scripts in most cases. Unless something is required it will likely be stripped if it causes any troubles. And bug reports will be sent here as user doesn't know better. See https://www.jwz.org/blog/2016/04/i-would-like-debian-to-stop-shipping-xscreensaver/

@CoffeeFlux
Copy link
Member Author

The comments on this have been fairly encouraging. If this issue can be addressed for Debian with some extra packaging changes on our end, that seems clearly better for everyone involved. Hopefully other distros can be similarly accommodating. There's already a vendored dependency in the repo anyway, given the luabins situation.

Let's continue to mandate LuaJIT with 5.2 compatibility and leave the checks intact, but ship a tarball with the LuaJIT subproject and git_version.h pre-populated. If a distro patches out that check and we get a bug report as a result of it, I'll email them to complain and explain that functionally they're packaging in a way that breaks almost all user scripts.

We should probably still consider shipping an AppImage/Flatpak, but it doesn't have to happen before 3.4.1. I'm conceptually on board with the swap to the upstream LuaJIT wrap, as maintaining our version is mildly annoying, but I'll wait until after 3.4.1 to merge that.

I'd advice against endorsing "vanilla" 5.2 which is EOL for a long time

That's a fair point, thanks. Whatever we do, it probably shouldn't be encouraging PUC 5.2.

@arch1t3cht
Copy link
Member

@aniolm9 I set up tarball generation in #251 . Source tarballs including git_version.h and the luajit subproject are now generated by the CI and should be attached to new releases, let me know if the generated tarballs work for you or if anything needs to be fixed.

@EL-File4138
Copy link
Contributor

EL-File4138 commented Dec 31, 2024

Fedora currently does not include Aegisub in the official repo, but RPM Fusion packs 3.3.3 from wangqr's fork.
LuaJIT is built with 5.2 compatibility (Oh d**n my mistake), so directly building against the system provided LuaJIT is not a problem. However, the package is still not provided as a Fedora package. I will try to push the package to pass review, but if again rejected, Continuing to support RPM Fusion package is also viable: Almost every user needs to enable it for the extra codecs of FFmpeg.

LuaJIT is built without 5.2 compatibility (Line 57, 58), and from what I assume about the Fedora packaging guideline is that:
1. No prebuild binary is allowed;
2. Packages which are not functional or useful without code or packages from third-party sources are not acceptable (I'm not sure about what this "not functional or useful" means here, since without 5.2 compat most of the functionality of Aegisub is still intact)

Currently I'm considering either:
- Pushing respective package to support LuaJIT with 5.2 compat;
- Try to create an alternative with 5.2 compat;
- Or just ditch support and build against system LuaJIT;

Either would be hard, since It seems like Aegisub has been rejected as a Fedora package. Continuing to support RPM Fusion package is also viable: Almost every user needs to enable it for the extra codecs of FFmpeg. On which self-provided LuaJIT should be built without problem.

@aniolm9
Copy link

aniolm9 commented Dec 31, 2024

@arch1t3cht it builds out of the box. I'll package it when 3.4.1 is released. Thanks a lot!

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

No branches or pull requests

8 participants