Bring Lua to 5.4.6.

[michaellenaghan]

This essentially re-does the work of #875 on top of master.

This is what I did to check that Cosmo's Lua extensions still worked:

$ build/bootstrap/make MODE=aarch64 o/aarch64/third_party/lua/lua
$ ape o/aarch64/third_party/lua/lua
>: 10
10
>: 010
8
>: 0b10
2
>: string.byte("\e")
27
>: "Hello, %s" % {"world"}
Hello, world
>: "*" * 3
***

I'm not sure how to test luaL_traceback2()?

This is what I did to check that Lua itself still worked:

$ cd third_party/lua/test/
$ ape ../../../o/aarch64/third_party/lua/lua all.lua

There's one test failure, in files.lua:

***** FILE 'files.lua'*****
testing i/o
../../../o/aarch64/third_party/lua/lua: files.lua:84: assertion failed!
stack traceback:
[C]: in function 'assert'
files.lua:84: in main chunk
(...tail calls...)
all.lua:195: in main chunk
[C]: in ?
.>>> closing state <<<

That isn't a result of these changes; the same test is failing in master.

The failure is here:

if not _port then   -- invalid seek
  local status, msg, code = io.stdin:seek("set", 1000)
  assert(not status and type(msg) == "string" and type(code) == "number")
end

The test expects a seek to offset 1,000 on stdin to fail — but it doesn't. status ends up being the new offset rather than nil.

If I comment out that one test, the remaining tests succeed.

[michaellenaghan]

@mrdomino Over to you. :-)

[mrdomino]

Yay! Welcome back! Taking a look now.

[mrdomino]

This is great, very exciting to see you back.

I don't know about luaL_traceback2 either yet. I'll try and think about this some more in the next day or two, but please feel free to badger me and/or @jart on redbean discord to get some attention on it.

I was curious about the all script at the root of the upstream repo - it seemed to be setting a ulimit, but I couldn't get the libs to build for Apple Silicon reasons, so I couldn't tell if it would make that one failing test case pass or not. (I haven't thought too carefully about it yet beyond just noticing that 1100 is close to 1000.)

Overall this looks correct and like it is applying the upstream diff from 5.4.3..5.4.6. I have added a few comments but other than those and the traceback function this is looking pretty good to me.

[mrdomino]

Oh I guess one other thing to ask - how closely did you check that the modifications described in README.cosmo aside from the traceback function work?

(Others can correct me if I'm wrong but) I don't think it's a showstopper if some of them break - we can fix or revert them later - but it'd be good to know what to expect going in.

[mrdomino]

Also cc @pkulchenko

[pkulchenko]

luaL_traceback2 was used to show the stack trace with parameter values; it's used in LuaCallWithTrace, which is used in Redbean to run Lua code. You should be able to see the extended stack trace by running something like this: redbean -e "function a(b)c()end a(2)" (with "params" indicating the extended stack trace):

stack traceback:
        [string "function a(b)c()end a(2)"]:1: in function 'a', params: b = 2;
        [string "function a(b)c()end a(2)"]:1: in main chunk

I confirmed that I get the expected result with the updated code.

how closely did you check that the modifications described in README.cosmo aside from the traceback function work?

I think that's what @michaellenaghan's initial comment showed, as those were all extensions that @jart put in.

[michaellenaghan]

I was starting to respond to comments individually, but I realized it would probably be better to set the scene in one place.

This PR consists of 2,591 additions and 1,373 deletions (by Github's count) across 90 files.

When I was trying to figure out which additions and deletions to make, I was constantly confronted with diffs like this:

and this:

and this:

(In those diffs Cosmo's Lua is on the left and Lua 5.4.6 is on the right.)

In the first diff, you can see there's an enum in Lua that isn't in Cosmo's Lua.

In the second diff, you can see there's a case in Cosmo's Lua that isn't in Lua.

In the third diff, you can see there's a function in Lua that isn't in Cosmo's Lua.

Are those Cosmo changes, Lua changes, or a bit of both? It took effort to figure it all out.

As I did, I tried to mark Cosmo changes to make life easier for future maintainers. I probably missed some somewhere, but I'm sure I've left things better than they were!

One of your comments said:

hmm, why is this here? Probably delete and make a note of it in README.cosmo

Maybe that helps to answer the question? Now when someone diffs and sees the missing enum, the start of the comment in Cosmo will line up with the comment in Lua and explain what's going on.

I think your comment implicitly assumed that the changes in the README were the only changes made to Lua? That isn't the case, not by a longshot; I think the README only documents user-visible changes.

More importantly, my goal wasn't to document Cosmo's changes — it was to make life easier for the next person who has to diff and merge upstream changes.

===

In doing this work I had to look at files over and over, and spotted some unintentional inconsistencies. For example, I think two of the files had the old Lua-style comments at the top, rather than the new Cosmo-style comments. And I think two of the files were missing the statically yoinked copyright notices.

They aren't part of the 5.4.6 changes, they're just mistakes I noticed while making the 5.4.6 changes, and I fixed them.

===

A year ago I tried to make this process easier by creating a series of stacked PRs.

The first one, #872, did a "re-sync" to 5.4.3. It didn't change any code; it just normalized some vertical whitespace (in part to make the upcoming Lua diffs easier), added some missing "[jart]" comments, added some missing copyright notices, etc. The whole point was to separate the non-substantive changes from the upcoming substantive ones.

Then I layered the Lua changes on top of that, one version at a time, in the hope that that would provide a better audit trail.

But the time wasn't right, and I don't think anyone cared for (or about) the stacked PR approach, so now we're at the other extreme: one PR that does all of those things at once — and, unfortunately, mixes non-substantive changes with substantive ones.

===

I'm going to go back now and respond to the individual comments, and sometimes refer back to this one.

[michaellenaghan]

I was curious about the all script at the root of the upstream repo - it seemed to be setting a ulimit, but I couldn't get the libs to build for Apple Silicon reasons, so I couldn't tell if it would make that one failing test case pass or not. (I haven't thought too carefully about it yet beyond just noticing that 1100 is close to 1000.)

Just to make sure we're on the same page: I didn't run Lua's tests in the upstream repo, I ran them here, in Cosmo's repo. My exact commands are in the initial comment — and, as you can see, I ran them on Apple Silicon.

[michaellenaghan]

how closely did you check that the modifications described in README.cosmo

That was in my initial comment:

$ build/bootstrap/make MODE=aarch64 o/aarch64/third_party/lua/lua
$ ape o/aarch64/third_party/lua/lua
>: 10
10
>: 010
8
>: 0b10
2
>: string.byte("\e")
27
>: "Hello, %s" % {"world"}
Hello, world
>: "*" * 3
***

Decimal number, octal number, binary number, "\e", etc.

[michaellenaghan]

Maybe that helps to answer the question? Now when someone diffs and sees the missing enum, the start of the comment in Cosmo will line up with the comment in Lua and explain what's going on.

I realized that I could illustrate what I meant with updated versions of the screenshots:

===

===

Much easier to understand, no?

[mrdomino]

Now when someone diffs and sees the missing enum, the start of the comment in Cosmo will line up with the comment in Lua and explain what's going on.

I think your comment implicitly assumed that the changes in the README were the only changes made to Lua? That isn't the case, not by a longshot; I think the README only documents user-visible changes.

More importantly, my goal wasn't to document Cosmo's changes — it was to make life easier for the next person who has to diff and merge upstream changes.

Yeah, this makes sense and does seem like it provides a much better affordance for future contributors than just the README.cosmo summary.

It would be nice for README.cosmo to actually detail everything that we changed, but it sounds like that ship may have sailed for the time being.

It would be really nice to be able to somehow apply the diff from 5.4.3..5.4.6 as a three-way merge and get conflicts out of it, but this is probably beyond the capabilities of git, and almost certainly beyond the capabilities of the way we're currently using git.

I think the only thing left is to bikeshed the format of the tag - as the guy who pushed this particular boulder up the mountain (how many times now), you have a big stake in that; but I would like to suggest maybe // [cosmo] instead of // [jart] as the color for this one.

For example, I think two of the files had the old Lua-style comments at the top, rather than the new Cosmo-style comments. And I think two of the files were missing the statically yoinked copyright notices.

Ok, I just hadn't seen the static yoinks in the other files. There was maybe one case where an old header got removed but the new one didn't get put in yet, that you'll probably have addressed individually.

Just to make sure we're on the same page: I didn't run Lua's tests in the upstream repo, I ran them here, in Cosmo's repo. My exact commands are in the initial comment — and, as you can see, I ran them on Apple Silicon.

Ok, so you ran the upstream lua tests against the cosmopolitan tree .lua files?

What happens if you do this before running all.lua?

ulimit -S -s 1100

Will now proceed to look at the individual comments.

[michaellenaghan]

It would be nice for README.cosmo to actually detail everything that we changed, but it sounds like that ship may have sailed for the time being.

Indeed. You can get a rough sense, though, by searching for "[jart]" in the lua/ tree. There are 29 matches in 16 files. (That's definitely a lower bound, not an upper one; there are changes that, for various reasons, I didn't mark.)

It would be really nice to be able to somehow apply the diff from 5.4.3..5.4.6 as a three-way merge and get conflicts out of it, but this is probably beyond the capabilities of git, and almost certainly beyond the capabilities of the way we're currently using git.

Yes, that's a beautiful dream. :-) I'm using a paid-for diff tool that supports three-way merging. I dunno, man. It's still a very high-intensity effort. Well, it is for me anyway!

I think the only thing left is to bikeshed the format of the tag - as the guy who pushed this particular boulder up the mountain (how many times now), you have a big stake in that; but I would like to suggest maybe // [cosmo] instead of // [jart] as the color for this one.

I agree, but... I used "[jart]" because "[jart]" was already being used. If you grep across Cosmo you'll see 167 matches in 89 files. Only 16 of those files are in lua/. So, you know, that seems to be the current standard.

I suggest that if you want to change the tag you change it in one PR, across the entire codebase; I don't want to introduce a lua/-only change.

Ok, I just hadn't seen the static yoinks in the other files. There was maybe one case where an old header got removed but the new one didn't get put in yet, that you'll probably have addressed individually.

There are different things happening in different files.

The first thing to be aware of is that some of the files in the dir — maybe 20 or so — were added by Cosmo. They have different rules: Cosmo copyrights at the top, and no __static_yoinks.

The second thing to be aware of is that Lua's .h and .c files also have different rules. The .h files removed the comments at the top; the .c files replaced them — and added __static_yoinks.

Ok, so you ran the upstream lua tests against the cosmopolitan tree .lua files?

No no no. Cosmo's Lua has its own copy of Lua's tests; I linked to them in my comment! And I pointed out that the exact commands I used were right there in the first comment. You can do what I did to run them.

If you run Cosmo's Lua directly against the upstream tests various tests will fail due to Cosmo's changes. Cosmo's copy of the tests have been modified to accommodate its changes. (Well, kind of; some tests have just been commented out.)

[mrdomino]

I suggest that if you want to change the tag you change it in one PR, across the entire codebase; I don't want to introduce a lua/-only change.

Yeah, sounds good to me.

No no no. Cosmo's Lua has its own copy of Lua's tests; I linked to them in my comment!

Yeah, right, ok - that's what I thought.

I'm still curious what happens if you set that ulimit before running the tests. I might try if I get a chance but I'm in the middle of a few things at the moment.

Anyway we're getting closer.

[michaellenaghan]

I'm still curious what happens if you set that ulimit before running the tests. I might try if I get a chance but I'm in the middle of a few things at the moment.

$ ulimit -s
8176
$ ulimit -S -s 1100
$ ulimit -s
1100
$ ape ../../../o/aarch64/third_party/lua/lua all.lua
< ... same results ... >
$ ulimit -h
...
-s or --stack-size
       The maximum stack size.
...

Did you really mean -s?

[mrdomino]

Huh, bummer.

Yeah - it's from the upstream lua all (not all.lua) script. They do ulimit -S -s 1100 before running lua -W all.lua.

[mrdomino]

(Of course, it's possible that they meant to do a different ulimit - there seems to be at least one other typo in that file on the v5.4.6 tag.)

[michaellenaghan]

(Of course, it's possible that they meant to do a different ulimit - there seems to be at least one other typo in that file on the v5.4.6 tag.)

Ah, OK, now I see what you're talking about.

I'm only guessing, but a) Lua is used in constrained environments; and b) maybe they're specifically trying to verify that it will work in those constrained environments?

I'll call that "plausible." :-)

[mrdomino]

The notes in README.cosmo look great to me - though I hate to be pedantic, it is missing a trailing newline atm. 🙂

I'm satisfied with this now. Update the README and give me your final okay, and I will push go.

[pkulchenko]

Looks good to me as well!

[michaellenaghan]

I took one last pass through, and found one symbol that needed to change.

I think we're good to go.

But... :-)

Lua has an odd way of building in API-level "debug" support; you use something like -DLUA_USER_H='"ltests.h"'. That ends up including ltests.h, which then has the effect of compiling ltests.c. If you do that, you'll discover that ltests.c doesn't actually compile.

That isn't a problem with this change; it happens in master too. From a quick look, I think the problem is mostly — maybe entirely? — missing system-level #includes.

Not a stop ship thing, and maybe not even a worth fixing thing, but a thing to know about.

Let's push go!

[mrdomino]

All right, great. I would love it if you could file bugs for errata like these so we can go back and take a look at them later.

[michaellenaghan]

I would love it if you could file bugs for errata like these so we can go back and take a look at them later.

Done! #1225, #1226