-
-
Notifications
You must be signed in to change notification settings - Fork 12.3k
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
lua@5.1 & lua: update luarocks to 2.4.4 #21282
Conversation
Don't merge this straight away, looking into a something. |
Alright, this should be good to 🚢 whenever you're happy & CI is happy @ilovezfs. In way of explanation:
(We put the Christmas decorations up at uni since we're all only here for another couple weeks; the above list didn't take me 4 hours to get through 😄) |
Oh, new problem. How fun. Looking into it, again. |
This is "Not great" ™️ but my recommendation is that it is still better than the status quo, and resolves several issues not only around I intend to wander upstream later today and try to initiate some kind of conversation around the issues discovered trying to get it to build the old way, and indeed test whether they're already been resolved in |
I think I'd prefer we not do this. Build steps in |
If necessary I’ll split this into two PRs whilst I talk to upstream, since the Lua formula here remains working as-is. It’s the versioned stuff there’s some issues around. |
Actually 3 PRs maybe, the Lua@5.3 patch & pkg-config updates probably shouldn’t wait much longer sigh. |
I demand at least 4 PRs. |
I mean, I could probably manage it if you're desperate 😉. |
Must … have … PR … It's like 🚰 |
What is this conflicted PR I see? |
Don't know to what you refer to 😉. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 post_install
So is the issue here just about the luarocks build reporting it cannot find lua? |
Yeah. They seem to have broken detection of versioned executables, and even if you hack around that problem you hit a second problem that it thinks the header version doesn't align with the executable version because it thinks the executable version is null, for some reason 🙃. |
I think it's just that
since the patch is using |
Yeah, that was what I ran into last night poking around but ran out of time to test fixes before I had to die for the day. |
The |
ah ok … I'm just saying I'm not sure they actually broke anything. |
Something changed upstream, I presume, as this wasn't an issue till recently, but yeah it could just be amongst the detection tweaks they tripped a wire that had been sat there for us quietly for quite some time 🙈. |
The same |
I'll be back online properly in a few hours & can take a peek then, if you like. |
np |
If you want to leap over me by all means don't feel obligated to wait or anything, but yeah, happy to poke around later if you're not chomping at the bit to have at it. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
If you've got ideas on resolving this without leaning on |
I guess that serves as CI confirmation that I'm not just having some weird local issue at least. |
So. This PR should return green now. Finally. |
The amount of wrong directions the |
Formula/lua@5.1.rb
Outdated
@@ -40,14 +40,17 @@ class LuaAT51 < Formula | |||
end | |||
end | |||
|
|||
# Don't use the https://luarocks.org/releases/luarocks-x.y.z.tar.gz URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment looks wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, maybe they've fixed the redirect now. I'll check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still there:
Resource: luarocks
==> Downloading https://luarocks.org/releases/luarocks-2.4.4.tar.gz
==> Downloading from http://luarocks.github.io/luarocks/releases/luarocks-2.4.4.tar.gz
/usr/bin/curl --show-error --user-agent Homebrew/1.5.12-9-g49927ed (Macintosh; Intel Mac OS X 10.13.4) curl/7.54.0 --fail --location --remote-time --continue-at - --output /usr/local/var/homebrewcache/lua--luarocks-2.4.4.tar.gz.incomplete http://luarocks.github.io/luarocks/releases/luarocks-2.4.4.tar.gz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have you reported it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can go one better and file a PR, tbh. It's a one-liner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the problem? |
A variation of:
was indeed required as you suggested months ago, but there was also an issue where upstream apparently tweaked the header detection logic in a way that doesn't work with Homebrew's directory structure here, and that header silliness lead me down a ridiculous rabbit hole. In combination they work fine. |
It became obvious enough when I dropped into the debug shell for extended periods and just ran build after build after build with slight tweaks each time, but outside the debug shell a lot of the helpful messages are hidden by |
end | ||
|
||
def install | ||
# Use our CC/CFLAGS to compile. | ||
inreplace "src/Makefile" do |s| | ||
s.gsub! HOMEBREW_PREFIX, prefix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this actually work for a non-standard prefix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should do, theoretically. It is just a slight variation of what we've been doing in the lua
formula for ages, without anyone complaining about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the replacement not fail if the prefix isn't in the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why wouldn't the prefix be in the file? We're the ones adding that line in the inline patch. Maybe I'm misunderstanding you? I've slept for about 4 hours over the last day 🙃.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, OK. That's what I get for looking at this on my iPhone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+ $(CC) -dynamiclib -install_name HOMEBREW_PREFIX/lib/liblua.5.1.dylib \
+ -compatibility_version 5.1 -current_version 5.1.5 \
+ -o liblua.5.1.5.dylib $^
compared to:
+ $(CC) -dynamiclib -install_name @LUA_PREFIX@/lib/liblua.5.3.dylib \
+ -compatibility_version 5.3 -current_version 5.3.4 \
+ -o liblua.5.3.4.dylib $^
in the lua
formula. Same principle, just a slight divergence at some point for some reason. Possibly my fault given how much I've touched these formulae over the years.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not look right. If I am not mistaking, this should be
s.gsub! "HOMEBREW_PREFIX", prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think so? brew unpack --patch lua@5.1
shows the blob as I expected it to be:
$(LUA_A): $(CORE_O) $(LIB_O)
$(CC) -dynamiclib -install_name /usr/local/lib/liblua.5.1.dylib \
-compatibility_version 5.1 -current_version 5.1.5 \
-o liblua.5.1.5.dylib $^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get this for non-standard homebrew prefix.
brew upgrade lua@5.1
==> Upgrading 1 outdated package, with result:
lua@5.1 5.1.5_6
==> Upgrading lua@5.1
==> Downloading https://www.lua.org/ftp/lua-5.1.5.tar.gz
######################################################################## 100.0%
Error: inreplace failed
src/Makefile:
expected replacement of #<Pathname:/home/comp/chengxu/usr> with #<Pathname:/home/comp/chengxu/usr/Cellar/lua@5.1/5.1.5_6>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure I tried a string replacement in the default prefix and hit issues with it, which is why I didn't go down that route. Maybe we should just save ourselves some hassle & change it to @LUA_PREFIX@
to match the lua
formula.
Would ya look at that. A green CI build. Thought I'd never see the day here at times 🤦♂️. |
Formula/lua.rb
Outdated
s.gsub! lib.to_s, "#{HOMEBREW_PREFIX}/lib" | ||
s.gsub! bin.to_s, "#{HOMEBREW_PREFIX}/bin" | ||
s.gsub! libexec, opt_libexec | ||
s.gsub! include, "#{HOMEBREW_PREFIX}/include" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe HOMEBREW_PREFIX/"include"
Upstream merged the PR so the redirect is HTTPS to HTTPS now, which is fine, so I'll push new commits to use the official URL and after that I think we're alright here if you're happy.
|
Shipped! Thanks @DomT4 |
Thanks Joe. Nice to be able to finally delete this branch, heh. |
Or just hoard branches as I do! |
That would be the death of what’s left of my sanity. |
brew install --build-from-source <formula>
, where<formula>
is the name of the formula you're submitting?brew audit --strict <formula>
(after doingbrew install <formula>
)?I should've jumped on top of this a while ago, apologies.