search

conda-forge / openblas-feedstock

A conda-smithy repository for openblas.
BSD 3-Clause "New" or "Revised" License
9 stars 39 forks source link

Only require `libgfortran` at run-time on Linux #6

Closed jakirkham closed 8 years ago

jakirkham commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

cc @pelson @ocefpaf @msarahan

jakirkham commented 8 years ago

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.

pelson commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

I believe you are correct.

jakirkham commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

jakirkham commented 8 years ago

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

msarahan commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

...or by verify are you discussing the versioning constraints in PR ( https://github.com/conda-forge/staged-recipes/pull/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 commented 8 years ago

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

msarahan commented 8 years ago

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 notifications@github.com 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 https://github.com/conda-forge/openblas-feedstock/pull/6#issuecomment-228405766, or mute the thread https://github.com/notifications/unsubscribe/AACV-Wi_ZT1725mFmkjerOkb1bHMruB6ks5qPBEmgaJpZM4IzbJV .

jakirkham commented 8 years ago

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.