Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

getfenv() is gone in Lua 5.3 #3156

Closed
vsky279 opened this issue Jun 11, 2020 · 11 comments
Closed

getfenv() is gone in Lua 5.3 #3156

vsky279 opened this issue Jun 11, 2020 · 11 comments

Comments

@vsky279
Copy link
Contributor

vsky279 commented Jun 11, 2020

I'm sorry I don't have time to study the issue myself now. So maybe I am reporting something while the issue is on the other side of the keyboard. I guess this has some easy solution.

Expected behavior

LFS example working under Lua 5.3

Actual behavior

node.flashindex("_init")()
Lua error: _init.lua:55: attempt to call a nil value (global 'getfenv')
stack traceback:
	_init.lua:55: in main chunk
	stdin:1: in main chunk
	[C]: in ?
	[C]: in ?

Test code

Standard LFS example loaded in LFS.

node.flashindex("_init")()

NodeMCU startup banner

NodeMCU 3.0.0.0 built with Docker provided by frightanic.com
	branch: watertank
	commit: 78ab92f24021bb843e611f4daac0124649304f7f
	release: 2.0.0-master_20170202 +523
	release DTS: 202006112149
	SSL: false
	build type: float
	LFS: 0x10000 bytes total capacity
	modules: enduser_setup,file,gpio,http,net,node,rtcmem,rtctime,sjson,sntp,sun,tmr,uart,wifi
 build 2020-06-11 22:06 powered by Lua 5.3.5 on SDK 3.0.1-dev(fce080e)

Hardware

ESP8266 ESP-12E

@lmercucci
Copy link

lmercucci commented Jun 11, 2020 via email

@nwf
Copy link
Member

nwf commented Jun 12, 2020

This is not a LFS issue; Lua 5.2 (and therefore 5.3) removed the getfenv (and setfenv) functions. See http://www.lua.org/manual/5.2/manual.html#8 for details.

@TerryE will have to update his _init.lua example.

@nwf nwf changed the title LFS under Lua 5.3 getfenv() is gone in Lua 5.3 Jun 12, 2020
@TerryE TerryE self-assigned this Jun 12, 2020
@TerryE
Copy link
Collaborator

TerryE commented Jun 12, 2020

Lua has subversion releases, e.g 5.1.x and these are not breaking, but going from Lua 5.1 to 5.3 is itself a breaking change. I've tried to implement as many of the 5.1 compatibility features as possible but as Nathaniel says the environment architecture was a breaking change on 5.2.

We need really need to go through all examples and modules including the lfs ones to mitigate this and try to provide backwards compatibility. I'll do this in the next day or so.

In the meantime, as a workaround, you will see that LFS is now implemented in the core and so LFS.modname() etc. works without needing to run any _init function. If you want to review the implementation, see app/lua53/lnodemcu.c:L279.

@TerryE TerryE removed the bug label Jun 12, 2020
@HHHartmann
Copy link
Member

Will there also be a solution for loadfile/dofile?
I use them to allow LFS overrides from SPIFFS

@TerryE
Copy link
Collaborator

TerryE commented Jun 12, 2020

If you want search lists, why not just stick to require ()?
And let me go to sleep! 🥴

@vsky279
Copy link
Contributor Author

vsky279 commented Jun 14, 2020

It seems to me that simple modification should fix the issue:

- local G=getfenv()
+ local G=_ENV or getfenv()
  G.LFS = setmetatable(lfs_t,lfs_t)

However I don't know what's wrong now as I get immediate restart with node.flashindex() (e.g. node.flashindex("telnet").

@TerryE
Copy link
Collaborator

TerryE commented Jun 14, 2020

Lukas, that's because _G already has a metatable that you don't want to overwrite. See luaopen_base() in app/lua53/lbaselib.c. All of the Lua base functions and the modules are in a ROTable called ROM, which is why you can do a for k,v in pairs(ROM). The open code effectively does a setmetatable( _G, {__index=ROM}). That's how all of these functions and libraries are accessible through the default environment without taking up entries in a table in RAM.

ROM already includes LFS so why try to replace it?

@nwf
Copy link
Member

nwf commented Jun 15, 2020

I suspect @vsky279's objective is a replacement _init.lua (ETA: or, more generally, initialization procedure) that works on both 5.1 and 5.3, while the global symbol LFS being provided by the firmware itself is (AFAICT) new to 5.3?

@TerryE
Copy link
Collaborator

TerryE commented Jun 15, 2020

the global symbol LFS being provided by the firmware itself is (AFAICT) new to 5.3?

Yes. This means that new Lua developers can use LFS from the get-go without needing an _init module, but if we want to avoid a break here then all we need is some ìf _VERSION == 'Lua 5.1' then logic in the module.

ALso the whole idea of _init and the other lua_examples/lfs was that they would be just that examples to act as a template for apps developers to build on. Moving from 5.1 to 5.3 is a migration process.

@vsky279
Copy link
Contributor Author

vsky279 commented Jun 16, 2020

As @nwf guessed, my goal was to have _init.lua compatible with both 5.1 and 5.3 Lua.
It's nice we can use LFS from scratch in 5.3. Though my understanding is that we still need to define package.loaders[3] to make require work with LFS.
loadfile and dofile should be redefined too.

So executing whole _init.lua but parts mentionned above under if _VERSION == 'Lua 5.1' then should do the thing.

See _init.lua

@nwf
Copy link
Member

nwf commented Jun 16, 2020

FWIW, I think

package.loaders[3] = function(module) -- loader_flash
  local fn, ba = index(module)
  return ba and "Module not in LFS" or fn
end

can just be

package.loaders[3] = function(module) return LFS[module] end

and similarly for the other uses of node.flashindex (aka index) in your _init.lua mod.

Regardless of that, tho', please do feel free to open a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants