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

Fix a race condition between Lua and LuaRocks #234

Merged

Conversation

sage-etcher
Copy link
Contributor

Lua and LuaRocks both set (overwrite) the LUA_PATH variables, meaning which ever's Environment file is sourced last, is the only one taken into account. This fixes that for Lua >= 5.3.0 and LuaRocks >= 3.0.1 by making the change to append their paths instead of overwriting.

w/o this some packages fail to build when they need to use LuaRocks modules, one example is Awesome 4.3

#128

@Nuc1eoN
Copy link
Member

Nuc1eoN commented May 6, 2024

To me it makes sense, not sure why it was set to overwrite the LUA_PATH varables. I suppose it was an oversight.

But since @hishamhm is our LuaRocks guru and created the recipes, I'm gonna let him be the judge :)

@Nuc1eoN Nuc1eoN requested a review from hishamhm May 6, 2024 18:29
Applied to modern-ish versions only, Lua >= 5.3.0, LuaRocks >= 3.0.1
Fixing a race condition of when each's 'Environment' file gets sourced.
@hishamhm
Copy link
Member

hishamhm commented May 8, 2024

Yes, the changes make sense! I don't recall the context where this came to be, but most likely the Lua and LuaRocks recipes were made at different times, and most likely I didn't catch this behavior because I'm always running some weird in-development version of LuaRocks.

One small note is that the ;; has special meaning in Lua, meaning "expand the hardcoded defaults here". If I'm reading the changes in this PR, the Lua recipes are prepending and adding a ; in the end, and the LuaRocks recipes are appending and adding a ; at the front. This will produce a ;; in the middle. Lua's hardcoded defaults are usually pretty standard /usr or /usr/local based paths, though, so these paths should turn out to be equivalent to our /System/Index paths and all should work well. So that's probably harmless, so +1 from me on this PR!

(Speaking of Lua and LuaRocks recipes, one quirk that happens in my — admittedly very outdated — Gobo machine is that my Awesome installation requires /Programs/Lua/Current to be Lua 5.3, and if I switch my Current to Lua 5.4, Awesome fails to start up (I think it links to liblua.so instead of liblua.so.5.4 or similar?), so whenever I need to switch Lua versions, I end up using SymlinkProgram to switch temporarily and then back. I can't verify exactly the details of the behavior is because I currently don't have access to my Gobo machine (don't worry, I'm safe) )

@sage-etcher
Copy link
Contributor Author

One small note is that the ;; has special meaning in Lua, meaning "expand the hardcoded defaults here".

Interesting note! I wish I could take credit and say it was intentional, but it wasn't- though it's good to know, thanks! =DD

my Awesome installation requires /Programs/Lua/Current to be Lua 5.3, and if I switch my Current to Lua 5.4, Awesome fails to start up (I think it links to liblua.so instead of liblua.so.5.4 or similar?), so whenever I need to switch Lua versions, I end up using SymlinkProgram to switch temporarily and then back.

Yes, I've been looking into this exact issue. It's due to the lua_newuserdata function from 5.3 being changed to a preprocessor macro in 5.4. Recompiling Awesome fixes the issue, so long as you have the needed Luarocks modules installed for Lua54.

The full thread on the issue can be found here at issue #128.

@Nuc1eoN
Copy link
Member

Nuc1eoN commented May 9, 2024

I can't verify exactly the details of the behavior is because I currently don't have access to my Gobo machine (don't worry, I'm safe) )

Oh gosh! Can't believe have been a victim of that.. Glad that you're safe 👍


About the Lua 5.4 issue with Awesome, yes as pointed out we are working on that in #12, and this PR here is part of that effort. It has been quite a rabbit hole.

Thank you for the review of this PR :)

@Nuc1eoN Nuc1eoN merged commit dcd7442 into gobolinux:master May 9, 2024
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

Successfully merging this pull request may close these issues.

None yet

3 participants