lua 5.3.4, rename from lua@5.3 and lua@5.1: add unversioned pkg-config file #21300

ilovezfs · 2017-12-03T19:19:46Z

Have you followed the guidelines for contributing?
Have you checked that there aren't other open pull requests for the same formula update/change?
Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

DomT4 · 2017-12-03T19:33:47Z

Formula/lua.rb

  resource "luarocks" do
-    url "https://luarocks.github.io/luarocks/releases/luarocks-2.4.3.tar.gz"
-    sha256 "4d414d32fed5bb121c72d3ff1280b7f2dc9027a9bc012e41dfbffd5b519b362e"
+    url "https://keplerproject.github.io/luarocks/releases/luarocks-2.3.0.tar.gz"


This wouldn't need downgrading if you keep the executable of the lua formula as lua and version the others.

DomT4 · 2017-12-03T19:35:01Z

Will look this over when I'm not cooking, but I'd be a little surprised if this doesn't break a small horde of formulae. Ecosystem support for 5.3 is somehow lagging behind a touch, despite 5.3 not being an especially new major release at this point.

ilovezfs · 2017-12-03T19:37:23Z

@DomT4 yeah, @fxcoudert asked why we hadn't done this yet so I figured I'd see where we stand.

ilovezfs · 2017-12-03T19:39:31Z

One question is how viable depends_on "lua@5.2" is as a temporary (ahem) strategy for the ones that 💥

DomT4 · 2017-12-03T21:32:10Z

One question is how viable depends_on "lua@5.2" is as a temporary (ahem) strategy for the ones that

A pretty long temporary strategy I'd bet 🙈. To be honest one of my big reluctances on this is screwing Travis up for people who use lua given how slowly the ecosystem has been moving 😓.

You might get a "truer" CI result here with lua reclaiming the main unversioned executables/etc.

ilovezfs · 2017-12-03T21:44:26Z

You might get a "truer" CI result here with lua reclaiming the main unversioned executables/etc.

Yeah. This is in the brainless stage right now.

ilovezfs · 2017-12-03T21:45:16Z

You might get a "truer" CI result here with lua reclaiming the main unversioned executables/etc.

maybe should go keg_only on the versioned ones …

DomT4 · 2017-12-03T21:47:54Z

It has been discussed before. I pretty consistently argue against it, at least whilst things like python and python3 are fine existing alongside each other, but I know that has been the cause of plenty of arguments over the years as well. I'm very unconvinced about the UX of keg_only on things that involve executables, but I accept I may have only tentatively won that argument over the years on lua because I've been regularly the only person doing major legwork on them 😄.

ilovezfs · 2017-12-03T21:49:01Z

I'm very unconvinced about the UX of keg_only on things that involve executables

You're aware brew link -f is now persistent across upgrades, right?

DomT4 · 2017-12-03T21:50:47Z

Yeah, but then you end up with only one Lua linked at once still, and I'm not the heaviest user of lua in the world but regularly find myself reaching for different versions locally, certainly more often than I need to with ruby or node.

ilovezfs · 2017-12-03T22:23:57Z

@DomT4 PR refreshed.

ilovezfs · 2017-12-04T09:36:51Z

@DomT4 so one option available here is to build with compatibility -DLUA_COMPAT_MODULE which seems to make it possible to use lua 5.3 for the ones that otherwise need lua 5.2. But since that's not the upstream default, it may be a religious violation.

ilovezfs · 2017-12-04T11:43:42Z

Ditto LUA_COMPAT_5_1 which then also defines LUA_COMPAT_MODULE amongst other things.

MacPorts is using this: https://github.com/macports/macports-ports/blob/master/lang/lua/files/patch-src-Makefile.diff#L8

So that explains why they're able to use 5.3 with things that would otherwise be 5.3 incompatible.

DomT4 · 2017-12-04T15:12:19Z

Apologies, came back from the gym last night & was exhausted, so didn't reappear as planned.

MacPorts is using this: https://github.com/macports/macports-ports/blob/master/lang/lua/files/patch-src-Makefile.diff#L8

Yeah. I don't have a huge problem with this other than having concerns about facilitating an ecosystem that is already slooooooooooooooooooow to move on feeling like there's even less importance to updating their codebases to support the latest & greatest major version of Lua, but if MacPorts is already doing it I guess we might as well concede that ship has sailed and see if we can use it as an avenue to update.

DomT4 · 2017-12-04T15:15:18Z

