-
Notifications
You must be signed in to change notification settings - Fork 511
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
Comments
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.
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.
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. |
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. |
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:
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). |
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? |
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.
To me, this seems like the perfect solution to this problem, are there any problems with this that I am missing?
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. |
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).
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
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. |
|
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.
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.
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.
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/ |
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 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.
That's a fair point, thanks. Whatever we do, it probably shouldn't be encouraging PUC 5.2. |
Fedora currently does not include Aegisub in the official repo, but RPM Fusion packs 3.3.3 from wangqr's fork.
|
@arch1t3cht it builds out of the box. I'll package it when 3.4.1 is released. Thanks a lot! |
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:
__ipairs
metatable entryipairs(subs)
, and many third party scripts have copied this patternCurrent *nix distro status (to be updated)
Note: anything that says 3.3.x is probably based on the wangqr fork
Competing priorities
In deciding how to handle this, I have a few different concerns:
Possible paths forward
There is no perfect solution here, but a few options that have been brought up:
__ipairs
specifically and pray it doesn't break too many scriptsMy 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.
The text was updated successfully, but these errors were encountered: