geany / geany-plugins

The combined Geany Plugins collection
http://plugins.geany.org/
594 stars 266 forks source link

[GeanyLua] For version 5.4.3? #1133

Open jradxl opened 3 years ago

jradxl commented 3 years ago

Any change this plugin will be upgraded to Lua 5.4.3 any time soon?

elextr commented 2 years ago

Since Geanylua is orphaned (ie has no maintainer) I suspect it will be a case of someone needing to do it.

xiota commented 1 year ago

LuaJIT appears to be a drop-in replacement for Lua 5.1 that would require only build script or other minor changes. Based on Git activity, LuaJIT is actively maintained. Perhaps GeanyLua could switch to LuaJIT (issue #1228)?

elextr commented 1 year ago

As I noted above, the plugin is orphaned, so somebody needs to take it over and then make the change.

xiota commented 1 year ago

I am willing to look into what would be necessary to switch to LuaJIT, but if there would be opposition to such a change, it wouldn't be worth the effort.

In principle, switching to LuaJIT should be easier than switching to Lua 5.4+ because LuaJIT is intended to be a drop-in replacement for Lua 5.1. LuaJIT appears to be actively maintained, while Lua 5.1 has been deprecated for nearly two decades. At the moment, it's working, but there's a possibility that it will eventually be dropped by some distros or stop compiling because of some future gcc, glibc, or other update.

elextr commented 1 year ago

Well, there is no plugin maintainer to object ;-)

The simplest solution would be just to make it a new plugin ("Geanyluajit" maybe) until its stable and supports the platforms the existing one does. That also means a Geanylua is still available while the distros get their heads around the new dependency. Then Geanylua can be simply deleted when it breaks.

I can't figure out if the bundles include Geanylua and if a bundled lua dll is included for Windows (@eht16) or OSX (@techee)?

techee commented 1 year ago

It's not included for macOS - I don't remember why, if it was just that I didn't try or know how to use the plugin, or if there was some actual problem. Right now the macOS binary uses the "hardened runtime" where JIT is disabled but one can easily add a certain flag and enable it so it shouldn't be a problem.

techee commented 1 year ago

Lua 5.4 is not a drop-in replacement for Lua 5.1, so it would require major rewrites.

Just out of curiosity, what are the changes that make it hard to migrate to the new version?

xiota commented 1 year ago

Just out of curiosity, what are the changes that make it hard to migrate to the new version?

I'm not familiar with Lua, but took a look a while ago. I saw accumulated API-breaking changes. Compatibility functions/macros did exist between adjacent releases, but those that were of interest had been deprecated and removed by 5.4. Both 5.1 and 5.4 APIs would have to be learned well enough to almost be able to rewrite the plugin from scratch. Further, a 5.5 release, with more API-breaking changes, could be just a couple years away. (5.4 was released about 2.5 years ago. Average time between 5.x releases is about 4.5 years. Longest is 6 years.)

From LuaJIT Installation:

LuaJIT is API-compatible with Lua 5.1. If you've already embedded Lua into your application, you probably don't need to do anything to switch to LuaJIT, except link with a different library...

So it seems there's potential, with minimal effort, to switch to a maintained library with reduced concern for future API-breaking changes.

LuaJIT is supported on Windows and macOS. Packages are available for Debian and Fedora.

xiota commented 1 year ago

I just tried rebuilding GeanyLua with LuaJIT. It's fairly simple:

Would a single PR be acceptable, or would separate PRs be preferred?

techee commented 1 year ago

I'm just wondering - what problem does the migration to luajit try to solve? As far as I can say, Lua 5.1 is available as a package and offered by distributions. I've just checked luajit's open bugs and one of them is this one

https://github.com/LuaJIT/LuaJIT/issues/559

where it doesn't support pointer authentication which is used on ARM-based Apple computers:

https://support.apple.com/guide/security/operating-system-integrity-sec8b776536b/web

