MightyPirates / OpenComputers

Home of the OpenComputers mod for Minecraft.
https://oc.cil.li
Other
1.59k stars 430 forks source link

[lua] require not respecting relative or absolute pathing #2967

Closed myrddraall closed 6 years ago

myrddraall commented 6 years ago

When requiring a module using a relative path

 local mymod = require("./MyMod");

the module will be loaded from the various lib/ directories first. if there is a naming conflict the wrong module is loaded. if the path is not relative to the programs cwd then the module is not loaded at all

with absolute paths

 local mymod = require("/etc/myStuff/MyMod");

the module generally wont be found at all

To fix this i've modified the require function inside /lib/package.lua

function require(module)
  checkArg(1, module, "string")

  --[[
    Change #1
       Check if the module path is relative and if so convert to an absolute path.
       By changing the module variable to the absolute path 
       modules are only loaded once for each file, while maintaining the ability to 
       load multiple modules with the same "name"
  ]]
  if module:sub(1, 1) == "." then
    local fs = require("filesystem")
    local basePath = fs.path(debug.getinfo(3).source:sub(2))
    module = fs.concat(basePath, module)
  end
  -- end Change #1

  if loaded[module] ~= nil then
    return loaded[module]
  elseif not loading[module] then
    local library, status, step

    --[[
      Change #2
        if the module has an absolute path then do not search the lib directories
    ]]
    if module:sub(1, 1) == "/" then
      step, library, status = "not found", package.searchpath(module, "?.lua;?/init.lua")
    else
      step, library, status = "not found", package.searchpath(module, package.path)
    end
    -- end Change #2
    if library then
      step, library, status = "loadfile failed", loadfile(library)
    end

    if library then
      loading[module] = true
      step, library, status = "load failed", pcall(library, module)
      loading[module] = false
    end

    assert(library, string.format("module '%s' %s:\n%s", module, step, status))
    loaded[module] = status
    return status
  else
    error("already loading: " .. module .. "\n" .. debug.traceback(), 2)
  end
end

Hopefully this or something like it can be implemented into OpenOS as it makes require much more consistent

payonel commented 6 years ago

our require code should respect package.path exactly like real life lua if you find that our require does not respect package.path as you see in the real world, give me a repro of that specifically.

I have tested your example, and it works fine. If you want relative paths to be checked first, change your package.path, not the require code (unless there is a bug with the require code)

Also keep in mind that, like real lua environments, we cache our require results, and will continue to do this. Thus the context of a require load and the relative path WILL ONLY matter for the first time you load it. We are not going to change that.

payonel commented 6 years ago

If you would like to clear the cache of a package you have previously loaded (via require), feel free to use package.loaded[THE_EXACT_STRING_I_USED_TO_LOAD_MY_LIBRARY] = nil

for example:

local my_mod_lib_path = "./MyMod"
package.loaded[my_mod_lib_path] = nil
local my_lib = require(my_mod_lib_path)
--[[ do some testing with my_lib ]]--
myrddraall commented 6 years ago

@payonel

Thus the context of a require load and the relative path WILL ONLY matter for the first time you load it. We are not going to change that.

I wasn't proposing this my fix only ever loads the modules once it just treats require("shell") separately from require("./shell") and if require("./shell") points to the same file as require("/myShellExt/shell") the file is only loaded once

If you want relative paths to be checked first, change your package.path, not the require code (unless there is a bug with the require code)

This work if and only if the relative require path is valid to the pwd of the running program

given the directory structure:

/
  > myprogram
    > somedir
      > c.lua
      > d.lua
      > e.lua
    > a.lua
    > b.lua
    > test1.lua
    > test2.lua
    > test2.lua

and the files:

print("test1.lua")
require("./a");
print("test2.lua")
require("./b");
print("test3.lua")
require("./somedir/e");
print("a.lua")
print("b.lua")
require("./somedir/c")
print("c.lua")
require("./d");
print("d.lua")
print("e.lua")
require("../a");

Test1 A

/myprogram # test1

expected output:

test1.lua
a.lua

output:

test1.lua
a.lua

Test1 A works

Test1 B

/ # myprogram/test1

expected output:

test1.lua
a.lua

output:

test1.lua
/lib/package.lua: module './a' not found

Test 2

/myprogram # test2

expected output:

test2.lua
b.lua
c.lua
d.lua

output:

test2.lua
b.lua
c.lua
/lib/package.lua: module './b' load failed
/lib/package.lua: module './somedir/c' load failed
/lib/package.lua: module './d' not found

This test fails to find d.lua relative to c.lua

Test 3

/myprogram # test3

expected output:

test3.lua
e.lua
a.lua

output:

test3.lua
e.lua
a.lua

This one works, though i'm not sure why the '../' pathing works but the './' in test2 does not

I've never used lua in the real world but this seems inconsistent to me and is certainly not in line with other languages I've used IRL

the fix i proposed is modeled after node.js

the rules it uses are

  1. if the path is relative then look for the file only on the path relative to the file that called require
  2. if the path is absolute use only the absolute path
  3. if the path has no prefix search the node module paths

if these aren't the rules that lua follows and must always respect path then i get why you don't want to change it

how ever the above still seem like broken behavior since they were run with the original package.lua

I ran each test on 2 computers the expected outputs are from the computers with my modifications and the actual outputs from a computer with a fresh OpenOS

payonel commented 6 years ago

If you compare this to a real lua environment, our openos behavior matches in each test case in lua, when you load module or file, you are executing that code from your current context.

In your first test2 example, your cwd is /, and there is no /./a file

In your second test2 example, your cwd is /myprogram, and there is no /myprogram/./d.lua

myrddraall commented 6 years ago

@payonel I get why it doesn't work, just seems broken to me

the one I don't understand is Test3 there is no /myprogram/../a.lua either and yet that one works... maybe it shouldn't?

myrddraall commented 6 years ago

For anyone that is looking into it, I've created a workaround that doesn't modify package.lua

/lib/import.lua

function import(module)
  if module:sub(1, 1) == "." then
    local fs = require("filesystem")
    local basePath = fs.path(debug.getinfo(3).source:sub(2))
    module = fs.concat(basePath, module)
  end
  if module:sub(1, 1) == "/" then
    local oPath = package.path;
    package.path = "?.lua"
    local imported = require(module)
    package.path = oPath
    return imported;
  else
    return require(module)
  end
end

then either use

require("import")
local myLib = import("./myLib")

or you can add

/boot/04_import.lua

require("import");

in which case you can use local myLib = import("./myLib") without the require("import") in each file

payonel commented 6 years ago

it is because require paths drops all .. relative parent pathing, it is a by-design concept in lua, it is related to the sandboxing aspect of the language. In some environments, this would let you load any path in the system.

in the error output, you'll notice that .. becomes an extra /.

payonel commented 6 years ago

try changing your test1.lua to require ../../../../a.lua it'll still work or cd into somedir, and run e.lua directly, it'll fail to find a as well even though it is ../a.lua

myrddraall commented 6 years ago

I see. Well I'm happy with my import wrapper. this way there is no chance of it breaking other code and i get it to work in a way that's intuitive to me

thanks for taking the time to go through it