Use upstream luajit meson wrap

[kasper93]

No description provided.

[kasper93]

I don't know if this is needed/wanted. But I think upstream wrap by benoit-pierre is pretty ok. Of course current Aegisub is good too, but this PR transfers maintenance cost of the luajit wrap elsewhere.

[guijan]

I... sent a pull request to your pull request because using github reviews to suggest changes didn't work so well. kasper93#1

I simplified the code a little, some conditionals weren't needed and I put the arguments to dependency('luajit', ...) in a Meson dictionary to avoid duplication. Half of the LoC reduction in my PR is just whitespace changes.

I tested the path with system_luajit=false using Alpine Linux and compilation succeeded. I don't have a system with a suitable luajit to test the system_luajit=true path.

[arch1t3cht]

Thanks! In principle this is definitely wanted, the less we have to vendor and wrap ourselves the better.

However, this doesn't actually work for me as it is (on Arch Linux, using meson 1.6.1, with non-5.2 system luajit installed). The subproject is executed, but the dependency() call afterwards still returns the pkg-config dependency. I believe that's why the original code used a manual get_variable solution. (In fact, I am slightly confused as to why the meson.override_dependency call works at all here. When I tried making our luajit wrap override the dependency, I got an error because the dependency had already been resolved. Maybe this depends on the meson version, I'd have to look into this further.)

However, the upstream luajit wrap doesn't actually write the result of declare_dependency into a variable, so it'd have to be updated or patched (with a diff) locally. When patching it this way, however (and passing -Ddefault_library=static when configuring Aegisub), things compile.

Finally, 5.2 compat doesn't actually seem to work with this wrap. The flag -DLUAJIT_ENABLE_LUA52COMPAT flag is only set for the target, but it also needs to be set for the native objects for buildvm (and this suggests that maybe it should be carefully checked if any other flags are missing for native objects).

In summary, while switching to an upstream wrap would be nice, it does seem to need a bit of extra work, at least some of which needs to happen on the wrap itself. (I don't expect you to do that work, I'm just writing out everything I know here. I might do it myself eventually.)

The two review comments are from before I wrote all of the above so they're not really pressing, but I'll leave them in so I don't forget.

[arch1t3cht]

Why do you have default_options together with allow_fallback: false here?

[kasper93]

In case meson.override_dependency were called before the initial lookup it would always use subproject. This can happen if Aegisub itself is used as subproject. Unlikely as it may be, it can happen that those default_options would be used.

[arch1t3cht]

Does it make sense to check this option in the subproject, which won't necessarily match the value of system_luajit for the parent project? Or is this just to match the parent meson.build's logic one-to-one? (If so, could you maybe add a comment above this section saying something like Duplicate luajit detection logic from main meson.build?)

[kasper93]

Or is this just to match the parent meson.build's logic one-to-one?

Correct. In fact if we agree that luabins is only used as a subproject of main meson.build we can remove this duplicated logic completely.

[kasper93]

However, this doesn't actually work for me as it is (on Arch Linux, using meson 1.6.1, with non-5.2 system luajit installed). The subproject is executed, but the dependency() call afterwards still returns the pkg-config dependency.

I was kinda expecting that. It probably uses the cached dependency lookup. I was hoping that all args to dependency() are used for the cache, but apparently not. Either way, I will fix that when I get a sec. Likely using different required version would invalidate the cache.

In fact, I am slightly confused as to why the meson.override_dependency call works at all here. When I tried making our luajit wrap override the dependency, I got an error because the dependency had already been resolved. Maybe this depends on the meson version, I'd have to look into this further.

Yeah, I was expecting override_dependecy actually did the job of overriding it.

However, the upstream luajit wrap doesn't actually write the result of declare_dependency into a variable, so it'd have to be updated or patched (with a diff) locally.

Exactly, that's why I did all the changes here to adopt it.

[kasper93]

However, this doesn't actually work for me

Should be fixed now.

[arch1t3cht]

Thanks for looking into this!

With meson 1.5 this now works for me on Linux (with system luajit installed) and on Windows (with no luajit installed, barring problems in the wrap). With meson 1.0 (which we'd like to support since that is what Debian stable currently ships), I get an error about system_luajit being an unknown option in the luabins subproject. However, like you said this option check could just be removed entirely in the luabins subproject (I do think we can be sure that luabins will only ever be called as a subproject here).
In fact, now that the luajit wrap uses override_dependency (I am still slightly confused why this works when it didn't work for me before but I'll just go with it), the entire logic in the luabins subproject could be replaced with only a dependency('luajit'), but personally I'd still prefer having an explicit 5.2 compat check there. I've had issues in the past where Aegisub used the luajit wrap but luabins used system luajit, and that can cause quite a headache if you don't notice it.
Alternatively, the luabins subproject could get its own system_luajit option that's passed on from the main meson.build, but in my experience that can cause headaches with reconfiguring existing build folders. So I'd opt to just replace required: get_option('system_luajit') with required: false in luabins.

Apart from that, all the Aegisub-side build logic seems to work now. The remaining problems are ones in the upstream luajit wrap itself (apart from broken 5.2 compat I also now saw that it doesn't (always) work on Windows since it calls a shell script), which I'll look into myself at some point. Once that's figured out, we can merge this PR.

Finally: Do you know if this behavior of the caching in dependency() being invalidated by the different version is documented somewhere or can be assumed to be stable? (e.g. do you know of some other project that relies on it?) I wouldn't think that there's any huge risk of this breaking in the future, but this does feel a bit Hyrum's Law-like to me.

[kasper93]

the entire logic in the luabins subproject could be replaced with only a dependency('luajit'), but personally I'd still prefer having an explicit 5.2 compat check there. I've had issues in the past where Aegisub used the luajit wrap but luabins used system luajit, and that can cause quite a headache if you don't notice it.

Yes, it is better to check it again in luabins, if anything to be safe we don't mix different luajit variants.

So I'd opt to just replace required: get_option('system_luajit') with required: false in luabins.

This was my idea, but apparently I copied it by accident in the end. Fixed now. As for system_luajit option itself, if we always use luabins as a subproject after parent project do the check, the parent project will already error out if system_luajit==true, else it will use internal luajit, so it is safe to do so. It's fragile, if someone moves code around, but as it is not, it should work all the time.

The remaining problems are ones in the upstream luajit wrap itself (apart from broken 5.2 compat)

What do you mean exactly? Does it not compile with 5.2 compat? Looking at the code everything should be fine, but I didn't test the actual build. The define is there tho.

I also now saw that it doesn't (always) work on Windows since it calls a shell script), which I'll look into myself at some point.

I think it calls sed for something. Generally it should work if you have at least git installed on Windows, which provides all needed tools.

Finally: Do you know if this behavior of the caching in dependency() being invalidated by the different version is documented somewhere or can be assumed to be stable? (e.g. do you know of some other project that relies on it?) I wouldn't think that there's any huge risk of this breaking in the future, but this does feel a bit Hyrum's Law-like to me.

This is iffy. Currently I actually do dependency('luajit', method: 'pkg-config') and after subproject('luajit'), dependency('luajit'). Without method this ensures that cache is not used. I doubt this is in anyway documented as such, but at the same time I wouldn't think they remove method from cache key. The method is set to pkg-config initially, because there is no method: 'subproject`` or anything like that that would make this explicit. We could update upstream wrap to define luajit_depso the oldget_variablemethod work. But there are two code paths for static/shared so inboth` it wouldn't be single dep.

I didn't expect initially all this changes to be required. I was hoping for drop-in replacement, but as we can see some shenanigans are needed. At least there is an assert there, so it if fails for whatever reason it will be immediately visible and not silently using wrong version.

[arch1t3cht]

Does it not compile with 5.2 compat? Looking at the code everything should be fine, but I didn't test the actual build.

The define is there for the luajit library, but not for buildvm, which also needs it.

[guijan]

Definitely a bug to report to the Meson wrap then.

[kasper93]

I've pushed small test to catch invalid luajit early. Either if it slips through our meson checks or subproject is built incorrectly.

EDIT: and mesonbuild/wrapdb#1848

[arch1t3cht]

Thanks! This now looks good to me pending mesonbuild/wrapdb#1853 .

[arch1t3cht]

Oh, I didn't see your edit. I'll update the PR to mention the issue.

[arch1t3cht]

Force pushed to update the wrap. Thanks again!