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

mpv.rb: always build with lua@5.1 #51335

Merged
merged 1 commit into from Apr 6, 2020
Merged

Conversation

@jnozsc
Copy link
Contributor

jnozsc commented Mar 8, 2020

  • 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?
  • 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 <formula>)?

Context:

  • Since mpv-player/mpv@d852ad2 , mpv starts to prefer luajit than lua@5.1
  • mpv's own testing workflow is using luajit link
  • The MacOS.version < :catalina part is added, since luajit 2.0 stable branch is broken on Catalina.
    I know @fxcoudert and other people have already tried to fix it, as #46928 and LuaJIT/LuaJIT#521 , but when I tried to make luajit works with Catalina, I ran into this issue mpv-player/mpv#7512 . And mpv developers confirm luajit 2.0 is sort of broken on Catalina.
    Even luajit's developer recommends to use 2.1 branch, which is beta, can not add to homebrew due to the policy.
    I have already ask luajit whether it is possible to release a new stable version for Catalina. LuaJIT/LuaJIT#563. but I am not sure when it will or will not happen
  • if we could do something like depends_on "luajit": HEAD , that will fix the Catalina compatible issue, but I remember that depends_on version is not allowed, right? :(

updated:
One month later, no update from luajit, I hope they could release a new version in the future. For now, the better solution is always force build mpv with lua@5.1, no matter luajit is installed or not

@jnozsc jnozsc force-pushed the jnozsc:mpv_depends_on_luajit branch from fcf3906 to 3188eea Mar 9, 2020
@SMillerDev

This comment has been minimized.

Copy link
Member

SMillerDev commented Mar 9, 2020

If it's only HEAD that's affected and it's not currently broken I'd wait until the next luajit release.

@jnozsc

This comment has been minimized.

Copy link
Contributor Author

jnozsc commented Mar 9, 2020

well, I hope but doubt that luajit will get a stable release very soon. I have created the ticket for them, let's see.

@stale

This comment has been minimized.

Copy link

stale bot commented Apr 1, 2020

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

@stale stale bot added the stale label Apr 1, 2020
@jnozsc

This comment has been minimized.

Copy link
Contributor Author

jnozsc commented Apr 1, 2020

This is not stale, we are waiting for luajit to release a new version.
Unfortunately, there is no response from luajit yet.

@SMillerDev @chenrui333 could we consider to merge this PR as a compromise solution?

@stale stale bot removed the stale label Apr 1, 2020
@SMillerDev

This comment has been minimized.

Copy link
Member

SMillerDev commented Apr 1, 2020

No, and it is stale since nothing is happening with it. If it gets closed you can resubmit when LuaJIT responds

@jnozsc

This comment has been minimized.

Copy link
Contributor Author

jnozsc commented Apr 1, 2020

but this is broken on HEAD

@SMillerDev

This comment has been minimized.

Copy link
Member

SMillerDev commented Apr 1, 2020

And head builds are not generally supported because they can break so easily.

@jnozsc jnozsc force-pushed the jnozsc:mpv_depends_on_luajit branch from 3a643d4 to d5fa7b7 Apr 2, 2020
@jnozsc

This comment has been minimized.

Copy link
Contributor Author

jnozsc commented Apr 2, 2020

@SMillerDev
One mpv developer recommends to use a build flag to force the lua version . so I update my PR to always force pick up lua@5.1 as lua runtime when build mpv, no matter luajit is installed or not.

@jnozsc jnozsc changed the title mpv.rb: change lua compiler to luajit mpv.rb: always build with lua@5.1 Apr 2, 2020
@jnozsc jnozsc force-pushed the jnozsc:mpv_depends_on_luajit branch from d5fa7b7 to 0d6db6f Apr 2, 2020
@SMillerDev

This comment has been minimized.

Copy link
Member

SMillerDev commented Apr 2, 2020

Please don't restrict I to Catalina but always force this.

@jnozsc

This comment has been minimized.

Copy link
Contributor Author

jnozsc commented Apr 2, 2020

ok

@jnozsc jnozsc force-pushed the jnozsc:mpv_depends_on_luajit branch from 0d6db6f to a236b7e Apr 2, 2020
@jnozsc

This comment has been minimized.

Copy link
Contributor Author

jnozsc commented Apr 2, 2020

it seems macOS 10.14 build test may have some issue, even without change on mpv.rb 0d6db6f not sure whether it is a CI issue or not

@Bo98

This comment has been minimized.

Copy link
Member

Bo98 commented Apr 2, 2020

I made a change to brew earlier that might help there. It looks like the GitHub Actions nodes had not yet received that change at the time of build started, but I reckon Jenkins nodes will do.

@SMillerDev

This comment has been minimized.

Copy link
Member

SMillerDev commented Apr 3, 2020

@BrewTestBot test this please

@jnozsc jnozsc force-pushed the jnozsc:mpv_depends_on_luajit branch from dca4b18 to 8a75fcb Apr 4, 2020
@Bo98

This comment has been minimized.

Copy link
Member

Bo98 commented Apr 4, 2020

I suspect this might be an issue with the CLT - particularly since it is interfacing with Swift which generally is better supported by Xcode. Try depends_on :xcode => :build.

@jnozsc jnozsc force-pushed the jnozsc:mpv_depends_on_luajit branch from 8a75fcb to f40262d Apr 4, 2020
@jnozsc

This comment has been minimized.

Copy link
Contributor Author

jnozsc commented Apr 6, 2020

is this ready to merge?

@Bo98

This comment has been minimized.

Copy link
Member

Bo98 commented Apr 6, 2020

Yeah, thanks!

@Bo98 Bo98 merged commit ca173c8 into Homebrew:master Apr 6, 2020
4 checks passed
4 checks passed
tests (10.15)
Details
tests (10.14)
Details
tests (10.13)
Details
continuous-integration/jenkins/ghprb Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.