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

since we rename the openblas symbols we also need to rename the library #13407

Merged
merged 1 commit into from
Oct 1, 2015

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Oct 1, 2015

As discussed in #8734, since we rename the openblas symbols (we add a 64_ suffix), we also need to rename the library in order to avoid conflicts. This renames it to add a _64_ 64_ suffix to libopenblas.

I'm hoping to get this changed upstream in openblas (OpenMathLib/OpenBLAS#656), but I want to get this interim patch in Julia in the meantime, as this is a critical bugfix for me — I'm getting a ton of people on Linux who can't use PyPlot (JuliaPy/PyPlot.jl#140) or other numpy-based stuff (JuliaPy/PyCall.jl#65) due to conflicting libopenblas.so libraries. There is a workaround with LD_PRELOAD, but it is awkward and daunting for new users.

(@tkelman, any chance of backporting this fix to 0.4?)

@tkelman
Copy link
Contributor

tkelman commented Oct 1, 2015

This will add an extra underscore relative to what your openblas patch would do. Could we instead pull in that patch file? We need to upgrade openblas to fix a few outstanding bugs once a new version is available.

@tkelman
Copy link
Contributor

tkelman commented Oct 1, 2015

I'll also need to rebuild the SCS.jl binaries and install different downloads conditional on julia version.

Why would this only have started happening within the last few weeks? Did something change about the way pycall imports numpy?

@stevengj
Copy link
Member Author

stevengj commented Oct 1, 2015

@tkelman, I think it is because increasing numbers of people are using the Conda packages, and they apparently use openblas. There have been scattered problem reports for a while, but it also took us a while to diagnose the problem.

Shouldn't SCS.jl just use Base.libblas_name?

@stevengj
Copy link
Member Author

stevengj commented Oct 1, 2015

We could also pull in my OpenBLAS patch, e.g. if we build OpenBLAS from our own branch this would be easy. However, I don't think we should wait on a new OpenBLAS release to fix this in Julia.

Since it is a purely internal library, I didn't think it should matter if we rename it now, and then rename it again when a new OpenBLAS comes out. Shouldn't any package that uses the Julia BLAS libraries be using Base.libblas_name anyway?

@stevengj
Copy link
Member Author

stevengj commented Oct 1, 2015

Oh, because SCS is a pre-built C DLL, then it has to hard-code the soname on Windows.

Maybe I could just modify this patch to only add the suffix on Unix systems. Would that simplify life for SCS?

@tkelman
Copy link
Contributor

tkelman commented Oct 1, 2015

Elemental and a few others I suspect. Anything that relies on a binary dep that itself relies on blas.

I'm against breaking packages twice when it's easy enough to only break them once. I said exactly this on the issue a few weeks ago.

I'm fine with pulling it in as a patch file to make the name consistent with what we will use post-openblas upgrade. Git will complain when that happens but it'll need a rebuild to apply this (and a rebuild again after the version upgrade, such is life) anyway so make -C deps distclean-openblas will be the sledgehammer fix for that.

Would also rather use consistent names on all platforms if we can.

@stevengj
Copy link
Member Author

stevengj commented Oct 1, 2015

I might be able to omit the extra underscore without patching openblas, using the LIBPREFIX Makefile variable in OpenBLAS.

@stevengj
Copy link
Member Author

stevengj commented Oct 1, 2015

Would it eliminate package breakage to not add the suffix on Windows, since only Windows requires you to hard-code the dependency in a pre-built DLL?

@tkelman
Copy link
Contributor

tkelman commented Oct 1, 2015

Maybe, but see my edit. Would rather have names consistent across platforms. I can rebuild the binaries, it's not that big a deal, but would rather only do it once.

…we also need to rename the library to avoid conflicts
@stevengj
Copy link
Member Author

stevengj commented Oct 1, 2015

Okay, see the revised patch, which only appends 64_ to the library name, consistent with my proposed OpenBLAS patch.

@tkelman
Copy link
Contributor

tkelman commented Oct 1, 2015

I think this'll work. Let's try it.

tkelman added a commit that referenced this pull request Oct 1, 2015
since we rename the openblas symbols we also need to rename the library
@tkelman tkelman merged commit 598ac84 into JuliaLang:master Oct 1, 2015
@stevengj stevengj deleted the rename_openblas branch October 2, 2015 00:32
stevengj added a commit that referenced this pull request Oct 2, 2015
…eed to rename the library to avoid conflicts

(cherry picked from commit b0bc951)
ref #13407
@tkelman
Copy link
Contributor

tkelman commented Oct 2, 2015

Arpack, and maybe also suitesparse, are broken by this. Will fix.

tkelman added a commit that referenced this pull request Oct 2, 2015
tkelman added a commit that referenced this pull request Oct 2, 2015
tkelman added a commit that referenced this pull request Oct 2, 2015
@ScottPJones
Copy link
Contributor

I can't build any longer on the Mac, it is getting a bunch of warnings like _LAPACKE_cbbcsd not found. Modification of this symbol failed, and then dies trying to link with errors such as LAPACKE_cbbcsd64 references.

@tkelman
Copy link
Contributor

tkelman commented Oct 2, 2015

Gist the entire error output, otherwise that isn't enough information to go off of. make -C deps distclean-openblas is the usual way to force a clean rebuild here.

@ScottPJones
Copy link
Contributor

cleanall doesn't clean up openblas as well? If not, that might have been my problem.

@ScottPJones
Copy link
Contributor

@tkelman Thanks, that did the trick. Is there a "make reallycleanall", that makes sure you are really starting from scratch?

@tkelman
Copy link
Contributor

tkelman commented Oct 2, 2015

distcleanall

@ScottPJones
Copy link
Contributor

Thanks again. Still running into problems though, running test, got an error that it couldn't open @rpath/libopenblas.dylib, from linalg/arnoldi.jl, line 8, calling aupd_wrapper at linalg/arpack.jl:49.
There is a libopenblas64_.dylib.

@ScottPJones
Copy link
Contributor

(making a link makes the tests pass)

@tkelman
Copy link
Contributor

tkelman commented Oct 2, 2015

arpack and suitesparse link to openblas, so you usually need to distclean them too if you rebuild openblas

@stevengj
Copy link
Member Author

stevengj commented Oct 2, 2015

Why didn't Travis catch the ARPACK breakage?

@tkelman
Copy link
Contributor

tkelman commented Oct 2, 2015

Because it uses system versions of everything. A clean build of all deps from source would take way too long.

jiahao added a commit that referenced this pull request Oct 9, 2015
jiahao added a commit that referenced this pull request Oct 9, 2015
skumagai pushed a commit to skumagai/julia that referenced this pull request Oct 9, 2015
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.

None yet

3 participants