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

node.LFS.reload should detect before reboot if the LFS image given is from 5.1 luac.cross #3268

Open
mk-pmb opened this issue Sep 1, 2020 · 9 comments
Assignees

Comments

@mk-pmb
Copy link
Contributor

mk-pmb commented Sep 1, 2020

Expected behavior

  • cd app/lua/luac_cross && make LUA=53 clean all either builds a useful executable or refuses with a link to this issue to explain why we don't support i686. (Update: The command was wrong. The LUA=53 had no effect at the time, and built with LUA 5.1 regardless.)
    • I'd accept "nobody uses 32 bit CPUs anyway" if someone is bold enough to make that claim.
  • node.LFS.reload() detects before reboot that the reboot would fail, and thus, would not reboot at all.

Actual behavior

When I create an LFS image with luac.cross compiled from commit b4c148e (current dev branch) with LUA=53 on Ubuntu on an x86_64 machine, it works as expected. (Update: Because it was from a build of the entire project, thus using the luac from lua53. At the time I didn't know there was a difference, and thus paid no attention.)

When I try the same (Update: same luac command, but with luac.cross compiled using the errornous command from above) on an i686 machine, it goes through all the motions with no sign that anything is wrong, and not even the pass 1 check of node.LFS.reload complains. Only after the MCU reboot it fails with (null): invalid header in precompiled chunk.

Test code

-- -*- coding: UTF-8, tab-width: 2 -*-
return function () print('Hello.') end
( cd app/lua/luac_cross && make LUA=53 clean all
) && sudo mv -v luac.cross /usr/local/bin/luac-for-nodemcu
luac-for-nodemcu -f -o hello.$(uname -m).lfs hello.lua

NodeMCU startup banner

See UART LUA session log.

Hardware

LoLin v3 ESP8266

@TerryE TerryE changed the title On i686 GNU/Linux, luac.cross sneakily builds invalid LFS images that fool even node.LFS.reload node.LFS.reload fails to load LFS images build on 32-bit luac.cross (i686 GNU/Linux) Sep 2, 2020
@TerryE
Copy link
Collaborator

TerryE commented Sep 2, 2020

If you look at the code, then you will see that we support both 32-bit and 64-bit hosts. I understand your report that reload isn't working for an LFS image built on a 32-bit host, but at worst this is a bug and not some sort of deep-state conspiracy, so please avoid this type of wording. You are free to use sarcasm when you are paying for my time; it really doesn't help when I am giving it pro bono.

PS. Your first link is broken.

@TerryE
Copy link
Collaborator

TerryE commented Sep 2, 2020

OK, I have just tried an image build on my i5 laptop and an RPi4 running 32-bit rasbian compiling _init.lua into an LFS image. They are identical, so we haven't got fundamental error here. You will need to fix the link before I can do more. I might have some old 32-bit VM images that I can spin up, but I can't see any reason for the output from a 32-bit ARM compiled version and a 32-bit x86 compiled version to be different.

@TerryE
Copy link
Collaborator

TerryE commented Sep 2, 2020

On reflection saying as there isn't an error in RPi variants and I hope to have the next feature release within the week, I will check that for 32-bit x86 working.

@mk-pmb
Copy link
Contributor Author

mk-pmb commented Sep 3, 2020

Sorry if this came accross the wrong way, it wasn't meant against you or anyone in person. The wording "sneakily" was intended to be funny, I guess it wasn't after all. Sorry again.
The 32bit remark was to clarify that it's not overly important to me to continue the 32bit support. Lots of projects (e.g. Ubuntu) have chosen to drop 32bit versions lately, so considering the scarcity of C devs here, I considered the possibility that, if this bug could be too cumbersome, we might follow them. (Not saying we should though.)

No idea why the Wayback Machine deleted the file. I fixed the first link to point to the live version instead.

@mk-pmb
Copy link
Contributor Author

mk-pmb commented Sep 3, 2020

By accident I discovered that there is a luac in the lua53 directory as well, its subdirectory called "host". At first I couldn't find it because I had only searched for files with "cross" in the name.
That one works perfectly. So I guess the correct fix for the first part is to make the 5.1 luac Makefile refuse to build if the "LUA" variable is not empty, with a hint where the 5.3 luac is. I'll prepare a PR for that.

Edit: This also explains why it worked on the 64bit machine. There I used the default luac.cross which of course was the one from lua53. I didn't pay attention to that detail because at the time I had assumed there's only one luac.cross.

@mk-pmb mk-pmb changed the title node.LFS.reload fails to load LFS images build on 32-bit luac.cross (i686 GNU/Linux) node.LFS.reload should detect before reboot if the LFS image given is from 5.1 luac.cross Sep 3, 2020
@TerryE
Copy link
Collaborator

TerryE commented Sep 6, 2020

The makes are a bit cludgy when you swap versions. you really need to make LUA=51 clear before doing the make LUA=53 and v.v.

@stale
Copy link

stale bot commented Sep 7, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 7, 2021
@mk-pmb
Copy link
Contributor Author

mk-pmb commented Sep 8, 2021

I still want this.

@TerryE
Copy link
Collaborator

TerryE commented Sep 26, 2021

OK, let's not close this.

@stale stale bot removed the stale label Sep 26, 2021
@TerryE TerryE self-assigned this Sep 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants