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

ghc: fix build on 10.12 #2029

Closed
wants to merge 1 commit into from

Conversation

mistydemeo
Copy link
Member

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same formula update/change?
  • Have you built your formula locally prior to submission with brew install <formula> (where <formula> is the name of the formula you're submitting)?
  • Does your submission pass brew audit --strict --online <formula> (after doing brew install <formula>)?

Fixes an issue with building GHC on 10.12 because of the clock_gettime function. See #1957.

@BrewTestBot BrewTestBot added the in progress Stale bot should stay away label Jun 16, 2016
@ilovezfs
Copy link
Contributor

Timed out. This one took about 3 hours last time, so probably needs about a 4 hour timeout from now on.

@MikeMcQuaid
Copy link
Member

@mistydemeo Given how much Apple changes between the first developer preview and final release I think it may be worth waiting at least until the public beta before we start applying patches. Thoughts?

@ilovezfs
Copy link
Contributor

@MikeMcQuaid maybe just apply the patch with system "/usr/bin/patch", … if the detected OS is 10.12?

@MikeMcQuaid
Copy link
Member

We can do conditional patches without needing that. My concern is that we apply a patch that turns out to be unnecessary in future and we never realise. Maybe that's paranoid, I don't know, I don't feel strongly either way (except that it's a good idea to only patch this on 10.12).

@ilovezfs
Copy link
Contributor

How can a patch-do be limited to a specific OS?

@MikeMcQuaid
Copy link
Member

With an if around it; doesn't end up having the same problems as options do.

@ilovezfs
Copy link
Contributor

Ah OK. @mistydemeo filed an upstream issue about it, so I think it's a question of what upstream decides to do, but I'd not be paranoid about this from the Apple side. They've decided to implement clock_gettime for the first time, and there's definitely no reason to imagine they'd later want to go back to having no implementation for it. Several of the programs on opensource.apple.com patch around the fact that macOS has not previously had clock_gettime, so they're probably already undoing their various patches for that.

Inevitably, the patch in this PR will become unnecessary as soon as upstream stops hard coding the notion that macOS won't ever have the function, which clearly turned out to be false :)

@mistydemeo
Copy link
Member Author

@MikeMcQuaid Regardless of whether Apple changes this or not, this is definitely still an upstream bug; the #ifdefs are buggy, and there's no harm from patching it like this. Of the changes that Apple makes, I also doubt that they'll roll back adding a new POSIX function. :)

@MikeMcQuaid
Copy link
Member

@mistydemeo 👍 then 😀

@DomT4
Copy link
Member

DomT4 commented Jun 16, 2016

My position loosely on 10.12 patches is that we'll take them if submitted as PRs & shown to be working, but I'm not all that prepared myself to go chasing issues around until things are a little more stable, FWIW. This one makes sense to me, and much ❤️ to Misty for the willingness to chase it.

@mistydemeo
Copy link
Member Author

@BrewTestBot test this please

@MikeMcQuaid
Copy link
Member

@mistydemeo I think the patch should just be scoped to 10.12 if it's only needed there and if you've tested it locally it can go in without CI (as the jobs aren't testing 10.12 anyway).

@mistydemeo
Copy link
Member Author

@MikeMcQuaid Since it's likely to be taken upstream, I'll apply the patch on all platforms. I'll pull without bottles though.

@BrewTestBot BrewTestBot removed the in progress Stale bot should stay away label Jun 16, 2016
@MikeMcQuaid
Copy link
Member

@mistydemeo I'd rather we avoid applying patches on platforms they aren't needed 😢

@mistydemeo mistydemeo deleted the ghc_clock_gettime branch June 16, 2016 20:54
@mistydemeo
Copy link
Member Author

@MikeMcQuaid OK, scoped it to only 10.12. My preference though, for things that will be applied upstream, is to patch on all platforms even if they only affect certain platforms.

@MikeMcQuaid
Copy link
Member

@mistydemeo 🙇

@ilovezfs ilovezfs added the clock_gettime CI fails due to clock_gettime symbol label Nov 1, 2016
@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
clock_gettime CI fails due to clock_gettime symbol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants