Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Cannot import .so file in a macro file #33

Closed
datwaft opened this issue Sep 11, 2021 · 15 comments
Closed

Cannot import .so file in a macro file #33

datwaft opened this issue Sep 11, 2021 · 15 comments

Comments

@datwaft
Copy link
Contributor

datwaft commented Sep 11, 2021

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
Copy link
Owner

rktjmp commented Sep 11, 2021

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.

(fn create-fennel-loader [path modname]
;; (string, string) :: fn, string
;; assumes path exists!
(let [fennel (require-fennel)
code (read-file! path)]
(fn []
;; we must perform the dependency tree require *in* the
;; load function to avoid circular dependencies. By putting
;; it here we can be sure that the cache is already loaded
;; before hotpot took over.
;; Mark this macro module as a dependency of the current branch.
((. (require :hotpot.dependency_tree) :set) modname path)
(local options (doto (config.get-option :compiler.macros)
(tset :filename path)))
(fennel.eval code options))))

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
Copy link
Owner

rktjmp commented Sep 11, 2021

Obviously it would be nice if this did work.

@rktjmp
Copy link
Owner

rktjmp commented Sep 11, 2021

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

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
Copy link
Owner

rktjmp commented Sep 11, 2021

You can do this, rktjmp/hotpot-issue_33@caf7851 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".

@rktjmp rktjmp changed the title Cannot import luarock from macro file Cannot import .so file in a macro file Sep 11, 2021
@datwaft
Copy link
Contributor Author

datwaft commented Sep 11, 2021

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

Thanks a lot!

@rktjmp
Copy link
Owner

rktjmp commented Sep 11, 2021

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
Copy link
Contributor Author

datwaft commented Sep 11, 2021

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
Copy link
Owner

rktjmp commented Sep 11, 2021

%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
Copy link
Contributor Author

datwaft commented Sep 11, 2021

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
Copy link
Owner

rktjmp commented Sep 12, 2021

Bummer dude.

@datwaft
Copy link
Contributor Author

datwaft commented Sep 12, 2021

I opened an Issue in the Fennel repo for tracking purposes, you can see it here: bakpakin/Fennel#391

@datwaft
Copy link
Contributor Author

datwaft commented Sep 14, 2021

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
Copy link

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
Copy link
Owner

rktjmp commented Sep 14, 2021

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
Copy link

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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants