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

superenv: help Autotools with 10.12 SDK on 10.11 #970

Merged
merged 1 commit into from
Sep 15, 2016

Conversation

ilovezfs
Copy link
Contributor

The GNU Autotools tests for whether a given symbol is defined are
reliably coming to incorrect conclusions on 10.11 with the 10.12 SDK
in Xcode 8. This overrides its decisions by forcing the right answer
in superenv using ac_cv_func_* environment variables and setting them to
"no" on 10.11. The list of problematic symbols is from

grep 'weak$os10.11' MacOSX.sdk/usr/lib/system/libsystem_c.tbd

@BrewTestBot BrewTestBot added the in progress Maintainers are working on this label Sep 15, 2016
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Great work on this 👏

# Filter out symbols known not to be defined on 10.11 since GNU Autotools
# can't reliably figure this out with Xcode 8 on its own yet. The Xcode
# version doesn't need to be checked since these are harmless with Xcode 7
# even if they're unnecessary.
Copy link
Member

Choose a reason for hiding this comment

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

Personally I'd add the Xcode check anyway because better safe than sorry but will leave that decision up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if MacOS.version == "10.11" && DevelopmentTools.clang_build_version >= 800

yes?

Copy link
Member

Choose a reason for hiding this comment

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

Yeh, or you can do MacOS::Xcode.version >= "8.0"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My concern with that is that it won't fix the problem on a CLT-only install assuming Apple puts the same stuff in the 10.11 Xcode 8 CLT (if there is one actually coming out).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. Yes, your version is better then. CC @DomT4 to confirm about the CLT?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about the CLT on 10.11 yet to be honest because Apple releases CLT packages scoped to individual platforms, i.e. there's one available for macOS Sierra but not El Capitan, but when El Capitan gets its version it'll get its own .pkg.

It seems feasible they could push a CLT for 10.11 without the problems, but who knows until they do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's avoid speculatively fixing bugs and just do

if MacOS.version == "10.11" && MacOS::Xcode.installed? && MacOS::Xcode.version.to_i >= 8

We can always expand its scope later as needed.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed except avoid the .to_i and just do MacOS::Xcode.version >= "8.0" 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeh, feel free 😁

Copy link
Contributor

@zmwangx zmwangx left a comment

Choose a reason for hiding this comment

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

I wouldn't have thought OS X doesn't have basename_r and dirname_r lol.

Copy link
Member

@DomT4 DomT4 left a comment

Choose a reason for hiding this comment

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

Part of me wonders if it's better to add this change as an unique method like determine_cccfg, etc. I can't imagine this problem getting smaller in future years, and it may be easier to keep track of that way.

No big preference either way though, just a thought. Thanks for working on this & getting a PR pushed so quickly 🙇.

The GNU Autotools tests for whether a given symbol is defined are
reliably coming to incorrect conclusions on 10.11 with the 10.12 SDK
in Xcode 8. This overrides its decisions by forcing the right answer
in superenv using ac_cv_func_* environment variables and setting them to
"no" on 10.11. The list of problematic symbols is from

  grep 'weak$os10.11' MacOSX.sdk/usr/lib/system/libsystem_c.tbd
@ilovezfs
Copy link
Contributor Author

PR refreshed.

Part of me wonders if it's better to add this change as an unique method like determine_cccfg, etc. I can't imagine this problem getting smaller in future years, and it may be easier to keep track of that way.

It may eventually benefit from something like that, but I think for now that's probably over-engineering it.

Copy link
Member

@mistydemeo mistydemeo left a comment

Choose a reason for hiding this comment

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

🙇

@DomT4
Copy link
Member

DomT4 commented Sep 15, 2016

👍. I'm happy unless you're planning to add a test for this change or anything. Otherwise, let's ship it and see what breaks.

@ilovezfs ilovezfs merged commit a148aa3 into Homebrew:master Sep 15, 2016
@BrewTestBot BrewTestBot removed the in progress Maintainers are working on this label Sep 15, 2016
@ilovezfs
Copy link
Contributor Author

🚀

@DomT4
Copy link
Member

DomT4 commented Sep 15, 2016

Can confirm this at least fixes xz's runtime failure of false empty archive messages & installing python on 10.11 with Xcode 8.

People will need to reinstall xz and other impacted-at-run formulae by the looks of it.

@ilovezfs
Copy link
Contributor Author

People will need to reinstall xz and other impacted-at-run formulae by the looks of it.

indeed.

@chdiza
Copy link
Contributor

chdiza commented Sep 15, 2016

I got a malfunctioning xz with Xcode 8 on Sierra, which also was fixed by this PR (as far as I can tell).

@chdiza
Copy link
Contributor

chdiza commented Sep 15, 2016

That is to say: even with Xcode 8 and the associated CLT, on Sierra, xz gave me the "empty archive" nonsense and any formula I built whose tarball was an xz-ball was defective ("illegal instruction 4").

But after this PR it seemed to work.

@zmwangx
Copy link
Contributor

zmwangx commented Sep 15, 2016

This PR specifically has a MacOS.version == "10.11" guard, so I doubt it should have any effect on your Sierra environment.

@ilovezfs
Copy link
Contributor Author

@chdiza what's brew config have to say for itself?

@chdiza
Copy link
Contributor

chdiza commented Sep 16, 2016

brew --config has this to say:

HOMEBREW_VERSION: 0.9.9
ORIGIN: https://github.com/Homebrew/brew
HEAD: a148aa3a41d34a07caab5d17daa26b4749ff3c50
Last commit: 15 hours ago
Core tap ORIGIN: https://github.com/Homebrew/homebrew-core
Core tap HEAD: 0e99665f347c1554f55fbecf448956a44ccd99e5
Core tap last commit: 14 hours ago
HOMEBREW_PREFIX: /usr/local
HOMEBREW_REPOSITORY: /usr/local/hb
HOMEBREW_CELLAR: /usr/local/Cellar
HOMEBREW_BOTTLE_DOMAIN: https://homebrew.bintray.com
CPU: quad-core 64-bit ivybridge
Homebrew Ruby: 2.0.0-p648
Clang: 8.0 build 800
Git: 2.8.4 => /Applications/Xcode.app/Contents/Developer/usr/bin/git
Perl: /usr/bin/perl
Python: /usr/bin/python
Ruby: /usr/bin/ruby => /System/Library/Frameworks/Ruby.framework/Versions/2.0/usr/bin/ruby
Java: N/A
OS X: 10.12-x86_64
Xcode: 8.0
CLT: 8.0.0.0.1.1472435881
X11: N/A

Maybe it wasn't this particular commit that fixed the problem, but another one that landed within around 24 hours.

ilovezfs added a commit to ilovezfs/homebrew-core that referenced this pull request Sep 17, 2016
Erlang uses some customized Autotools tests, so the general superenv
workaround in Homebrew/brew#970 is insufficient
ilovezfs added a commit to Homebrew/homebrew-core that referenced this pull request Sep 18, 2016
Erlang uses some customized Autotools tests, so the general superenv
workaround in Homebrew/brew#970 is insufficient

Closes #4920.

Signed-off-by: ilovezfs <[email protected]>
ilovezfs added a commit to ilovezfs/homebrew-core that referenced this pull request Oct 18, 2016
gnutls uses some customized Autotools tests, so the general superenv
workaround in Homebrew/brew#970 is insufficient
ilovezfs added a commit to Homebrew/homebrew-core that referenced this pull request Oct 18, 2016
gnutls uses some customized Autotools tests, so the general superenv
workaround in Homebrew/brew#970 is insufficient
@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