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

Only ship libgfortran #3

Merged
merged 4 commits into from
Jul 15, 2016
Merged

Only ship libgfortran #3

merged 4 commits into from
Jul 15, 2016

Conversation

jakirkham
Copy link
Member

@jakirkham jakirkham commented Jul 10, 2016

Currently, we are shipping the whole libgcc package on Linux and OS X with scipy. This has proved problematic in our stack especially as was seen with numpy ( conda-forge/numpy-feedstock#15 ). However, unlike with openblas we cannot simply just ship libgfortran as scipy has C++ code and thus depends on libstdc++. Given the shipping of libstdc++ was the cause of the issues with code depending on numpy, it is tricky to find an easy fix here.

There are two main options to avoid shipping libstdc++. First, try to use gfortran and libgfortran from the gcc package only and otherwise use the image compilers and libraries. Second, try to use system compilers for the whole process, but have everything link to our libgfortran instead. Attempts to get the first point to work with openblas proved tricky and we were unable to get it to work. So, we are skipping that option and going for the second one instead. Hopefully we can evaluate the effectiveness of this strategy and move forward from there.

Fortunately the OS X situation is straightforward. We switched over to clang and thus libc++ in PR ( #2 ). So we are not tied to libstdc++. Thus it is merely a matter of having a libgfortran package on OS X. This is handled by PR ( conda-forge/staged-recipes#1022 ), which creates such a package by pulling out the needed libraries on OS X.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@jakirkham jakirkham changed the title WIP: Only ship libgfortran on Linux Only ship libgfortran on Linux Jul 11, 2016
@jakirkham
Copy link
Member Author

This works fine for me locally. I have tried building a binary and using it in a container without libgfortran and everything seems to work correctly. No further changes on my end. Please let me know if any changes are needed.

@rgommers
Copy link
Contributor

This needs a rebase now.

@jakirkham
Copy link
Member Author

Yep, kind of made it that way intentionally. 😉 Will do that now.

@jakirkham
Copy link
Member Author

Rebased. 😄

@rgommers
Copy link
Contributor

If I understand correctly, there will now be two libgfortrans at build time. You're building here with the gfortran from devtoolset but linking against the conda-provided libgfortran. And that this is the preferred libgfortran is guaranteed by <something I don't see so quickly> ?

@jakirkham
Copy link
Member Author

jakirkham commented Jul 11, 2016

Right that is the tricky thing. It is in the LIBRARY_PATH. Additionally patchelf goes back through and ensure that conda's $PREFIX is in the RPATH. This shows the correct linkage and my experimentation locally has confirmed this does indeed work. Though am welcome to other thoughts on how this might be done.

Edit: This comment said LD_LIBRARY_PATH and has been corrected to LIBRARY_PATH.

@rgommers
Copy link
Contributor

That sounds OK to me. I'm just asking because it wasn't quite clear, and even if I knew how to figure this out from the included packages I'd probably forget again later on. Is this kind of stuff in a few people's heads right now, or is there documentation in a central place somewhere?

@jakirkham
Copy link
Member Author

The patchelf stuff is handled by conda-build. So, is briefly explained in their docs.

Now I realized I said LD_LIBRARY_PATH, but that was incorrect. We are setting LIBRARY_PATH. This is what is forcing libgfortran to picked up from the PREFIX instead of the system. I'd be happy to add a comment or two for this.

Something else we might consider is running conda inspect linkages in the test section. While not a test of its own, it would serve to show how things are getting linked in scipy and thus provide a good reference. We can definitely run this on Linux, but I believe there was a bug when using it on Mac and don't believe it has been fixed.

@pelson
Copy link
Member

pelson commented Jul 11, 2016

This looks pretty good to me. 👍

@rgommers
Copy link
Contributor

Now I realized I said LD_LIBRARY_PATH, but that was incorrect. We are setting LIBRARY_PATH. This is what is forcing libgfortran to picked up from the PREFIX instead of the system. I'd be happy to add a comment or two for this.

Right, I see it now in build.sh. Both that and the conda-build docs are pretty clear, no comment needed I think.

@jakirkham
Copy link
Member Author

Alright, if we are happy with it, I'd be happy to see it merged. Though I think we should merge PR ( #4 ) first.

@pelson
Copy link
Member

pelson commented Jul 12, 2016

So this fixes the goal of avoiding shipping libstdc++ on Linux, but doesn't solve the problem for OSX, right?

@jakirkham
Copy link
Member Author

So this fixes the goal of avoiding shipping libstdc++ on Linux...

Correct.

...but doesn't solve the problem for OSX, right?

I decided to split this problem out over 2 PRs. That one ( #2 ) has already been accepted.

Though I may need to go back and do some cleanup. This is probably another special case where we need to think about how to improve the toolchain.

@jakirkham
Copy link
Member Author

Though if you are referring to the larger case of only shipping libgfortran for scipy on OS X, that PR does tee us up for it, but we still need the libgfortran package to finish the job.

@pelson
Copy link
Member

pelson commented Jul 12, 2016

Do you want to bump the version number @jakirkham?

@jakirkham
Copy link
Member Author

Yes I do. 🤦 Fixing it now.

@jakirkham jakirkham changed the title Only ship libgfortran on Linux WIP: Only ship libgfortran on Linux Jul 13, 2016
@jakirkham
Copy link
Member Author

Let's hold off on this until we can do the same for OS X (which should be soon).

@pelson
Copy link
Member

pelson commented Jul 13, 2016

👍

@jakirkham
Copy link
Member Author

I have added libgfortran for OS X here too. Though it will require that package to be uploaded.

I would strongly prefer to see PR ( #4 ) merged first as I don't want to run the risk of having a possibly broken OS X package if it can be avoided.

@jakirkham jakirkham changed the title WIP: Only ship libgfortran on Linux WIP: Only ship libgfortran Jul 14, 2016
@jakirkham
Copy link
Member Author

Also, while not strictly necessary, I would like to have openblas rebuilt with the changes from this PR ( conda-forge/openblas-feedstock#11 ) before we go down this road. Not that I expect it should matter in anyway, but just in case it does for some non-obvious reason.

@pelson
Copy link
Member

pelson commented Jul 14, 2016

I'm OK to merge this @jakirkham. Is there a reason it is WIP?

@jakirkham
Copy link
Member Author

Basically getting those other PRs in and OpenBLAS rebuilt. Also, wanted to retest this once OpenBLAS was built as it had been pulling in libgcc, but now should only pull in libgfortran.

For extra validation, I have also put up PR ( conda-forge/numpy-feedstock#18 ) and PR ( conda-forge/numpy-feedstock#19 ) to verify the same behavior I saw locally that numpy builds and tests out ok.

Once all of these pass, we should be fine to merge.

@jakirkham jakirkham changed the title WIP: Only ship libgfortran Only ship libgfortran Jul 14, 2016
@jakirkham
Copy link
Member Author

So both of the numpy tests passed as expected. This should be good to merge once CI completes.

@jakirkham
Copy link
Member Author

Assuming no further feedback. This is ready to go on my end.

@jakirkham
Copy link
Member Author

@pelson, care to do the honors?

@pelson pelson merged commit ec00534 into conda-forge:master Jul 15, 2016
@jakirkham jakirkham deleted the req_libgfortran branch July 15, 2016 15:09
@jakirkham
Copy link
Member Author

Thanks @pelson. Hopefully this makes things a little easier on people.

@pelson
Copy link
Member

pelson commented Jul 15, 2016

Great job @jakirkham. Thanks for doing this.

@jakirkham
Copy link
Member Author

This is now released! 🎇 🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants