Switch luvit to use luvit-loader instead of luvit/require package

[creationix]

This is the start for the proposed luvit 3.0 change.

This change in particular is a breaking change for some edge cases of require usage.

[creationix]

Hmm, looks like something is wrong with windows. Perhaps luvit-loader assumed unix paths somewhere.

[squeek502]

With this breaking change, I think it might also be worth re-evaluating relative requires, as they won't behave as expected with luvit-loader.

See:

• Some luvit-loader fixes/improvements lit#191 (comment)
• https://github.com/lua/lua/blob/5.1.x/src/loadlib.c#L454-L461

That is, relative requires are only truly relative on their first require, and if you require a module with the same relative path from a new location, you will get the cached value of the first require rather than the return from requiring the intended relative module.

Off the top of my head, this could be addressed by making relative requires relative to the bundle root rather than the current file.

[squeek502]

Also, feel free to use the luvit-loader require tests I adapted for luver (it's mostly the same as the Luvit require tests, with a few extra tests). The method I used to fake the currently executing file is a bit strange, but it works.

EDIT: Also worth noting that luvit-loader removes the circular dependency support that Luvit 2.0's require had.

[squeek502]

Here's the error I'm getting on Windows when doing make test:

luvit-loader>make test
"Testing luvit"
Uncaught exception:
...\luvit-loader\deps\dns.lua:101: attempt to redefine 'ERROR_SUCCESS' at line 7
stack traceback:
        [C]: in function 'cdef'
        ...\luvit-loader\deps\dns.lua:101: in main chunk
        [string "bundle:luvit-loader.lua"]:216: in function <[string "bundle:luvit-loader.lua"]:211>
        [C]: in function 'require'
        ...\luvit-loader\tests\test-dns.lua:19: in main chunk
        [string "bundle:luvit-loader.lua"]:216: in function <[string "bundle:luvit-loader.lua"]:211>
        [C]: in function 'require'
        ...\luvit-loader\tests\run.lua:37: in main chunk
        [C]: in function 'dofile'
        [string "bundle:main.lua"]:120: in function 'main'
        [string "bundle:init.lua"]:51: in function <[string "bundle:init.lua"]:49>
        [C]: in function 'xpcall'
        [string "bundle:init.lua"]:49: in function 'fn'
        [string "bundle:deps/require.lua"]:309: in function <[string "bundle:deps/require.lua"]:265>

[squeek502]

The Windows build error was due to the require auto-register in Luvi + require being included through package.lua:

https://github.com/luvit/luvi/blob/master/src/lua/luvibundle.lua#L327-L333

Weirdly enough, the same thing that affected Lit a while back is the reason why it still worked in the Linux CI (note also this might mean that the require auto-register in Luvi never worked in the Linux CI). (EDIT: Actually, unsure about this one--it is probably something similar but not exactly the same issue)

[creationix]

Thanks @squeek502

[squeek502]

This needs some special handling to maintain compatibility with things like luvit . redirecting to ./init.lua. Currently getting this error when doing luvit .:

C:\luvit-relative-require>luvit .
Uncaught exception:
cannot open C:\luvit-relative-require: Permission denied
stack traceback:
        [C]: in function 'dofile'
        [string "bundle:main.lua"]:120: in function 'main'
        [string "bundle:init.lua"]:51: in function <[string "bundle:init.lua"]:49>
        [C]: in function 'xpcall'
        [string "bundle:init.lua"]:49: in function <[string "bundle:init.lua"]:20>

[creationix]

That's an edge case I don't mind dropping support for. The closer we can get to vanilla lua behavior the better.

[squeek502]

To attempt to demonstrate the issue I brought up with relative requires, here's a simple (and somewhat likely) example:

File structure:

init.lua
deps/
+- one/
  +- package.lua
  +- init.lua
+- two/
  +- package.lua
  +- init.lua

Where the root init.lua contains:

local one = require('one')
local two = require('two')

print("one version: ", one.version())
print("two version: ", two.version())

each init.lua in deps contains:

return {
    version = function()
        local meta = require('./package')
        return meta.version
    end
}

and one's package.lua has 0.0.1 as the version, while two's package.lua has 2.0.0.

Here's the output of running Luvit 2 and Luvit 3 on the root init.lua:

>luvit2 init.lua
one version:    0.0.1
two version:    2.0.0

>luvit3 init.lua
one version:    0.0.1
two version:    0.0.1

There's really no way to get around this problem with a Lua package loader, as the package.loaded lookup and storage is done in the require C implementation, and it always looks up/stores using the originally passed in module name.

[creationix]

:/

[creationix]

So basically, there is no sane way to have relative requires in lua. I guess we could just write a library that doesn't touch require at all (it can import(name) or something)

[squeek502]

Yeah, I think that'd make the most sense--add a separate module specifically for relative requires. Another alternative would be to add a function that resolves a relative path to an absolute path based on the currently executing script, and then chain that into a require, e.g.:

require(resolve('./package'))

so that would then make the module name the absolute path, thereby making sure each module is unique.

[SoniEx2]

Try this:

mymodule.lua

local modname, modpath = ...
print(modname)
if not modpath then
  print("No modpath: Lua 5.1 detected") 
else
  print(modpath)
end
return {helloworld = function() print("Hello World!") end}

something.lua

local mymodule = require"mymodule"
mymodule.helloworld()

Run with lua something.lua or luajit something.lua (not sure if luvit would work here)

This is how you should do relative requires in Lua.

[creationix]

@squeek502 I don't think any of the default loaders support absolute paths. But I guess I could add a simple loader that does.

[squeek502]

@creationix the current loader has basic support for Linux absolute paths (but not Windows): https://github.com/luvit/luvit/pull/932/files#diff-b511e047bb8a8ec669ceeedd21cc8a73R176

@SoniEx2 not sure how that's relevant.

[creationix]

@squeek502 The suggestion by @SoniEx2 is a replacement for the debug.* hack and greatly simplifies the custom loader (in fact we will only need one for bundle paths)
I'm prototyping it now and seeing just how far it is from the original luvit require design.

[creationix]

So this works for basic relative requires without any loader at all for files on disk:

.
├── libs
│   ├── bar
│   │   ├── init.lua
│   │   └── local.lua
│   └── foo
│       ├── init.lua
│       └── local.lua
└── main.lua

-- main.lua
package.path = "./?/init.lua;" .. package.path
require('libs.foo')
require('libs.bar')

-- libs/foo/init.lua
print("Loading foo")
require(... .. ".local")

--- libs/foo/local.lua
print("loading foo.local")

-- libs/bar/init.lua
print("Loading bar")
require(... .. ".local")

--- libs/bar/local.lua
print("loading bar.local")

Output

> luajit main.lua
Loading foo
loading foo.local
Loading bar
loading bar.local

[creationix]

This above solution is very simple and works for some of the use cases. But there are still some problems.

• It requires that main.lua be at the root of the current working directory. If we try to load from another folder it will fail.

> cd ..
> luajit test/main.lua
luajit: test/main.lua:2: module 'libs.foo' not found:
    no field package.preload['libs.foo']
    no file './libs/foo/init.lua'
    no file './libs/foo.lua'
    no file '/usr/local/Cellar/luajit/2.0.4_3/share/luajit-2.0.4/libs/foo.lua'
    no file '/usr/local/share/lua/5.1/libs/foo.lua'
    no file '/usr/local/share/lua/5.1/libs/foo/init.lua'
    no file '/usr/local/Cellar/luajit/2.0.4_3/share/lua/5.1/libs/foo.lua'
    no file '/usr/local/Cellar/luajit/2.0.4_3/share/lua/5.1/libs/foo/init.lua'
    no file './libs/foo.so'
    no file '/usr/local/lib/lua/5.1/libs/foo.so'
    no file '/usr/local/Cellar/luajit/2.0.4_3/lib/lua/5.1/libs/foo.so'
    no file '/usr/local/lib/lua/5.1/loadall.so'
    no file './libs.so'
    no file '/usr/local/lib/lua/5.1/libs.so'
    no file '/usr/local/Cellar/luajit/2.0.4_3/lib/lua/5.1/libs.so'
    no file '/usr/local/lib/lua/5.1/loadall.so'
stack traceback:
    [C]: in function 'require'
    test/main.lua:2: in main chunk
    [C]: at 0x010352e320

The luvit require system works independent of the cwd and thus this is a serious regression.

[creationix]

Also we can't do more advanced searches like find ../../deps/foo as just "foo". This requires some sort of resolve helper like @squeek502 suggested since we can't do it in the loader anymore.

[SoniEx2]

Try setting the module root in package.path. You can do that in luvi, right?

[creationix]

@squeek502 how about this:

• Add a simple require loader to luvi itself that loads bundle paths.
• Write a library that performs luvit style resolve using either ... at caller or debug.* internally for the relative part.
• Write clear docs on the various use cases and how to use the library.

[squeek502]

I like that. It seems Luvit has been moving towards plain Lua conventions for a while, so this just seems like another leap towards that.

What about deps/libs special handling?

[creationix]

That would handled by the resolve library, not in core or a loader.

[squeek502]

That seems like it might be a bit odd, as that would mean you'd need to write different code to require a Lit-style dependency as opposed to a Lua-style one, right? Wouldn't basically all requires in a Lit-style package have to use the resolve helper, thereby making them either 1) incompatible with plain Lua or 2) always depend on the resolve helper?

I feel like I'm probably thinking about this wrong. Could you give an example of what the require calls of something like lit's libs/stats.lua would look like?

[creationix]

You're right that require alone can't do lit style path resolving exactly because of the problem with the cache you found. We have to resolve the relative path to a full path before calling require or the key in the cache will be too ambiguous.

We can either write a resolve library that returns the path and chain it to native require or write a library that replaces require and does the resolve internally.

[squeek502]

You're right that require alone can't do lit style path resolving exactly because of the problem with the cache you found.

I don't think I've said there's a problem with libs/deps resolving (it works fine with the current luvit-loader implementation)--the cache issue only affects module names that overlap, like relative requires that should resolve to different absolute paths.

We have to resolve the relative path to a full path before calling require or the key in the cache will be too ambiguous.

Could you expand on this a bit? Are you talking about something like require('json') being too ambiguous?

The only benefit to resolving that sort of thing beforehand I see would be to allow multiple versions of the same module to be required without them overlapping (e.g. test.lua calling require('json') resolves to deps/json.lua while deps/foo/bar.lua calling require('json') resolves to deps/foo/bar/deps/json.lua). Is that sort of thing what you have in mind?

[creationix]

test.lua calling require('json') resolves to deps/json.lua while deps/foo/bar.lua calling require('json') resolves to deps/foo/bar/deps/json.lua). Is that sort of thing what you have in mind?

Yes. Though lit doesn't support automatically making such deep trees (unlike npm which used to make deep trees by default). I think strongly preferring the shallow style makes most applications saner. I just want to support the edge case of conflicting packages with the same name through manual means for people who really need/want it. A resolve library can implement this logic and pass on lua style module names to require.

[squeek502]

A resolve library can implement this logic and pass on lua style module names to require.

Absolutely, I just don't think it should be the only way to require things in a Lit-style package. require('json') shouldn't need a resolve helper to load deps/json.lua from libs/stats.lua, for example, and that style of generic require is fully compatible with the Lua loader style this PR currently uses.

[SoniEx2]

How do you versioning?

[aiverson]

I think it is entirely possible to make relative requires work without patching the top level require itself or adding a helper. Presumably, if something is doing a relative require in the luvit convention, it is either the root package, or a package in the luvit ecosystem that was loaded by a luvit loader. The cache of resolved paths is exposed in package.loaded and is editable. The luvit loader or loaders can keep a hidden state. Whenever a luvit loader is invoked to resolve a relative path, it can unset the package.loaded value for the package stored in the given state if any, then write its own package name into that state. Any relative requires required from that included file will get recorded into the frame on top of the state. When the file being included returns to the loader but before the loader returns to require, the loader unsets the package.loaded values corresponding to the loads recorded in the top frame and pops it from the stack. This guarantees that the cache values for relative path requires won't be present outside of the file that originally required them. Unfortunately, that doesn't allow the loaded cache and relative resolvers to be kept working for code in the file that doesn't run at first load, but I think it may work well enough or be straightforward to fix. And how many libraries need to lazy-load things with require? Injecting an identically behaving require with a separate package.loaded cache into the code loaded by luvit relative loaders would allow that to work.

[creationix]

@aiverson sounds interesting, I'm not sure I quite understand your proposal.

[squeek502]

I think he's basically suggesting what luvit's require module does now (store it's own cache of loaded modules, separate from package.loaded). I'm not sure it's possible to re-implement that scheme without overriding the require function, though, since Lua package loaders have limited control over package.loaded in the necessary places.

Here's the require C code for Lua 5.1. It:

• Checks package.loaded first; if the key is found, it simply returns the value
• If the key is not found, it loops through package.loaders, calling each with the passed in module name
• If a loader returns a module, then it sets package.loaded[name] to the returned module and returns it back to the caller; otherwise (if no loader returns a module), require fails with an error

So, to use a separate cache, we'd need to either hook before the first step, to invalidate lookups so that we can use our cache as the lookup, or after the last step, to nil out package.loaded[name] for relative modules (so that our loader would be called the next time it's looked up and we can use our cache then). Loaders do not give us access to either of those locations, so I'm unsure how such a thing could be implemented.

[aiverson]

Here is some pseudocode to show my suggestion in more detail.

local held_package
local reldirs = {process_path()}
package.luvitloaded = {}

local function relative_loader(name)
  if held_package then package.loaded[held_package] = nil end
  local source, dir = search_package(name, reldirs[#reldirs])
  local res
  if source then
    reldirs[#reldirs + 1] = dir
    res = package.luvitloaded[source] or dofile(source, name)
    package.luvitloaded[source] = res
    reldirs[#reldirs] = nil
  else
    held_package = nil
  end
  if held_package then package.loaded[held_package] = nil end
  held_package = name
  return res
end

local function absolute_loader(name)
  if held_package then package.loaded[held_package] = nil end
  held_package = nil
  --Don't invalidate the cache on absolute paths
  local source, dir = search_package(name)
  local res
  if source then
    reldirs[#reldirs + 1] = dir
    res = package.luvitloaded[source] or  dofile(source, name)
    package.luvitloaded[source] = res
    reldirs[#reldirs] = nil
  end
  if held_package then package.loaded[held_package] = nil end
  held_package = nil
  return res
end

I hope that helps explain it.

This scheme ensures that all of the loaded cache entries for relative paths based on any directory besides the current one are invalidated before require returns. The last required relative path persists, so it is available if the same package is required again in the same file, but it is guaranteed to be invalidated before code executed by a luvit loader based on a different directory. This does require that modules using relative luvit requires must require any relative path on first run before returning, but I think that is both fixable and a reasonable restriction.

[squeek502]

Could you go into a bit more detail about what problem you're trying to solve? If it's the problem I outlined in this comment, then I'm confused about how it's addressed by your code. I might be missing something, though, as I'm unsure what held_package is meant to be doing.

Just to be clear, this is the problem as I see it (if we're only adding loaders and not implementing a full require replacement):

-- this is the C require function loosely converted to Lua code
function require(name)
  if package.loaded[name] then
    return package.loaded[name]
  end

  -- this is where the loaders get called, 
  -- and one loader **must** return a result, or require will error
  local res = ...  

  -- the C code then sets package.loaded[name] to the returned value (or true)
  package.loaded[name] = res or true
end

-- so, calling require with the same module name from different files 
-- will just return the first one required

-- from /foo/bar.lua
-- returns the value from /foo/relative.lua
-- and sets package.loaded['./relative']
require('./relative')

-- from /bar/foo.lua
-- should return the value from /bar/relative.lua, 
-- but actually returns the cached /foo/relative.lua
-- from package.loaded['./relative']
require('./relative')

Unless I'm missing something, this is intractable whenever the same module name is passed to require, as the C require code will bypass the loaders entirely if package.loaded['./relative'] exists, and the C require code will always set package.loaded['./relative'] after the loaders are run (so setting package.loaded['./relative'] to nil in a loader ends up doing nothing).

[aiverson]

In this case, the loader that is loading foo/bar.lua will unset package.loaded['./relative'] before returning so that the cache entry isn't found when bar/foo.lua is being loaded. held_package just points at the cache entry that needs to be deleted next. This can break if there are non-luvit modules that are using luvit relative requires, but a non luvit module shouldn't be using luvit relative requires.

[squeek502]

the loader that is loading foo/bar.lua will unset package.loaded['./relative'] before returning so that the cache entry isn't found when bar/foo.lua is being loaded

This isn't possible.

the C require code will always set package.loaded['./relative'] after the loaders are run (so setting package.loaded['./relative'] to nil in a loader ends up doing nothing).

[aiverson]

Yes it is. The call stack will look like this.

foo/relative.lua
relative_loader("./relative")
require("./relative")
foo/bar.lua
absolute_loader("foo.bar")
require("foo.bar")
main.lua

When require('./relative.lua') returns, package.loaded will have './relative' in it. This is fine because the flow of control hasn't moved into a different directory yet. absolute_loader('foo.bar') will unset that before it returns to require('foo.bar') and it will remain unset when require('foo.bar') returns to main.lua. main.lua will subsequently require bar.foo. when bar/foo.lua calls require('./relative') the cache entry will not exist.

[squeek502]

Ah, ok, I see now, thanks for explaining.

[creationix]

Interesting technique.