rktjmp / hotpot.nvim

:stew: Carl Weathers #1 Neovim Plugin.
MIT License
359 stars 9 forks source link

Cannot import .so file in a macro file #33

Closed datwaft closed 3 years ago

datwaft commented 3 years ago

Hello! I am here, again, with another bug.

This time the bug is related to trying to require a luarock module from a fennel macro file. I am trying to use this luarock inside my macro file to process strings using PCRE2 regexes.

As you can see in the next screenshot, the luarock can be required from init.lua and init.fnl, but it cannot be required from macrofile.fnl.

evidence

How to reproduce

Learning from past issues, I created this repository that contains how to reproduce the bug using docker.

My suspicions

I am using this:

local package_path_str = table.concat({
    "/opt/here/share/lua/5.1/?.lua", "/opt/here/share/lua/5.1/?/init.lua",
    "/opt/here/lib/luarocks/rocks-5.1/?.lua",
    "/opt/here/lib/luarocks/rocks-5.1/?/init.lua"
}, ";")

if not string.find(package.path, package_path_str, 1, true) then
    package.path = package.path .. ';' .. package_path_str
end

local install_cpath_pattern = "/opt/here/lib/lua/5.1/?.so"

if not string.find(package.cpath, install_cpath_pattern, 1, true) then
    package.cpath = package.cpath .. ';' .. install_cpath_pattern
end

To make neovim able to source luarocks as Lua modules. This is the way that packer recommends and uses.

Maybe the bug is related to the package.path variable.

rktjmp commented 3 years ago

Thanks for all the testing you're doing and thanks for the detailed repro.

I can't get this to work with core Fennel, See my fork: https://github.com/rktjmp/hotpot-issue_33 and run the fennel test

root@5b112ec28647:~# cd fennel-test/
root@5b112ec28647:~/fennel-test# fennel test.fnl
required-in-macro   FAIL
required-in-module  OK

# not 100% on if this is the correct usage, values from (print package.path package.cpath)
root@ef627953ea69:~/fennel-test# fennel --add-package-path ";/opt/here/share/lua/5.1/?.lua;/opt/here/share/lua/5.1/?/init.lua" --add-fennel-path ";/opt/here/share/lua/5.1/?.lua;/opt/here/share/lua/5.1/?/init.lua" test.fnl
required-in-macro   FAIL
required-in-module  OK

Not sure if this is intentional on Fennels part or an oversight, the macro code has some specific rules around it. It does have a specific macro-path it uses for finding macros, not sure if finding modules in macros inherits from it. See https://fennel-lang.org/reference#macro-module-searching

This might be a wontfix, temporarily? Just not sure if it's a fix for upstream, intentional or if hotpot should just be cloning over some path values somewhere.

Basically if you can make it work with the fennel binary, then 100% we will make it work in Hotpot. I might have just mis-wrote the option out.

As some background, all the code run in macros is handled by Fennel. When you call (import-macros ...), Hotpot will find the file requested (so "lib/my-macro.fnl"), and the pass the contents of that file off to Fennel to actually run it.

https://github.com/rktjmp/hotpot.nvim/blob/657c208b7b2d639746a7a5a33623b8619b5e50ec/fnl/hotpot/searcher/macro.fnl#L19-L33

See hotpot read a file then just push to fennel.eval

We can pass options there, so if there is an option that lets fennel find the module, we can do that. Of course there could be issues with path's being set/passed correctly, but that it works in normal modules tells me that they are working OK and shouldn't need tweaking.

Note that I am using the provides-fennel #30 branch of hotpot in that repro which includes some macro-file module-finding parity/fixes to hotpot which might be useful to you (doesn't seem to fix this issue though).

rktjmp commented 3 years ago

Obviously it would be nice if this did work.

rktjmp commented 3 years ago

I did some AFK thinking and realised why it doesn't work. I neglected to poke at the cpath properly before (too sleepy :zzz:).

Fennel (and Lua in general) has a fallback system for the package finders. You can read more about this in the lua docs, re: require.

With hotpot and normal fennel, the searchers roughly go: [preload, hotpot, neovim, package.path, package.cpath], because we insert our loaders in front of lua's defaults.

But the macro searcher in Fennel doesn't inherit from that list, it has its own.

https://github.com/bakpakin/Fennel/blob/e6a8b03fec1f24f14f3734cb4eb4926c9641b301/src/fennel/specials.fnl#L1084-L1104

Specifically the list:

(local macro-searchers [lua-macro-searcher fennel-macro-searcher])

Since these searchers don't look at package.cpath themselves, nor do they fallback to the regular searchers, the lua-rock rex_pcre2.so isn't found (because that's where it lives).

Again, I am not sure if this is intentionally omitted or just forgotten.

I could patch the hotpot searcher to look at the cpath but it's supposed to be "last in the list" and I am not 100% on how to even load a .so from lua right how. Or Hotpot could insert a cpath searcher into the end of Fennel's list but I don't really love that. I think it should be patched upstream if it's desired behaviour in Fennel.

rktjmp commented 3 years ago

You can do this, https://github.com/rktjmp/hotpot-issue_33/commit/caf7851f81d19640f8b4279d6294cd9e480e048f to insert the two final lua loaders into the macro searchers.

It will incur a start up penalty as you now have to load fennel every boot, or you could just comment/uncomment it when you make changes to those macro files. My first attempt was to put that code in the macro file and try to just effect the searcher when loading that one file but fennel.macro-searchers is nil in that context so you can't append to it.

I don't really endorse doing this though, it's relying on things being in a "probable state".

datwaft commented 3 years ago

Thanks! I will do some testing with what you told me and maybe open an issue on the fennel repo.

Thanks a lot!

rktjmp commented 3 years ago

I actually asked on the IRC channel,

technomancy@technomancy:libera.chat
when I read your question I was like "there's no way there's a legitimate use
case for loading C code from a macro" ... then I read the issue and saw it was
PCRE and I was like "oh... OK, so I guess there is one"

technomancy@technomancy:libera.chat
short-term I think writing your own macro searcher is the way to go. I will
need to think about this. it seems like overkill to include it out of the box
if there is literally only one single use case for it but if the implementation
is simple maybe

If you do make sure you include a fennel repro, not a hotpot one to avoid any confusion. Opening an issue at least lets it get tracked.

datwaft commented 3 years ago

Thanks!

I saw that on the IRC channel.

Fun fact: this all began because I wanted to use a regex expression that uses ?, and the default regex engine in Lua doesn't support that regex expression.

rktjmp commented 3 years ago

%x: (where x is any non-alphanumeric character) represents the character x. This is the standard way to escape the magic characters. Any punctuation character (even the non magic) can be preceded by a '%' when used to represent itself in a pattern.

%? should match ? or do you mean back captures%?

datwaft commented 3 years ago

I mean an optional capture group like:

(no)?(\w+)

Limitations of Lua patterns

Especially if you're used to other languages with regular expressions, you might expect to be able to do stuff like this:

'(foo)+' -- match the string "foo" repeated one or more times '(foo|bar)' -- match either the string "foo" or the string "bar" Unfortunately Lua patterns do not support this, only single characters can be repeated or chosen between, not sub-patterns or strings. The solution is to either use multiple patterns and write some custom logic, use a regular expression library like lrexlib or Lua PCRE, or use LPeg [3]. LPeg is a powerful text parsing library for Lua based on Parsing Expression Grammar [4]. It offers functions to create and combine patterns in Lua code, and also a language somewhat like Lua patterns or regular expressions to conveniently create small parsers.

rktjmp commented 3 years ago

Bummer dude.

datwaft commented 3 years ago

I opened an Issue in the Fennel repo for tracking purposes, you can see it here: https://github.com/bakpakin/Fennel/issues/391

datwaft commented 3 years ago

I tested the patch recommended by @technomancy and it works, you can see it in this branch.

I also tested it in my hotpot configuration and it seems to work.

I think this issue can be closed now.

Maybe the patch can be added to a FAQ or something.

technomancy commented 3 years ago

I don't know much about hotpot but if it has compiler sandboxing disabled already then including this macro searcher in hotpot itself might be the way to go?

rktjmp commented 3 years ago

Hotpot aims to be "just Fennel", so it can disable the sandbox but doesn't by default, and won't patch this loader in by default either unless it were to be up streamed in someway.

As is, I think the user inserting the c-loader as shown is the solution.

Hotpot might (should?) get an optional hook interface so you can lazily insert the loader without having to directly require fennel, so you can avoid the startup time cost of (require :fennel).

Probably is worth noting the work around somewhere in the Fennel docs, or at least noting the limitation in the macro docs and why.

technomancy commented 3 years ago

OK yeah if it's sandboxed then that's not a great idea. I'll add an explanation in api.md.