-
-
Notifications
You must be signed in to change notification settings - Fork 11.3k
gcc: sandbox within series, not minor release #34303
Conversation
365e607
to
55fb1dc
Compare
It ignored the passed
Full paths here: https://gist.github.com/mistydemeo/8a66cc983f3086ac8a80 The libraries are installed to stable locations, e.g. cc @tdsmith, @staticfloat, @tkelman for feedback |
@@ -25,6 +25,7 @@ def osmajor | |||
url "http://ftpmirror.gnu.org/gcc/gcc-4.9.2/gcc-4.9.2.tar.bz2" | |||
mirror "ftp://gcc.gnu.org/pub/gcc/releases/gcc-4.9.2/gcc-4.9.2.tar.bz2" | |||
sha1 "79dbcb09f44232822460d80b033c962c0237c6d8" | |||
revision 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to revision everything built against gfortran
too I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep - and make sure to give enough notice to outside taps that they can rebuild/revision their stuff before this goes in.
Looks good to me 👍 |
Building Fortran code with gfortran outside of Homebrew produces these install names:
|
Looks good. Would this be spun out to the versioned gcc formulae? Here's the linkage with R:
|
I don't anticipate new releases in any of the pre-4.9 GCC series, so I'd rather err on the side of leaving them as is - that way we don't break any software users have already built just in the name of consistency. I'll apply the same change to |
Thanks for working on this (what's that about Santa Claus?), but I'll have to defer to @staticfloat, if he has time to test this out, to say whether this improves matters for JuliaLang bottling. Definitely looks promising. Is gcc_s in your Fortran test linking to |
That's normal for software built outside of Homebrew; the compiler picks the full path rather than the symlinked path. Inside Homebrew the install names are rewritten post-build, so that would point to |
Ah, gotcha, missed that distinction. So this should fix the bottle problem, but doesn't address the need to rebuild Fortran sources after gcc updates for users who compile things (outside Homebrew) from source with Homebrew's gfortran? That's probably the safe thing to do anyway, but it's a persistent cause of issue filings that we can try to mitigate with better docs. |
@mistydemeo this troubles me:
Compare the linkage to libgfortran.3.dylib and libquadmath.0.dylib vs. the linkage to libgcc_s.1.dylib. I wonder why that one still has a linkage into the Cellar? |
I'm confused too - I'm not sure why that happens. I noticed it occurs with both gfortran and g++. |
Here's the verbose output from
|
The current GCC formula has more or less the same behaviour: # after rebuilding test
$ otool -L ./test
./test:
/usr/local/lib/gcc/x86_64-apple-darwin13.4.0/4.9.2/libgfortran.3.dylib (compatibility version 4.0.0, current version 4.0.0)
/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1197.1.1)
/usr/local/Cellar/gcc/4.9.2/lib/gcc/x86_64-apple-darwin13.4.0/4.9.2/libgcc_s.1.dylib (compatibility version 1.0.0, current version 1.0.0)
/usr/local/lib/gcc/x86_64-apple-darwin13.4.0/4.9.2/libquadmath.0.dylib (compatibility version 1.0.0, current version 1.0.0) |
The dylib ID is set correctly (second line of the above output), so I'm stumped. |
I think I figured it out, I need to compile the whole thing with the install name machinery turned off though. Will report back. |
I'm super busy right now so I don't have time to test in any detail. But
|
With this PR + the following patch: diff --git a/Library/Formula/gcc.rb b/Library/Formula/gcc.rb
index ae1f08d..05866b8 100644
--- a/Library/Formula/gcc.rb
+++ b/Library/Formula/gcc.rb
@@ -133,6 +133,8 @@ def install
args << "--enable-multilib"
end
+ inreplace "libgcc/config/t-slibgcc-darwin", "@shlib_slibdir@", "#{HOMEBREW_PREFIX}/lib/gcc/#{version_suffix}"
+
mkdir "build" do
unless MacOS::CLT.installed?
# For Xcode-only systems, we need to tell the sysroot path. I get:
|
Added and updated configure args. |
Can confirm it works now:
|
@@ -132,6 +130,10 @@ def install | |||
args << "--enable-multilib" | |||
end | |||
|
|||
# Ensure correct install names when linking against libgcc_s; | |||
# see discussion in https://github.com/Homebrew/homebrew/pull/34303 | |||
inreplace "libgcc/config/t-slibgcc-darwin", "@shlib_slibdir@", "#{HOMEBREW_PREFIX}/lib/gcc/#{version_suffix}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to submit this upstream too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jacknagel can answer that, but I think this is an edge case in how we package it / our symlink structure rather than an upstream issue per se.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeh, I think it would still be handy to let upstream at least know that this is something we'd like to configure.
Misty's terminal output above is making me giggle like a preteen. This looks great. |
All right, if this looks good let's set a schedule for getting this in. I'd like all the external taps to have advance notice to revision their formulae and rebuild bottles so they're ready to push at the same time we merge this. How long do you think you need? |
So the proposed plan is that I merge this branch locally, build my bottles and revision my formulae, and push alongside this merge into mainline? |
Yep! You can run |
Great. I totally forgot about |
@tdsmith How much time do you think homebrew-science and homebrew-python will need to get ready? There are no bottles, but there are formula revisions to do. @MikeMcQuaid Is it safe to do formula revisions for things that need fortran in this PR? Will they be rebuilt using the new GCC if their commits come afterwards? |
I know it's a pain, but can you build |
It may well be. I'm trying to fix bot issues but I'll need help from you folks to do so. Remember that anything the bot does can be tested locally with |
I can repro with pyside as the only argument to |
I can't reproduce it using pyside, but I can with binwalk. I don't think it's a bot failure, as I can also reproduce it with |
Python dependency is ending up in the list of binwalk's recursive requirements twice, once as optional and once as required. |
Jack and I were discussing the binwalk issue, and it looks like it's happening here in The Jack's planning to take a look at this over the next few days. |
@tdsmith Out of curiosity - given that binwalk uses pyside, which does have a |
Definitely maybe; our Python and Apple's oughta be binary compatible so as long as libraries don't explicitly link to libpython (they shouldn't) it shouldn't be a problem. Any problems should manifest as segfaults on import. If this has been working it might be a clue that we don't need a We shouldn't need to resource numpy and scipy on OS X >= 10.9 if we use system Python since Apple ships 'em. (Doesn't solve this problem on 10.8 and earlier, alas.) |
I'm going to hold off on merging this until binwalk can be revisioned.
Yep, we still have to build the 10.8 bottle... :( |
@mistydemeo When I try to do the
|
Think we can cheat on fixing dependency resolution right now by pulling #34945, which removes the :python dependency from pyside and shiboken (and oughta be worth doing independent of this issue). |
@BrewTestBot test this please |
This needs a rebase, which I'd done locally but I've held off on merging to avoid starting the bot on a build. Pushing now that @tdsmith has pushed a workaround for binwalk. |
@staticfloat Once the bottles are built, I'll wait on merging this until you're ready - sorry for the issues. |
Yep, no worries. Ping me when the bottles are built, and I'll try building the Julia bottle once more. It should only take me ~20 minutes to build the Julia bottle. |
Bottle build is finished. Let me know when you're ready! |
Great! FFTW bottle is working and Julia bottle is good to go!
|
Had to come in here again and just reiterate how great it's going to be to have this new functionality. It's going to cut down on OSX user issues by a measurable fraction. Thank you so much for this everybody! |
Great! I'll merge this when I wake up tomorrow. |
Merged! |
Further to the discussion in #33948 - this removes
--enable-version-specific-runtime-libs
from GCC, and instead sandboxes libraries and headers into locations versioned by series instead of minor release.