Formula/corsixth.rb


    resources.each do |r|
      r.stage do
        system "luarocks", "build", r.name, "--tree=#{luapath}"
      end
    end

-    # Ensures it uses the intended Lua (5.2) rather than 5.1/5.3 or


Was this double checked locally on a system that has multiple versions of Lua installed?

(CMake being CMake has the tendency to do whatever the hell it likes, even when passed direct variables in some cases, because life is fun 🙃)

DomT4 · 2017-12-04T15:18:43Z

Formula/lua.rb


    # This resource must be handled after the main install, since there's a lua dep.
    # Keeping it in install rather than postinstall means we can bottle.
    if build.with? "luarocks"
      resource("luarocks").stage do
        ENV.prepend_path "PATH", bin
+        lua_prefix = prefix


I'm honestly not sure why I added this in the first place given it was only used once, as far as I can see 😓.

DomT4 · 2017-12-04T15:19:10Z

Formula/lua.rb

-        (pkgshare/"5.2/luarocks").install_symlink Dir["#{libexec}/share/lua/5.2/luarocks/*"]
-        bin.install_symlink libexec/"bin/luarocks-5.2"
-        bin.install_symlink libexec/"bin/luarocks-admin-5.2"
+        (share/"lua/5.3/luarocks").install_symlink Dir["#{libexec}/share/lua/5.3/luarocks/*"]


This can go back to being pkgshare.

DomT4 · 2017-12-04T15:19:54Z

Formula/lua.rb

    Requires:
-    Libs: -L${libdir} -llua -lm
+    Libs: -L${libdir} -llua.5.3 -lm


This is probably fine remaining as -llua -lm here.

DomT4 · 2017-12-04T15:20:22Z

Formula/lua.rb

    Cflags: -I${includedir}
    EOS
  end

-  def caveats; <<~EOS


This caveat is still relevant but the same thing happens with python and python3 and we don't warn about it so I don't have strong feelings 🤷‍♂️.

DomT4 · 2017-12-04T15:20:58Z

Formula/lua.rb

  test do
-    system "#{bin}/lua", "-e", "print ('Ducks are cool')"
+    system "#{bin}/lua-5.3", "-e", "print ('Ducks are cool')"


IMO, keep this as lua since that's what the actual executable is called. We just install the symlink for compatibility mostly.

DomT4 · 2017-12-04T15:43:36Z

Formula/lua@5.2.rb

+        system "make", "build"
+        system "make", "install"
+
+        (pkgshare/"5.2/luarocks").install_symlink Dir["#{libexec}/share/lua/5.2/luarocks/*"]


This one will need to however be changed back to (share/"lua/5.2/luarocks")

does anything actually break?

From memory, yes, but I can look into it properly at some point later probably.

DomT4 · 2017-12-04T15:44:13Z

Formula/lua@5.2.rb

+    EOS
+  end
+
+  def caveats; <<~EOS


Should probably decide what to do with these caveats and then harmomise it across all the Lua versions.

ilovezfs · 2017-12-04T15:48:19Z

Yeah. I don't have a huge problem with this other than having concerns about facilitating an ecosystem that is already slooooooooooooooooooow to move on feeling like there's even less importance to updating their codebases to support the latest & greatest major version of Lua, but if MacPorts is already doing it I guess we might as well concede that ship has sailed and see if we can use it as an avenue to update.

I think MacPorts is the odd one out in terms of what other package managers have done here.

ilovezfs · 2017-12-04T16:12:29Z

One reason in favor of building with LUA_COMPAT_5_1 is that this versioned lua@5.2 formula, because it's not keg_only, is hardly a drop-in replacement for the original unversioned lua 5.2. But that points more toward making it keg_only than it does toward shipping a non-5.3 5.3.

DomT4 · 2017-12-05T04:36:01Z

Will look this over again/etc after class today. Things are a bit hectic this week as there's an alarmingly large exam on Friday 🙈.

ilovezfs · 2017-12-05T04:58:38Z

I'm thinking maybe no 5.2 formula since presumably 5.3's 5.2 compat should cover that case regardless of whether we also build it with 5.1 compat.

DomT4 · 2017-12-05T05:12:08Z

Agreed. If you're leaning towards having 5.2 compatibility on, at least for now, it would make sense to not have a lua@5.2 formula.

DomT4 · 2017-12-05T05:12:56Z

Off-topic but there's a new icu4c release 😰. Presumably will want to wait for Node & whatever else again.

and use upstream's vendored lua

ilovezfs · 2017-12-06T08:06:35Z

@DomT4 PR refreshed.

ilovezfs · 2017-12-06T08:11:01Z

@BrewTestBot test this please

ilovezfs · 2017-12-06T08:22:09Z

Yeah, that doesn't sound unreasonable to me. I'm very strongly coming around to brew update just printing a warning and unlinking any deleted formula automatically, but 🙈.

Yes, given they can't always be expressed as renames that makes sense to me.

ilovezfs · 2017-12-06T11:36:03Z

==> brew audit lua --online
==> FAILED
Error: 1 problem in 1 formula
lua:
  * revisions should only increment by 1

@BrewAudit you're just going to have to learn to live with it.

DomT4 · 2017-12-06T18:48:49Z

Thanks for pushing this through @ilovezfs. Appreciate the help on the lua situation ❤️.

DomT4 reviewed Dec 3, 2017

View reviewed changes

ilovezfs force-pushed the lua-5.3.4 branch from c947de6 to b3f7539 Compare December 3, 2017 22:27

ilovezfs force-pushed the lua-5.3.4 branch from 642e371 to 65ca709 Compare December 4, 2017 12:23

DomT4 reviewed Dec 4, 2017

View reviewed changes

ilovezfs added 17 commits December 6, 2017 00:04

libswiften: depend on lua@5.1 instead of lua

8d15b90

lsyncd: revision for lua

a636310

lutok: revision for lua

a1496ab

monotone: revision for lua

d6ccbd2

mpv: revision for lua

4c80c07

onscripter: revision for lua

fe4c157

osm2pgsql: revision for lua

3764d07

osrm-backend: revision for lua

1fb8bcf

pdns: revision for lua

6a85681

pdnsrec: revision for lua

1eea86b

pioneer: remove lua dependency

981b14c

and use upstream's vendored lua

radare2: revision for lua

04291fa

rpm: depend on lua@5.1 instead of lua

4f4f09a

sile: revision for lua

8d2bcc2

tracebox: revision for lua

5f71f2a

wireshark: revision for lua

6d830e9

xmoto: depend on lua@5.1 instead of lua

f52bbec

ilovezfs force-pushed the lua-5.3.4 branch from c6d8953 to f52bbec Compare December 6, 2017 08:05

ilovezfs changed the title ~~lua 5.3.4, lua@5.3: delete, lua@5.1: add unversioned pkg-config file~~ lua 5.3.4, rename from lua@5.3 and lua@5.1: add unversioned pkg-config file Dec 6, 2017

ilovezfs added audit failure CI fails while auditing the software formula rename PR or issue relates to changing a formula's name lua Lua use is a significant feature of the PR or issue labels Dec 6, 2017

ilovezfs closed this in 85ce43b Dec 6, 2017

ilovezfs mentioned this pull request Dec 9, 2017

lua 5.3 or luajit support needed mpv-player/mpv#5205

Closed

Homebrew locked and limited conversation to collaborators May 4, 2018

lua 5.3.4, rename from lua@5.3 and lua@5.1: add unversioned pkg-config file #21300

lua 5.3.4, rename from lua@5.3 and lua@5.1: add unversioned pkg-config file #21300

Conversation

ilovezfs commented Dec 3, 2017 • edited Loading

Choose a reason for hiding this comment

DomT4 commented Dec 3, 2017

ilovezfs commented Dec 3, 2017

ilovezfs commented Dec 3, 2017

DomT4 commented Dec 3, 2017

ilovezfs commented Dec 3, 2017

ilovezfs commented Dec 3, 2017

DomT4 commented Dec 3, 2017

ilovezfs commented Dec 3, 2017

DomT4 commented Dec 3, 2017

ilovezfs commented Dec 3, 2017

ilovezfs commented Dec 4, 2017

ilovezfs commented Dec 4, 2017

DomT4 commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilovezfs commented Dec 4, 2017

ilovezfs commented Dec 4, 2017

DomT4 commented Dec 5, 2017

ilovezfs commented Dec 5, 2017

DomT4 commented Dec 5, 2017

DomT4 commented Dec 5, 2017

ilovezfs commented Dec 6, 2017

ilovezfs commented Dec 6, 2017

ilovezfs commented Dec 6, 2017

ilovezfs commented Dec 6, 2017

DomT4 commented Dec 6, 2017

ilovezfs commented Dec 3, 2017 •

edited

Loading