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 require libgfortran at run-time on Linux #6

Merged
merged 6 commits into from
Jun 27, 2016
Merged

Only require libgfortran at run-time on Linux #6

merged 6 commits into from
Jun 27, 2016

Conversation

jakirkham
Copy link
Member

This attempts to use only libgfortran as a dependency at run-time dependency. FWICT on Mac only links against libgfortran. However, Linux appears to be a mess and links to both libgcc_s and libgfortran. We try an extremely experimental solution to fix this in this PR.

@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
Copy link
Member Author

cc @pelson @ocefpaf @msarahan

@jakirkham
Copy link
Member Author

After thinking about this some more, I think it might be easiest if we try to use the existing libgfortran on Linux as it seems to have a newer ABI version than the gcc compiler (1.6 vs 1.5). We can make that change now and continue to use libgcc for Mac. We can later swap out libgfortran on Mac for the one pulled from the compiler. I'm just a little wary of creating our own libgfortran on Linux if it will have an older ABI version. Does that make sense?

I'm going to make this change now so all of you can see in this PR what I'm proposing, but will wait for your feedback.

@jakirkham jakirkham changed the title WIP*2: Only require libgfortran at run-time WIP: Only require libgfortran at run-time Jun 24, 2016
@pelson
Copy link
Member

pelson commented Jun 24, 2016

This makes sense to me, but it is worrying how subtle the underlying problem is - seems like we could easily end up in the same place (or worse, crippled by fear of breaking anything). Given the subtlety, I don't know if we can address that though...

@jakirkham
Copy link
Member Author

I think @msarahan was 💯 correct in saying that we need to standardize on compilers. The subtlety of these problems is a direct consequence of having situations where we need to mix and match. For instance, defaults is built with a newer gcc and we have to be very careful not to mess that up; even though, we don't have access to such a compiler ATM. Once we do have such a compiler, we can be more confident in reusing pieces of it.

Though it is also possible that shipping libgcc around is causing some problems here. This is another reason why I'm a bit concerned that I cannot reproduce this ATM. It may come down to mismatches of library versions that we ship versus ones available on the target machine. The only way I know of to solve that is to have a newer version than the system. The version of libgfortran we are shipping here should be pretty new. So, I'm hopeful this doesn't cause problems.

Another point worth mentioning is libquadmath. The libgfortran package from defaults does not include this as discussed in the meeting earlier. However, the standard BLAS interface does not using quad precision floats. Also, I don't see any linkage libquadmath when inspecting this locally. So, this should not affect us.

In any event, built this locally in the container locally from this recipe so that we can try it out. I put this in my own channel so people who would like to play with it and get more comfortable can. I have placed it under a dev label so as not to break myself or others still using this channel on my end. It can be seen here ( https://anaconda.org/jakirkham/openblas/files ). Please try and let me know if this fixes the problem.

@jakirkham
Copy link
Member Author

I was able to run the full numpy test suite (using the existing conda-forge package) with this openblas. So, this does seem to still work as far as that is concerned.

@pelson
Copy link
Member

pelson commented Jun 24, 2016

So, this does seem to still work as far as that is concerned

Cool. So we now need to rebuild anything that has accidentally pulled in a libstdc++ dependency. It will be a hard thing to identify, but at least we know of one candidate (matplotlib).

@jakirkham
Copy link
Member Author

I believe you are correct.

@jakirkham
Copy link
Member Author

We will have to explore the viability of a similar solution with scipy I suspect (as it has C++ code). Fortunately, I was already exploring the possibility of using only gfortran and otherwise using clang/clang++ for Mac and have already had success. We may just need to roll that into the same PR and reuse some of the things learned to apply a similar strategy on Linux (using the devtoolset compiler instead of the packaged gcc/g++).

@jakirkham
Copy link
Member Author

While I was unable to use the existing copy of matplotlib (e.g. import matplotlib.contour) with this in the container, I was able to rebuild matplotlib with a version bump in the container and then import it. So, this is also a good sign.

@jakirkham
Copy link
Member Author

I have placed the rebuilt matplotlib in my channel too. It also has a dev label. ( https://anaconda.org/jakirkham/matplotlib/files )

@jakirkham jakirkham changed the title WIP: Only require libgfortran at run-time Only require libgfortran at run-time Jun 24, 2016
@jakirkham
Copy link
Member Author

I'd like to also know @msarahan's thoughts on this simple yet effective proposal.

@msarahan
Copy link
Member

I haven't taken a detailed look, but I think it should work for now, until we get the compiler built and deployed. It would be good to know somehow which libgfortran you're getting (and I think you want the one from defaults).

@jakirkham
Copy link
Member Author

jakirkham commented Jun 24, 2016

It is the one from defaults. Also, I have verified that it provides ABI version 1.6 at the highest end, which means it will work with things built using the gcc package as that has version 1.5.

@jakirkham
Copy link
Member Author

...or by verify are you discussing the versioning constraints in PR ( conda-forge/staged-recipes#861 ). While this would be nice, I don't want the perfect to be the enemy of the good. This already improves the situation significantly.

@jakirkham
Copy link
Member Author

I say this as it sounds like that will take more manpower/time, which I don't have much of ATM.

@msarahan
Copy link
Member

Nah, that would be better, but just not having libgfortran on conda forge
for now is OK. Just whatever we can do to make it hard for people to
accidentally break things.

On Fri, Jun 24, 2016, 12:17 jakirkham [email protected] wrote:

I say this as it sounds like that will take more manpower/time, which I
don't have much of ATM.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AACV-Wi_ZT1725mFmkjerOkb1bHMruB6ks5qPBEmgaJpZM4IzbJV
.

@jakirkham
Copy link
Member Author

jakirkham commented Jun 24, 2016

Great. It sounds like we are in agreement then.

Would be nice to hear back about some field tests. 😉

I'll let people decide when they are happy enough to merge this (by letting them actually merge it). From my personal testing, this combined with some build number bumps (for packages with C++) fixes the problem AFAICT.

@jakirkham jakirkham changed the title Only require libgfortran at run-time Only require libgfortran at run-time on Linux Jun 24, 2016
@pelson pelson merged commit 405ea12 into conda-forge:master Jun 27, 2016
@jakirkham jakirkham deleted the use_libgfortran branch June 27, 2016 14:14
@jakirkham jakirkham mentioned this pull request Jul 14, 2016
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