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

[email protected]: undeprecate #105069

Closed
wants to merge 1 commit into from
Closed

Conversation

carlocab
Copy link
Member

@carlocab carlocab commented Jul 4, 2022

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • 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?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

This formula still has dependents, so, per our policy, this shouldn't be
deprecated. Leaving this deprecated also leads to users complaining.
See, for example, #105067.

I haven't marked this as closing #105067 since there may still be other things
we can do about libquvi.

This formula still has dependents, so, per our policy, this shouldn't be
deprecated. Leaving this deprecated also leads to users complaining.
See, for example, Homebrew#105067.
@carlocab carlocab added the CI-syntax-only Change only affects brew syntax, not the install. Only run syntax CI. label Jul 4, 2022
@BrewTestBot BrewTestBot added the legacy Relates to a versioned @ formula label Jul 4, 2022
@cho-m
Copy link
Member

cho-m commented Jul 4, 2022

Dependents:

stone-soup is probably only blocker for deprecation.

@Bo98
Copy link
Member

Bo98 commented Jul 4, 2022

  • stone-soup -- Actively maintained with Lua 5.1 dependency.

This has LuaJIT support (USE_LUAJIT) so we can use that.

This was referenced Jul 5, 2022
@cho-m
Copy link
Member

cho-m commented Jul 5, 2022

This has LuaJIT support (USE_LUAJIT) so we can use that.

I guess this brings to question how we want to handle LuaJIT as a dependency now that we are updating luajit #104765.

We migrated all dependents to luajit-openresty, but we might as well migrate everything back to official luajit if we plan to keep it up-to-date. There doesn't seem to be any advantage of continuing to use luajit-openresty (i.e. main code is same and OpenResty extensions aren't used) and most people that need LuaJIT would probably first go to luajit formula.


For others, I opened PRs to consider deprecation as the projects don't seem to be maintained.

@danielnachun
Copy link
Member

We migrated all dependents to luajit-openresty, but we might as well migrate everything back to official luajit if we plan to keep it up-to-date. There doesn't seem to be any advantage of continuing to use luajit-openresty (i.e. main code is same and OpenResty extensions aren't used) and most people that need LuaJIT would probably first go to luajit formula.

I still feel more comfortable relying on luajit-openresty to satisfy dependencies in formulae. We only have to update its dependents when there is a tagged release and it makes it easier for us to track API compatibility. To me luajit is more or less a head-only formula for which we will provide build artifacts on a regular basis, but I don't see it as a suitable dependency for other packages. However I am curious what other maintainers think about this as there are a range of reasonable opinions here.

@carlocab
Copy link
Member Author

carlocab commented Jul 5, 2022

The LuaJIT C API is stable. You can skim the Git log of the installed headers to see. (See below in case you don't trust the eyeball test.)

In fact, the current HEAD (LuaJIT/LuaJIT@4c2441c) is ABI compatible with the v2.1.0-beta3 tag, which dates back to 2017:

❯ abi-compliance-checker --gcc-path=gcc-11 -lib luajit-5.1 -old luajit.tag.xml -new luajit.head.xml
Preparing, please wait ...
Using GCC 11 (x86_64-apple-darwin21, target: x86_64)
WARNING: May not work properly with GCC 4.8.[0-2], 6.* and higher due to bug #78040 in GCC. Please try other GCC versions with the help of --gcc-path=PATH option or create ABI dumps by ABI Dumper tool instead to avoid using GCC. Test selected GCC version first by -test option.
Checking header(s) 2.1.0-beta3 ...
Checking header(s) 2.1.0-beta3-HEAD ...
Comparing ABIs ...
Comparing APIs ...
Creating compatibility report ...
Binary compatibility: 100%
Source compatibility: 100%
Total binary compatibility problems: 0, warnings: 1
Total source compatibility problems: 0, warnings: 2
Report: compat_reports/luajit-5.1/2.1.0-beta3_to_2.1.0-beta3-HEAD/compat_report.html

Here's the compat report for reference: compat_report.html.zip (zipped because GitHub doesn't like raw html)

The source compatibility check also indicates that HEAD hasn't broken the C API from the v2.1.0-beta3 tag.

The Lua bits (API + bytecode) are a bit harder for me to gauge, since I'm not so familiar with Lua. I've heard mixed (anecdotal) reports, but I'm going to guess that keeping the Lua side stable is a lot easier than not breaking the C API/ABI (perhaps accidentally), so I think things are fine on that front too.

In any case, even if the above weren't true: I think API stability is useful only insofar as we know that clients are targeting the API version that we're using. I suspect most projects are actually targeting some version of upstream LuaJIT rather than the OpenResty fork, so the stability of the API might not be of that much value to us here.

Our users also seem to have a clear preference between the two versions if we look at installs-on-request. Upstream LuaJIT has 3-6 times as many installs-on-request as the OpenResty fork, depending on the time period. (Interestingly, LuaJIT HEAD also has much fewer build failures than non-HEAD installs.)

Analytics

==> install-on-request (30 days)

Index Name (with options) Count Percent
1 luajit --HEAD 741 64.10%
2 luajit 415 35.90%
Total 1,156 100.00%

==> install-on-request (90 days)

Index Name (with options) Count Percent
1 luajit --HEAD 2,282 64.92%
2 luajit 1,232 35.05%
3 luajit --devel --with-gc64 1 0.03%
Total 3,515 100.00%

==> install-on-request (365 days)

Index Name (with options) Count Percent
1 luajit --HEAD 9,744 64.20%
2 luajit 5,433 35.80%
3 luajit --devel --with-gc64 1 0.01%
Total 15,178 100.00%

==> build-error (30 days)

Index Name (with options) Count Percent
1 luajit 137 87.26%
2 luajit --HEAD 20 12.74%
Total 157 100.00%

==> install-on-request (30 days)

Index Name (with options) Count Percent
1 luajit-openresty 188 98.43%
2 luajit-openresty --HEAD 3 1.57%
Total 191 100.00%

==> install-on-request (90 days)

Index Name (with options) Count Percent
1 luajit-openresty 1,329 99.40%
2 luajit-openresty --HEAD 8 0.60%
Total 1,337 100.00%

==> install-on-request (365 days)

Index Name (with options) Count Percent
1 luajit-openresty 4,909 99.05%
2 luajit-openresty --HEAD 47 0.95%
Total 4,956 100.00%

==> build-error (30 days)

Index Name (with options) Count Percent
1 luajit-openresty 0 0.00%

@cho-m
Copy link
Member

cho-m commented Jul 5, 2022

First, just noting that stone-soup is using some older/deprecated Lua functions (like luaL_reg rather than luaL_Reg) which was only supported until Lua 5.1 & LuaJIT 2.0.5, but isn't supported in current LuaJIT 2.1.0.


In case of homebrew LuaJIT analytics, I think it is a bit difficult to compare due to things like:

  • I have a feeling that users looking for LuaJIT would usually seek the former based on naming or not knowing what OpenResty variant is.
  • There are probably users who tried to build stable on Apple Silicon and account for part of non-HEAD failures

For choosing between luajit and luajit-openresty, I am guessing it probably won't make a large difference in terms of stability.

OpenResty says they synchronize commits. I don't know if they do any additional testing that would make the particular tag they set any "better" than an arbitrary commit we select from LuaJIT head.

Looking at some other repositories based on neovim dependency tree, there are examples of both (basing on either hard dependency or preferred from virtual package):

  • OpenResty fork is used/preferred by: Alpine, FreeBSD, Spack
  • LuaJIT Head is used/preferred by: Arch, Gentoo, MacPorts, Fedora

@carlocab
Copy link
Member Author

carlocab commented Jul 5, 2022

First, just noting that stone-soup is using some older/deprecated Lua functions (like luaL_reg rather than luaL_Reg) which was only supported until Lua 5.1 & LuaJIT 2.0.5, but isn't supported in current LuaJIT 2.1.0.

That's a show-stopper for using LuaJIT then. Unless we want to keep it at 2.0.5, but I don't think that's a good option.

In case of homebrew LuaJIT analytics, I think it is a bit difficult to compare due to things like:

  • I have a feeling that users looking for LuaJIT would usually seek the former based on naming or not knowing what OpenResty variant is.
  • There are probably users who tried to build stable on Apple Silicon and account for part of non-HEAD failures

This is true, but I think that if there's one version that users are going to look for first and there are no other distinguishing factors otherwise, then we should probably use that as our main dependency already (since more users are more likely to already have it installed).

For choosing between luajit and luajit-openresty, I am guessing it probably won't make a large difference in terms of stability.

OpenResty says they synchronize commits. I don't know if they do any additional testing that would make the particular tag they set any "better" than an arbitrary commit we select from LuaJIT head.

Agree with both points.

@Bo98
Copy link
Member

Bo98 commented Jul 5, 2022

First, just noting that stone-soup is using some older/deprecated Lua functions (like luaL_reg rather than luaL_Reg) which was only supported until Lua 5.1 & LuaJIT 2.0.5, but isn't supported in current LuaJIT 2.1.0.

This sounds like something that could be upstreamed given luaL_reg was deprecated in Lua 5.0 and changing to the newer name does not break compatibility with anything.

@carlocab
Copy link
Member Author

carlocab commented Jul 5, 2022

Actually, we could probably get away with setting -DluaL_reg=luaL_Reg in CPPFLAGS or something and it would compile fine. Have upstream really not handled this yet? It seems like something they would've already.

@cho-m
Copy link
Member

cho-m commented Jul 6, 2022

Actually, we could probably get away with setting -DluaL_reg=luaL_Reg in CPPFLAGS or something and it would compile fine. Have upstream really not handled this yet? It seems like something they would've already.

Since Lua 5.1 still supports deprecated functions, I am guessing upstream doesn't see it as a priority especially as they have their own repos for bundled copies https://github.com/crawl/crawl-lua (5.1.5), https://github.com/crawl/crawl-luajit (2.0.3).

Makefile doesn't seem to use CPPFLAGS. On Linux build, it looks like the CFLAGS value is overridden, but I don't know if that might impact Makefile logic. There is an EXTERNAL_FLAGS that may be usable.

Anyway, manually adding a -DluaL_reg=luaL_Reg workaround, upstreaming a fix, or building a bundled Lua 5.1 are all options for dropping [email protected] dependency. Now that other dependents are deprecated, stone-soup is the only remaining non-deprecated usage of [email protected].

@cho-m
Copy link
Member

cho-m commented Jul 6, 2022

Another attempt on stone-soup with EXTERNAL_DEFINES=-DluaL_reg=luaL_Reg built with current luajit-openresty but running it caused a stack overflow.

/opt/homebrew/Cellar/stone-soup/0.28.0/data/dat/des/builder/layout.des:1654: stack overflow

The easiest way to deprecate [email protected] properly is to just allow stone-soup to build its own Lua 5.1 by resourcing the commit tarball matching submodule in https://github.com/crawl/crawl/tree/0.28.0/crawl-ref/source/contrib, e.g.

@@ -38,7 +38,13 @@ class StoneSoup < Formula
     sha256 "68fb519c14306fec9720a2a5b45bc9f0c8d1b9c72adf45c37baedfcd949c35a2"
   end

+  resource "lua" do
+    url "https://github.com/crawl/crawl-lua/archive/c8f7b1ea86188970118b52df9c5b231b454bca0e.tar.gz"
+    sha256 "f9ba4efe02a7ba3b8d21540b6ffed01d2f521abb019163ed7f5dd6499da931ae"
+  end
+
   def install
+    (buildpath/"crawl-ref/source/contrib/lua").install resource("lua")
     ENV.cxx11
     ENV.prepend_path "PATH", Formula["[email protected]"].opt_libexec/"bin"
     xy = Language::Python.major_minor_version "python3"
@@ -57,7 +63,7 @@ class StoneSoup < Formula
         BUILD_SQLITE=
         BUILD_FREETYPE=
         BUILD_LIBPNG=
-        BUILD_LUA=
+        BUILD_LUA=y
         BUILD_SDL2=
         BUILD_SDL2MIXER=
         BUILD_SDL2IMAGE=

@carlocab
Copy link
Member Author

carlocab commented Jul 6, 2022

Not that keen on a resourced lua5.1. If we're building it already anyway, may as well let users benefit from it in other ways.

It's not as if [email protected]and dependents have caused us any maintenance problems (e.g. build failures) that need dealing with.

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Jul 28, 2022
@cho-m
Copy link
Member

cho-m commented Jul 30, 2022

Close as merged in #106848

@cho-m cho-m closed this Jul 30, 2022
@cho-m cho-m added the superseded PR was replaced by another PR label Jul 30, 2022
@carlocab carlocab deleted the lua5.1-dep branch July 30, 2022 19:35
@github-actions github-actions bot added the outdated PR was locked due to age label Aug 30, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-syntax-only Change only affects brew syntax, not the install. Only run syntax CI. legacy Relates to a versioned @ formula outdated PR was locked due to age stale No recent activity superseded PR was replaced by another PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants