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

Build V8 with libcxx instead of libstdc++, fixes #14230 #14271

Closed
wants to merge 2 commits into from
Closed

Build V8 with libcxx instead of libstdc++, fixes #14230 #14271

wants to merge 2 commits into from

Conversation

jeroen
Copy link
Contributor

@jeroen jeroen commented Jun 5, 2017

See #14230 for details.

V8 3.15 uses an old version of gyp to build, which defaults to the legacy libstdc++. This leads to compatibility problems with applications build with clang + libcxx (default on Mavericks and up).

By setting -mmacosx-version-min=10.9 clang will build against the libcxx instead.

@DomT4
Copy link
Member

DomT4 commented Jun 5, 2017

Be surprised if simply copying this doesn't work fine.

@jeroen
Copy link
Contributor Author

jeroen commented Jun 5, 2017

@DomT4 I guess that would work as well. Would you prefer that?

@DomT4
Copy link
Member

DomT4 commented Jun 5, 2017

I'm no longer a maintainer here so it's not my call ultimately, but that's tried & tested, and more brewish than hardcoding in one specific version of macOS that may not align with the system the user is running.

@jeroen
Copy link
Contributor Author

jeroen commented Jun 6, 2017

Right, either way works fine probably. Who is the new maintainer of this formula?

@DomT4
Copy link
Member

DomT4 commented Jun 7, 2017

Individual formulae do not have specific maintainers, in theory. @ilovezfs runs homebrew/core these days, choice is on him.

@jeroen
Copy link
Contributor Author

jeroen commented Jun 7, 2017

OK I'll wait for @ilovezfs to respond if he is OK with the current solution or not.

@jeroen
Copy link
Contributor Author

jeroen commented Jun 11, 2017

@ilovezfs ping, does this look OK to merge?

@ilovezfs
Copy link
Contributor

@jeroen do you mind testing if the lines @DomT4 linked to work as well?

@stale stale bot added the stale No recent activity label Jul 2, 2017
@stale
Copy link

stale bot commented Jul 2, 2017

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 removed the stale No recent activity label Jul 8, 2017
@jeroen
Copy link
Contributor Author

jeroen commented Jul 8, 2017

@ilovezfs sorry for the delay. I have now changed the commit to do what you suggested and use the same lines from the v8 formula to fix the same problem.

@ilovezfs
Copy link
Contributor

ilovezfs commented Jul 8, 2017

@jeroen no worries. And I assume you've tested it works as expected?

@jeroen
Copy link
Contributor Author

jeroen commented Jul 8, 2017

Yes I build locally and it seems to fix the problem.

@DomT4
Copy link
Member

DomT4 commented Jul 8, 2017

Theoretically you'll want the needs :cxx11 as well, for the folks who have ancient compilers installed.

@jeroen
Copy link
Contributor Author

jeroen commented Jul 8, 2017

I think the library builds fine without CXX11, it just has to use libcxx...

@jeroen
Copy link
Contributor Author

jeroen commented Jul 15, 2017

I think this is good to go?

@jeroen
Copy link
Contributor Author

jeroen commented Jul 19, 2017

Ping? Anything else we need to change?

@ilovezfs
Copy link
Contributor

@BrewTestBot test this please

@jeroen
Copy link
Contributor Author

jeroen commented Jul 19, 2017

@ilovezfs ilovezfs closed this in ddfe5c7 Jul 19, 2017
@ilovezfs
Copy link
Contributor

Thanks @jeroen! Shipped :)

@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.

3 participants