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

stk-4.5.0 enable C++11 #8828

Closed
wants to merge 1 commit into from
Closed

stk-4.5.0 enable C++11 #8828

wants to merge 1 commit into from

Conversation

tresf
Copy link
Contributor

@tresf tresf commented Jan 14, 2017

Allows pre-10.9 clangs to build stk in C++11 mode

To use before merged (as long as this PR is still open):

brew reinstall https://raw.githubusercontent.com/tresf/homebrew-core/patch-2/Formula/stk.rb

If this PR is merged, you should update your homebrew.

Fixes CXX compile error:

Linking CXX shared module
Undefined symbols for architecture x86_64:

@woodruffw
Copy link
Member

Does this have any appreciable difference on post-10.9 builds? Just wondering if we should guard this with a version check if it changes the library in any serious ways.

@tresf
Copy link
Contributor Author

tresf commented Jan 14, 2017

Just wondering if we should guard this with a version check if it changes the library in any serious ways.

To my understanding, only 10.8 and lower would benefit (or perhaps more accurately Xcode 5.1.1 and older.) I checked other projects with this same flag, this is all I could find: https://github.com/Homebrew/homebrew-core/blob/master/Formula/llvm.rb#L169

Not all formulas with this flag have this check enabled.

@tresf
Copy link
Contributor Author

tresf commented Jan 14, 2017

More about this problem is available here: Homebrew/legacy-homebrew#30512

Note, 10.9 is so old now that it is my understanding that all libraries are built from source (--build-from-source) would be redundant, but this formula by default seems to link against libstdc++ which won't work with any modern project, it will cause Undefined symbols for architecture x86_64:.

The project that is being built has the following brew dependencies:

pkgconfig fftw libogg libvorbis libsndfile libsamplerate jack sdl stk 
fluid-synth portaudio node fltk qt@5.5

Stk is currently the only package on 10.9 which causes compile linking errors.

@tresf
Copy link
Contributor Author

tresf commented Jan 14, 2017

So in the case of this project, ENV.libcxx is less of a C++11 flag and more of a linking flag. When the default build links against libstdc++, it breaks any projects trying to leverage libc++.

  • Is there an alternate method in our brew installation script which can force this at install time?
  • Would this PR benefit by tagging the upstream author?

Tests macOS 10.11, Xcode 7.3 b3

  • PASS brew reinstall https://raw.githubusercontent.com/tresf/homebrew-core/patch-2/Formula/stk.rb --from-source
  • PASS Recompile (relink) downstream project against stk

What other scenarios can I help test?

Does this have any appreciable difference on post-10.9 builds?

I think the safe answer is, no, no appreciable difference.

@MikeMcQuaid
Copy link
Member

but this formula by default seems to link against libstdc++ which won't work with any modern project

That's the default standard library on < 10.9 so you should continue to use that. Another option is to add fails_with :clang for the old versions so it builds with a modern GCC and libstdc++ there or simply mark this formula as requiring 10.9 or above with depends_on :macos => :mavericks

@tresf
Copy link
Contributor Author

tresf commented Jan 18, 2017

That's the default standard library on < 10.9 so you should continue to use that.

Isn't this presumptuous?

so it builds with a modern GCC and libstdc++

But it's libstdc++ that's the problem. Without a way to force libc++ when the dependency is downloaded, the package is dead in the water.

Is there a way to simply force the formula to use ENV.libcxx at brew reinstall time so we can just shim this downstream?

or simply mark this formula as requiring 10.9 or above

I'm neither qualified nor an authority to make this call.

@MikeMcQuaid
Copy link
Member

Isn't this presumptuous?

No. macOS ships with a given standard library.

But it's libstdc++ that's the problem. Without a way to force libc++ when the dependency is downloaded, the package is dead in the water.

libc++ isn't provided on all versions of macOS and it's horribly broken on some older versions.

Is there a way to simply force the formula to use ENV.libcxx at brew reinstall time so we can just shim this downstream?

I'm not sure what this means.

I'm neither qualified nor an authority to make this call.

That's fine, I am. Let's just make this depends_on :macos => :mavericks.

@tresf
Copy link
Contributor Author

tresf commented Jan 18, 2017

depends_on :macos => :mavericks.

I'm building on 10.8, Xcode 5.5, will that work?

I'm not sure what this means.

I'll paraphrase. If I add ENV.libcxx to the install step of the formula, it forces the stk formula compile to rebuild and relink against libc++, which we use explicitly now. Is there a brew ... command line switch during formula retrieval which allows one to override ENV.libcxx? If so, we'll put the switch in our documentation and close this as invalid. If not, we need to hand-edit the formula each time we build on that platform. Note, of 14 dependencies, this stk package is currently the only formula which generates the Undefined symbols for architecture x86_64 linking errors. Not even Qt5.5 throws these.

No. macOS ships with a given standard library.

  • AFAIK, Xcode 5.5 ships with ability to link against libstdc++ as well as libc++
  • AFAIK we require an the older SDK in order to run on older OSs

and it's horribly broken on some older versions.

Ok. So do we just hand-patch this? If so, we can link to the commit in our documentation and close this out, but I found other formulas using this technique too, so I'm not sure what the approval process looks like.

@MikeMcQuaid
Copy link
Member

I'm building on 10.8, Xcode 5.5, will that work?

In that case it won't allow you to build, no.

If I add ENV.libcxx to the install step of the formula, it forces the stk formula compile to rebuild and relink against libc++, which we use explicitly now.

We don't support using libc++ in this way on those older versions.

AFAIK, Xcode 5.5 ships with ability to link against libstdc++ as well as libc++

It does. As a package manager, though, we need to use a consistent standard library to avoid issues.

AFAIK we require an the older SDK in order to run on older OSs

You shouldn't be building in Homebrew at all to run on older OSs.

So do we just hand-patch this?

Yes, please.

@tresf
Copy link
Contributor Author

tresf commented Jan 24, 2017

You shouldn't be building in Homebrew at all to run on older OSs.

Can you please expand on this statement?

@MikeMcQuaid
Copy link
Member

@tresf We try very hard to build optimised for a single platform and, ideally, single computer. If you want to support older versions of macOS I'd advise building outside of Homebrew where you have more control over the environment.

@tresf
Copy link
Contributor Author

tresf commented Jan 24, 2017

If you want to support older versions of macOS I'd advise building outside of Homebrew where you have more control over the environment.

Thanks for the advice. Although this sounds great in practice, large projects ( > 0.5m lines of code) rely on solid package managers like Homebrew for resolving dependency problems. Since XCode does not offer backwards compatibility option, packagers need to have simple, well-defined steps for various environments and Homebrew is a part of those steps for many large projects.

So regardless of whether or not this libc++ change is a sane default for 10.8, as a package maintainer, I'm having difficulty understanding the pros in maintaining a true sandboxed build environment from scratch. Homebrew (and friends) is really the only sane option I'm aware of.

@tresf tresf deleted the patch-2 branch March 21, 2017 01:52
@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