If there is some compelling reason to move to luajit, it might be worth it and sacrifice the macOS version of the plugin (which doesn't exist yet anyway) but I haven't seen such a reason mentioned above.

elextr commented 1 year ago

https://github.com/geany/geany-osx/issues/41#issuecomment-1454152266 seems to suggest that the plugin will never work on Windows or Wayland either.

If "somebody" contributed it I bet it could just hook into the Geany keyevents and use GDK platform independently like Geany does.

xiota commented 1 year ago

There's no "problem" to "solve", per se.

elextr commented 1 year ago

IIUC the reason things like gegl moved to luajit was for speed, its a pixel manipulation library after all, they really don't care about whatever the new features of Lua 5.4 are since they just use Lua as an extension language, not an application language. And now there is an ecosystem of extensions in Lua 5.1 I doubt they want to upgrade to Lua 5.4 until Luajit supports it and then only if it is strictly Lua 5.1 backward compatible so existing Lua extensions work.

I would have thought Geanylua is in the same category, performance that minimises UI delays is important if Lua code runs between keystrokes, writing whole applications in Geanylua is not.

Given that Geanylua only appears to work on Linux X11 it is likely to become unusable from that before Lua 5.1 is removed, so I'm not sure if @xiota wants to invest the effort in porting to Luajit or Lua 5.4 before that is fixed.

Also @kugel- does Peasy support luajit?

xiota commented 1 year ago

Given that Geanylua only appears to work on Linux X11 it is likely to become unusable from that before Lua 5.1

Wayland is still feature incomplete. The number of distributions supporting X11 hasn't dwindled to a mere handful yet.

Are there any other X11 issues besides the key event hooks?

I'm not sure if xiota wants to invest the effort in porting to Luajit or Lua 5.4 before that is fixed.

I'm already using GeanyLua with LuaJIT. "Porting" was mostly changing the build script. My existing (simple) Lua scripts are working as expected with no change.

I keep revisiting GeanyLua because it's useful, but dislike using/working on it because it's on a deprecated for nearly two decades branch of Lua. LuaJIT seems to be the most expedient path.

elextr commented 1 year ago

Wayland is still feature incomplete.

Thats intended, Wayland was invented to drop old historic parts of X11, but maybe it dropped too much at the start.

The number of distributions supporting X11 hasn't dwindled to a mere handful yet.

No distro is going to "not support X11" any time soon, thats what XWayland is for. Where the problem comes is that if the Geany packaged by the distro defaults to running on Wayland then X11 events won't be received, so that Geanylua code won't work, or even worse will attempt to interpret a Wayland event as an X11 event.

And Wayland by default is increasing, Ubuntu 22.04 is Wayland by default IIUC, Fedora has been for a while, Debian Gnome since Debian 10 IIUC.

Are there any other X11 issues besides the key event hooks?

AFACGSCT no (As Far As Crappy Github Search Can Tell ;-)

LuaJIT seems to be the most expedient path.

If its only a build change then basically I agree. But the X11 problem still remains, I will open a separate issue for that.

@hyperair how do we make sure packagers are aware that the dependency of a plugin changed?

techee commented 1 year ago

https://github.com/geany/geany-osx/issues/41#issuecomment-1454152266 seems to suggest that the plugin will never work on Windows or Wayland either.

It's not that bad. First, there's a native code for Windows for this already in the plugin. Second, the whole code I think just serves for exposing some key event grabbing to lua scripts which I think isn't so essential - I think users won't make some advanced GUI stuff with the plugin so if this functionality isn't available on macOS or Wayland, nothing too bad will happen. One can just ifdef-out the code starting from here

https://github.com/geany/geany-plugins/blob/2665511f731e613944815ab62885ecf52e3037e6/geanylua/glspi_app.c#L450

and ending here

https://github.com/geany/geany-plugins/blob/2665511f731e613944815ab62885ecf52e3037e6/geanylua/glspi_app.c#L607

plus the line here

https://github.com/geany/geany-plugins/blob/2665511f731e613944815ab62885ecf52e3037e6/geanylua/glspi_app.c#L632

on these platforms and the whole plugin compiles (I succeeded to compile it on macOS, I just did run into some autoconf-fighting issues so didn't get it running yesterday, will try when I have more time).

Since LuaJIT is maintained, there's a reasonable chance the issue will be fixed sometime around when mainline Linux fully supports Apple M# processors.

We are not talking about linux here but native macOS build which would break because of the pointer authentication issue. IMO much better thing would be to keep using lua and rather try to port the plugin to the latest lua version. I don't think performance is an issue here as I would suspect users use this plugin for some simple extensions rather than CPU-intensive tasks.

elextr commented 1 year ago

First, there's a native code for Windows for this already in the plugin.

Yeah, I belatedly noticed, see edit on https://github.com/geany/geany-plugins/issues/1230#issue-1609545767

As noted in that issue, I think it can be implemented by connecting to proper GTK key-press and key-release signals and be portable.

As for the luajit vs Lua 5.4.3, apart from unknown work to port it, is 5.4.3 backward compatible at the Lua level, or will scripts break?

xiota commented 1 year ago

As for the luajit vs Lua 5.4.3, apart from unknown work to port it, is 5.4.3 backward compatible at the Lua level, or will scripts break?

The Lua manuals list "incompatibilities". 5.2, 5.3, 5.4. I don't know how significant they are for GeanyLua, but I'd expect simple scripts would probably keep working, while more complicated ones are more likely to run into problems.

kugel- commented 1 year ago

Also @kugel- does Peasy support luajit?

Generally yes, I have found that distributions generally do not include lua support in libpeas packages. That is one reason that I want to build a geany-flatpak so that I can include a lua-enabled libpeas.

xiota commented 1 year ago

Since LuaJIT is maintained, there's a reasonable chance the issue will be fixed sometime around when mainline Linux fully supports Apple M# processors.

We are not talking about linux here but native macOS build which would break because of the pointer authentication issue.

My understanding is that it's an ARM64e processor feature/issue. When Linux gets to the point that M# processors are fully supported, there will be more developers who would be interested in looking at the issue.

IMO much better thing would be to keep using lua and rather try to port the plugin to the latest lua version. I don't think performance is an issue here as I would suspect users use this plugin for some simple extensions rather than CPU-intensive tasks.

LuaJIT can be supported with a build script change. The script can fall back on plain Lua 5.1 when LuaJIT is unavailable. #1233 works with both LuaJIT and Lua 5.1 on Linux. Packagers should have to change only the depends. Testing/adjustments for Windows and Mac would be appreciated.

techee commented 1 year ago

My understanding is that it's an ARM64e processor feature/issue.

Yes, but I have a macbook with this processor already and I provide macOS binaries of Geany for it here https://download.geany.org/geany-1.38_osx_arm64.dmg which, if geanylua is added (I've just managed to get it running), will probably crash when the plugin is enabled (have to try what happens exactly).

LuaJIT can be supported with a build script change. The script can fall back on plain Lua 5.1 when LuaJIT is unavailable. https://github.com/geany/geany-plugins/pull/1233 works with both LuaJIT and Lua 5.1 on Linux. Packagers should have to change only the depends. Testing/adjustments for Windows and Mac would be appreciated.

But again, aren't we just complicating our lives with this unnecessarily? Why having two different dependencies doing basically the same when the current Lua 5.1 works fine?

xiota commented 1 year ago

But again, aren't we just complicating our lives with this unnecessarily? Why having two different dependencies doing basically the same when the current Lua 5.1 works fine?

I have a macbook with this processor already and I provide macOS binaries of Geany... if geanylua is added (I've just managed to get it running), will probably crash when the plugin is enabled (have to try what happens exactly).

If GeanyLua crashes on Mac with Lua 5.1, there is no change in plugin availability whether LuaJIT is used or not. If GeanyLua works on Mac with Lua 5.1, but not LuaJIT, it can be built with Lua 5.1 either by autodetection or setting the --with-lua-pkg flag (which currently isn't working, but is fixed by #1233).

Does that ARM64e bug only affect JIT? JIT can be turned off in LuaJIT. (I believe that is the default without some changes to turn JIT on.)

elextr commented 1 year ago

Why having two different dependencies doing basically the same when the current Lua 5.1 works fine?

Its not two dependencies, its alternate dependencies.

PR #1233 is a build system change only, #1231 is 8 occurrences of a name updated to use the real name (even on Lua5.1) and no longer use the compat name that is Lua5.1 only.

Given the size of the change required, to me there seems to be no reason not to offer a choice of either, a distro/bundle can decide which it wants to offer its users, known but slower and unsupported, or faster and supported (or in the case of Macos if its lua5.1 only due to security issues, thats fine).

The Lua manuals list "incompatibilities". 5.2, 5.3, 5.4. I don't know how significant they are for GeanyLua, but I'd expect simple scripts would probably keep working, while more complicated ones are more likely to run into problems.

Yeah, I didn't understand the importance (or not) of those so I hoped a Geanylua user could comment.

xiota commented 1 year ago

Yeah, I didn't understand the importance (or not) of those so I hoped a Geanylua user could comment.

I don't really know. But the changes in number handling seem significant and potentially able to cause problems in unexpected places. For instance, number handling changes, along with changes in the Geany API, are related to why geany.activate wasn't working.

hyperair commented 1 year ago

@elextr

@hyperair how do we make sure packagers are aware that the dependency of a plugin changed?

Just change the dependency of the plugin in build/geanylua.m4, and packagers should figure out that something changed when the next version fails at the configure step with the old build-dependencies I think? Maybe also drop a note in NEWS as well so if packagers go looking they'll find it.

techee commented 1 year ago

@elextr @xiota I think we maybe didn't understand each other - I have no real problem with using LuaJIT, I just think it's more beneficial to migrate to a new Lua version than being stuck at 5.1 - IMO it's just a workaround and not a real fix.

I just created this PR migrating GeanyLua to Lua 5.3 (as mentioned in the PR, I don't have Lua 5.4 in my distro so I didn't try that one): https://github.com/geany/geany-plugins/pull/1235. The changes were mostly cosmetic, nothing big popped up.

xiota commented 1 year ago

@techee I tried migrating to 5.4 shortly before this issue was opened. I got it to compile, but segfaults on load. Maybe gave up too quickly. I did not know about LuaJIT at the time. Stepwise migration would have been easier, but the distro I was using at the time made that difficult.

Problems with 5.3 and 5.4:

Benefits of LuaJIT over 5.4 are:

Problems with LuaJIT

techee commented 1 year ago

I just tried and on my other linux VM geanylua with the PR runs with Lua 5.4 without any modifications needed.

I'm not really convinced by the performance argument - I just don't see how anyone would notice. The whole Geany except Scintilla lexers and ctags parsers could be written in a scripting language and I don't think anyone would notice any performance difference.

(And, and this is probably just a personal preference, by adding JIT you make from a nice little interpreter a beast which introduces its own set of problems like CPU/OS/architecture dependence. I'm not sure how LuaJIT's JIT works but since most of the scripts will be short-running, it could make the execution of the scripts slower if it performs JIT compilation every time and doesn't store the precompiled binary somewhere. IMO, the only language where JIT makes sense is javascript in web browsers, but anywhere else where you need the performance, it's best to go with the native code.)

Regarding compatibility, yes, that could be an argument. But I suspect users of this plugin will use it for smaller utilities related to their workflow and they won't use any advanced features from the language where they might see differences.

Anyway, my personal preference would be using pure Lua but I'm fine if others have a different opinion and the LuaJIT patch gets merged.

xiota commented 1 year ago

I just tried and on my other linux VM geanylua with the PR runs with Lua 5.4 without any modifications needed.

I saw that note. (I'll try your branch in a little while.) The problem is the Lua language changes between versions. And there may be subtle API differences that work fine on the C side, but result in different Lua execution results. So if different platforms are using different versions of Lua, say 5.3 and 5.4, the scripts won't necessarily be compatible. Also, existing Lua 5.1 scripts may break.

I'm not really convinced by the performance argument - I just don't see how anyone would notice. The whole Geany except Scintilla lexers and ctags parsers could be written in a scripting language and I don't think anyone would notice any performance difference.

Whether someone notices depends on how performant their hardware is. I've seen Geany touted as being suitable for use on low-end hardware. At least a few such users would notice.

... by adding JIT you make from a nice little interpreter a beast which introduces its own set of problems like CPU/OS/architecture dependence. I'm not sure how LuaJIT's JIT works but since most of the scripts will be short-running, it could make the execution of the scripts slower if it performs JIT compilation every time and doesn't store the precompiled binary somewhere.

LuaJIT can run without JIT (as I understand, this is the default). According to benchmarks others have run, the LuaJIT interpreter, without JIT, runs faster than the Lua interpreter.

I suspect users of this plugin will use it for smaller utilities related to their workflow and they won't use any advanced features from the language where they might see differences.

Based on code snippets posted online, changes in Lua behavior can be surprising. Even if something breaking is unlikely, it would be highly disruptive to the workflows of users who do experience problems. If not using advanced features, why switch to Lua 5.4 rather than add LuaJIT build support?

Anyway, my personal preference would be using pure Lua but I'm fine if others have a different opinion and the LuaJIT patch gets merged.

Adding LuaJIT build support now doesn't in and of itself block work toward supporting another Lua release in the future.

elextr commented 1 year ago

@techee

But I suspect users of this plugin will use it for smaller utilities related to their workflow and they won't use any advanced features from the language where they might see differences.

(@elextr chuckles to self) Of course that argument could be run backwards, if scripts won't use the newer features there is no Lua reason to upgrade :grin:

Your comments on possible performance issues are valid, compiling first every time a script is run may be slower than interpreting for short scripts, but IIUC there is a compile cache so a function only gets compiled once. What the effect is can only be determined by benchmarking lots of Geanylua scripts, but we don't have those, and no Geanylua users have commented on this issue's issues.

Maybe there are no intensive users, only occasional users like @xiota so nobody will care about such details. For example it seems (#1236) that grabkey() has not been working for an unknown period of time, but nobody has complained? Dunno.

@xiota

Adding LuaJIT build support now doesn't in and of itself block work toward supporting another Lua release in the future.

True, and #1235 suggests the changes are simple enough that both 5.1 and 5.4 C APIs could be supported within the #ifdef pain threshold. As a wise man once said, if offered a questionable choice, take all three (at least until a winner is clear).

xiota commented 1 year ago

... performance issues...

I will ask someone who is having some performance issues while using some GeanyLua scripts whether he's willing to try LuaJIT to see if there's any noticeable difference. But I think the problem is that the computer is just slow.

True, and https://github.com/geany/geany-plugins/pull/1235 suggests the changes are simple enough that both 5.1 and 5.4 C APIs could be supported within the #ifdef pain threshold. As a wise man once said, if offered a questionable choice, take all three (at least until a winner is clear).

I'd request that adding support for 5.4, be deferred until late in 1.39 development or even a future release. This would make backporting bug fixes prior to the transition easier (eg, if a 1.38.1 bugfix branch were created). For instance, fixes for geany.activate (#1234) and geany.keygrab (#1236) can be applied to 1.38 with unmodified git-generated patches. #1235 itself would require more work to backport and would complicate backporting any subsequently written bugfix.
Nevermind. Figured it out.

(Is there a git command to extract only commits/patches that apply to one plugin/folder?)

git format-patch 1.38.0 -o patches -- build/geanylua.m4 geanylua/*.{c,h,am}

Then exclude 5e130960bc6b9f3b3158cdb35d27f6bba03ba3c5 and 01b19d2d401f49f220157f28cc5e53de491f71a9.

techee commented 1 year ago

True, and https://github.com/geany/geany-plugins/pull/1235 suggests the changes are simple enough that both 5.1 and 5.4 C APIs could be supported within the #ifdef pain threshold. As a wise man once said, if offered a questionable choice, take all three (at least until a winner is clear).

Hmm, IMO we should support the smallest set of Lua versions, otherwise it might get hard to maintain the plugin. My preference regarding what to support would be (in this order):

  1. Lua 5.1, or Lua 5.2, or Lua 5.3, or Lua 5.4
  2. Lua 5.1 + LuaJIT
  3. Some wild combination of the above