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

Add permit_weak_imports directive #741

Merged
merged 2 commits into from
Aug 19, 2016

Conversation

tdsmith
Copy link
Contributor

@tdsmith tdsmith commented Aug 18, 2016

Issue Homebrew/homebrew-core#3727 suggested we set -no_weak_imports for new versions of Xcode to ensure that e.g. building on 10.11 against the 10.12 SDK doesn't result in a situation where autotools thinks symbols exist that don't actually exist on the current platform.

Further discussion in golang/go#16770 revealed that some packages require weak imports to build normally.

This PR allows formulas to opt out of -no_weak_imports.

@tdsmith tdsmith added the in progress Maintainers are working on this label Aug 18, 2016
Issue Homebrew/homebrew-core#3727 suggested we set -no_weak_imports for
new versions of Xcode to ensure that e.g. building on 10.11 against the
10.12 SDK doesn't result in a situation where autotools thinks symbols
exist that don't actually exist on the current platform.

Further discussion in golang/go#16770 revealed that some packages
require weak imports to build normally.
@MikeMcQuaid
Copy link
Member

Reading golang/go#16770 (comment) I'm not sure this is the right solution.

I think we should strip -mmacosx-version-min=10.6 from Go's CFLAGS because everything else we do is build around building bottles that only work on a specific version and it's that that's causing the conflict.

Alternatively, we only apply -no_weak_imports on Xcode 8 on OS X 10.11 (or, in general, only when using the SDK that doesn't match the OS version). @ilovezfs remind me: we have other issues that only affect that case, right?

CC @achivetta for thoughts on this.

@ilovezfs
Copy link
Contributor

@MikeMcQuaid Yeah, my SDKROOT bug currently affects

ansible
baresip
h2o
mitmproxy
passenger
rrdtool
uwsgi
vim
wolfssl

I'm not thrilled with this -no_weak_imports idea. It seems like we're anticipating a need to cater to build systems that may make bad assumptions about macOS. I'd rather see us actually hit those bugs and then report them upstream rather than trying to side-step a problem that may or may not exist, and is basically always going to be an upstream bug when it happens. The right place for formulae that would need this is most likely 💀 since it means their upstream is no longer actively supporting macOS.

@MikeMcQuaid
Copy link
Member

@ilovezfs to be more explicit: does that bug also only affect e.g. using the OS X 10.X+1 SDK on OS X 10.X? If so, it feels like we do want to start special-casing that situation (which we previously decided to not do).

@ilovezfs
Copy link
Contributor

to be more explicit: does that bug also only affect e.g. using the OS X 10.X+1 SDK on OS X 10.X

Yes.

@MikeMcQuaid
Copy link
Member

Yeh, I think we may want various options that apply only to that case, then, rather than fixing it on a formula-by-formula basis.

@ilovezfs
Copy link
Contributor

@MikeMcQuaid set depends_on :macos => :el_capitan for every formula, and when Sierra ships, change them to depends_on :macos => :sierra. Problem solved. 😈

@tdsmith
Copy link
Contributor Author

tdsmith commented Aug 18, 2016

I think the relevant comment in the Go discussion is:

If I'm understanding the Go code correctly, it makes sense that Go does this because (a) it is building content that it'll static link into the binaries it produces, which should deploy back to 10.6 and (b) it has a check before its use of SecCertificateCopyNormalizedIssuerContent to fallback to an older path on older OS, which is the correct way to use a weak link.

We don't care whether our go executable can run on 10.6 but we do care about whether the go we build can produce executables that run on 10.6; the suggestion is that go needs to use the minimum deployment target flag to achieve that.

I might not have expected this was the case because go can cross-compile to darwin on systems without an Apple SDK available? but I also know nothing about go internals.

@MikeMcQuaid
Copy link
Member

@tdsmith In that case I think it makes sense to just set this flag for 10.11 using the 10.12 SDK (i.e. Xcode 8) and have a general block/method for handling the other various quirks that need set in this situation (although this can be punted to another PR).

@MikeMcQuaid
Copy link
Member

To be more explicit: I'm definitely 👎 on disabling this on a formula-by-formula basis and we can consider just disabling this flag on OS X 10.12 entirely for now if what I've described is too much work to do in the near future.

@DomT4
Copy link
Member

DomT4 commented Aug 18, 2016

I think removing mmacosx-version-min would be a mistake in this case, presuming our reading of the upstream situation with Go is correct.

Whilst we don't build our Go-using formulae to run as far back as 10.6, I think it'd be the wrong thing to do to make anyone who wants to develop backwards-compatible executables with go outside of Homebrew reinstall go from an alternative source. That feels pretty hostile & likely in the end would result in us having to introduce a special :go dependency.

@tdsmith
Copy link
Contributor Author

tdsmith commented Aug 18, 2016

Just for expectation-setting, I don't have any immediate plans to work on a different approach; I just thought this sounded like a chance to play with ENV internals. :)

I'm not really compelled by the idea that build failures against future SDKs are upstream bugs but I think that's probably a separate discussion vs. this PR.

@achivetta
Copy link

I don't have a strong opinion on the right approach here; my goal was just to make sure you had the tools to solve the broken packages before macOS GM'ed. But let me see if I can sort out the various cases.

There are three versions at play here:

  • The SDK version, which is determined by which Xcode is installed.
  • The OS version the user is running.
  • The deployment target set by the code, or the OS version if none.

OS == SDK == deployment is the common case of building random autoconf-based open source software on the latest macOS. In this case, -no_weak_imports should be a noop.

OS == deployment < SDK is the case where you are on OS X 10.11 with Xcode 8. When building software that wasn't written with macOS-style weak linking and a specific deployment target in mind (likely including almost everything that uses autoconf), this is what -no_weak_imports will fix (or at least move from a runtime failure to a build time failure).

deployment < OS == SDK and deployment < OS < SDK are situations where the developer almost certainly wanted the old weak-linking behavior. Go falls into this bucket and we shouldn't add -no_weak_imports here.

Okay, so a few thoughts on how to proceed:

  • If there's a way to detect when the project is passing -mmacos-version-min= and avoid adding -no_weak_imports, that's almost certainly a good thing to do. If not, you'll need an opt-out for such projects.
  • Only applying the flag when OS < SDK will likely result in fewer broken builds for users, but also hides issues that only occur when in that situation. e.g. if that was the current behavior, we wouldn't know that Go was broken in that situation.

Hope that helps! (Also, CC @jeremyhu in case he has other thoughts.)

@MikeMcQuaid
Copy link
Member

If there's a way to detect when the project is passing -mmacos-version-min= and avoid adding -no_weak_imports, that's almost certainly a good thing to do. If not, you'll need an opt-out for such projects.

@achivetta Thanks, that's really helpful. There is a way to do this but only on a single compiler invocation e.g. if you pass clang -mmacos-version-min=10.6 -Wl,no_weak_imports we can filter out the -Wl,no_weak_imports but if e.g. clang -mmacos-version-min=10.6 was used by one call and clang -Wl,no_weak_imports by another we would not be able to filter that out as we don't track state between compiler invocations. Do you think it would be sufficient to filter just within a single invocation?

@tdsmith
Copy link
Contributor Author

tdsmith commented Aug 18, 2016

aside: -mmacos-version-min can also be specified as the MACOSX_DEPLOYMENT_TARGET environment variable (see man clang) but I don't think that alone changes the feasibility of the superenv approach

@achivetta
Copy link

@MikeMcQuaid The problem with a per-compiler-invocation approach is that you need the -no_weak_imports flag at link time, but the deployment target takes effect at compile time. I'm not sure how it would work out for e.g. Go which I suspect does have different deployment targets per compilation unit.

@UniqMartin
Copy link
Contributor

Ideally and if we could auto-detect if a formula properly supports compiling against a macOS SDK and can properly deal with the situation that OS/deployment version < SDK version, we could consider turning off some of the adjustments made by our superenv. But since this is unlikely to be possible in practice, I think this PR solves this nicely on a formula-by-formula basis for those (relatively rare) cases where some of our adjustments don't really make sense.

@MikeMcQuaid
Copy link
Member

Thanks everyone. Think it's worth 🚢ing this as-is for now and we'll rethink later if we need to start whitelisting too many formulae.

@MikeMcQuaid MikeMcQuaid merged commit 893c80d into Homebrew:master Aug 19, 2016
@BrewTestBot BrewTestBot removed the in progress Maintainers are working on this label Aug 19, 2016
@tdsmith tdsmith deleted the weak-imports-opt-in branch August 20, 2016 03:51
@MikeMcQuaid MikeMcQuaid mentioned this pull request Aug 22, 2016
4 tasks
@Homebrew Homebrew locked and limited conversation to collaborators May 3, 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.

7 participants