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

luajit: add option to build in gc64 mode #20036

Merged
merged 1 commit into from
Nov 1, 2017

Conversation

rcombs
Copy link
Contributor

@rcombs rcombs commented Oct 30, 2017

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

brew audit gives me * devel version should not decrease (from 2.1 to 2.1.0-beta3); not sure what's going on there (I didn't touch anything relevant).

@ilovezfs
Copy link
Contributor

@DomT4 any idea why this isn't upstream default?

@DomT4
Copy link
Member

DomT4 commented Oct 30, 2017

As far as I'm aware it's an unfinished feature on the 2.1 branch & doesn't exist at all on the stable 2.0.x branch.

@rcombs
Copy link
Contributor Author

rcombs commented Oct 31, 2017

This being closed makes it seem like it's effectively finished: LuaJIT/LuaJIT#25

@DomT4
Copy link
Member

DomT4 commented Oct 31, 2017

The mailing list is still pretty active in terms of bug reports with this feature, so I would presume it's not "done". Nonetheless if this is added as an option it should be limited to devel for now since it doesn't exist in stable at all.

@rcombs
Copy link
Contributor Author

rcombs commented Oct 31, 2017

Ah. I have no idea how to do that.

@ilovezfs
Copy link
Contributor

devel do and head do

@@ -16,6 +14,12 @@ class Luajit < Formula
devel do
url "https://luajit.org/download/LuaJIT-2.1.0-beta3.tar.gz"
sha256 "1ad2e34b111c802f9d0cdf019e986909123237a28c746b21295b63c9e785d9c3"
option "with-gc64", "Build with 64-bit support"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New line between sha256 and the first option here & inside head do.


cflags = ""
cflags += "-DLUAJIT_ENABLE_LUA52COMPAT " if build.with? "52compat"
cflags += "-DLUAJIT_ENABLE_GC64" if build.with? "gc64"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically this should be something like:

cflags << "-DLUAJIT_ENABLE_GC64" if !build.stable && build.with?("gc64")

cflags += "-DLUAJIT_ENABLE_LUA52COMPAT " if build.with? "52compat"
cflags += "-DLUAJIT_ENABLE_GC64" if build.with? "gc64"

args << "XCFLAGS=#{cflags}" if cflags != ""
Copy link
Member

@DomT4 DomT4 Oct 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I love this syntax choice but I'll leave that down to ILZ.

@@ -35,7 +39,12 @@ def install
ENV.O2 # Respect the developer's choice.

args = %W[PREFIX=#{prefix}]
args << "XCFLAGS=-DLUAJIT_ENABLE_LUA52COMPAT" if build.with? "52compat"

cflags = ""
Copy link
Member

@DomT4 DomT4 Oct 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably %w[] rather than "".

@ilovezfs ilovezfs merged commit 06cdb63 into Homebrew:master Nov 1, 2017
@ilovezfs
Copy link
Contributor

ilovezfs commented Nov 1, 2017

Thanks @rcombs!

robohack pushed a commit to robohack/homebrew-core that referenced this pull request Nov 3, 2017
@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants