fix(main/neovim): turn nvim into a wrapper

[pipcet]

See #22328 for further discussion.

My main concern with this PR is that the workaround is likely brittle; Android's dlopen, which was already deliberately broken to prevent loading dynamic libraries, might very well be broken further by future Android versions when the Android people realize LD_PRELOAD is currently handled correctly.

Also fixes a minor idempotency issue (repeated builds of neovim failed previously, leaving /data/data/com.termux/usr/bin/sh as a symlink to /bin/sh).

[pipcet]

Sorry, forgot to bump the revision, making CI fail. Fixed now, I hope.

[pipcet]

@s-cerevisiae If you happen to have the time, it'd be great if you could confirm independently that this fixes the issue

[s-cerevisiae]

@pipcet Sorry, it seems that it didn't work. Did you forget to export the variable?

[pipcet]

@pipcet Sorry, it seems that it didn't work. Did you forget to export the variable?

You're totally correct. Both of my test systems had LD_PRELOAD already set and exported, so the export was indeed missing. Should work now?

[s-cerevisiae]

Ok I think it works for me this time. Thank you!

[TomJo2000]

There is another option I just remembered. Though that might need additional testing.

Arch Linux loads an additional config file in the systemwide sysinit.vim. https://gitlab.archlinux.org/archlinux/packaging/packages/neovim/-/blob/0.10.2-2/PKGBUILD#L93-101

In their case it's to add some RTP directories by default. But we should be able to use the same approach to append to LD_PRELOAD, without potentially running into issues with users using different #!$PREFIX/bin/sh implementations.

[pipcet]

There is another option I just remembered. Though that might need additional testing.

Arch Linux loads an additional config file in the systemwide sysinit.vim. https://gitlab.archlinux.org/archlinux/packaging/packages/neovim/-/blob/0.10.2-2/PKGBUILD#L93-101

In their case it's to add some RTP directories by default. But we should be able to use the same approach to append to LD_PRELOAD, without potentially running into issues with users using different #!$PREFIX/bin/sh implementations.

I don't think I understand. LD_PRELOAD needs to be set before the vim executable is executed, or the dynamic loader won't notice it in time, IIUC.

[TomJo2000]

I don't think I understand. LD_PRELOAD needs to be set before the vim executable is executed, or the dynamic loader won't notice it in time, IIUC.

Mmmh, yep LD_PRELOAD, it's kind in the name... So I guess exec LD_PRELOAD="$LD_PRELOAD:libluajit.so" nvim "$@" it is then.

[pipcet]

I don't think I understand. LD_PRELOAD needs to be set before the vim executable is executed, or the dynamic loader won't notice it in time, IIUC.

Mmmh, yep LD__PRE_LOAD, it's kind in the name... So I guess exec LD_PRELOAD="$LD_PRELOAD:libluajit.so" nvim "$@" it is then.

Just for completeness, one other way of working around this is to have nvim re-exec itself after modifying the environment, if necessary. I'm not sure it would be any better, though.

[TomJo2000]

If you can give me an example of how that would work, sure.

But I think a POSIX sh oneliner is probably the lesser evil here.

[TomJo2000]

Hmm the only really comparable wrapper I could dig up is the one for tizonia: https://github.com/termux/termux-packages/blob/9b54716ef8c8bc63cc07060c22ab2273608fcc52/packages/tizonia/exe_wrapper#L1-L11 Although that serves a slightly different purpose than our needs here. As it doesn't seem to be present in the final package and is only used as a shim in the build process.

[pipcet]

If you can give me an example of how that would work, sure.

But I think a POSIX sh oneliner is probably the lesser evil here.

Does neovim load child processes? Because those would inherit the LD_PRELOAD setting, I believe, and that's quite undesirable.

[TomJo2000]

Does neovim load child processes? Because those would inherit the LD_PRELOAD setting, I believe, and that's quite undesirable.

There's probably a way to make it do that. But I'm not sure it's something we should be worried about.

[s-cerevisiae]

Does neovim load child processes?

It does launch nvim --embed on startup but I'm afraid that's more of a implementation detail. And any config is probably processed in the --embed process. Imo using a simple wrapper script is more reliable than trying to trick the editor to do something. (And please don't do anything that can possibly slow down the startup time because, you know, it's nvim.)

Edit: by slowing down I mean things like letting nvim execing itself somehow. Wrapper script is a common practice and it doesn't affect startup time noticably.

[TomJo2000]

And please don't do anything that can possibly slow down the startup time because, you know, it's nvim.

I think we'll be fine just eating the impact from appending and passing an environment variable and then calling exec. You got a couple microseconds to spare.

[TomJo2000]

@pipcet I have pushed a branch to my termux-packages development fork with a slight variation on the approach you have taken in this PR currently. https://github.com/TomJo2000/termux-packages-prs/commit/2df04d19155a186d27b0477df2a29ae26ea54ddd https://github.com/TomJo2000/termux-packages-prs/commit/f6388bb3eb42a98eb8250ed21a1a776ee574677f

With your permission I would like to push that changeset to this PRs branch so it can be merged as part of this PR.

That branch does also include your idempotency fix to the tree-sitter symlink, as well as some further housekeeping/general build script cleanup in a separate commit.

[s-cerevisiae]

I think we'll be fine just eating the impact from appending and passing an environment variable and then calling exec.

Yeah, the current shim is definitely fine.

[pipcet]

@pipcet I have pushed a branch to my termux-packages development fork with a slight variation on the approach you have taken in this PR currently. TomJo2000@2df04d1 TomJo2000@f6388bb

With your permission I would like to push that changeset to this PRs branch so it can be merged as part of this PR.

Absolutely, please go ahead and do that! The one problem I see is that A=B exec env works for me but exec A=B env doesn't.

[TomJo2000]

Absolutely, please go ahead and do that! The one problem I see is that A=B exec env works for me but exec A=B env doesn't.

Mmmh yep you're right I flipped those on accident and didn't catch it in testing because I already had neovim 0.10.2-3 installed from testing the artifact from this PR, so it didn't actually install the new version.

[TomJo2000]

Just pushed the commits.

I can't make commits as you, since that would be identity theft. ^{Though since your commits aren't signed I could have changed the Author information to yours} So the best I can do is give you Co-authored-by: credits on both commits.

You may want to consider signing your commits if that's something you care about. Your authentication SSH key can also be used as a signing key. https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification

[pipcet]

Just pushed the commits.

Thank you!

I can't make commits as you, since that would be identity theft. Though since your commits aren't signed I could have changed the Author information to yours So the best I can do is give you Co-authored-by: credits on both commits.

You may want to consider signing your commits if that's something you care about. Your authentication SSH key can also be used as a signing key. https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification

Thanks for the advice. I've decided against signing commits for now, for various reasons, and I'm perfectly happy with the commits as you pushed them :-)