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

The version of get_distutils_platform here differs from the one in multibuild #2

Open
mattip opened this issue Dec 18, 2019 · 11 comments

Comments

@mattip
Copy link
Contributor

mattip commented Dec 18, 2019

Two versions of get_distutils_platform exist: the one here and the one in multibuild/common_utils.sh. The one here does not handle other architectures (s390x, ppc64le, aarch64).

@matthew-brett
Copy link
Contributor

Do you have any thoughts how best to fix this?

@mattip
Copy link
Contributor Author

mattip commented Dec 18, 2019

Once choice would be to change the implementation, another would be to rename it here. Do you know if there are users of this repo other than openblas-libs ?

@matthew-brett
Copy link
Contributor

Numpy-wheels and scipy-wheels use this repo - no?

@mattip
Copy link
Contributor Author

mattip commented Dec 19, 2019

They seem to use get_gf_lib for all platforms and install_fortran on Darwin only.

The call to get_gf_lib starts in numpy-wheel/scipy-wheel as get_gf_lib "openblas-${OPENBLAS_VERSION}" "$plat"), which then dives into this repo and returns the openblas tarball, which matches the name on https://3f23b170c54c2533c070-1c8a9b3114517dc5fe17b7c3f8c63a43.ssl.cf2.rackcdn.com/ (which has really old versions of all the things). This then is the name used to download the openblas tarball for building numpy/scipy. I think that means openblas-lib should be also using this repo to get the tarball name it creates, so it should be using this repo's version of get_distutils_platform.

BUT: this version is not aware of the 64 bit builds (openblas64-v0.3* ) which are added after the call to get_distutils_platform nor is it aware of the other manylinux builds. Mess.

@mattip
Copy link
Contributor Author

mattip commented Dec 19, 2019

multibuild has a stripped-down version of this repo in _gfortran_utils.sh. Maybe we should add all this repo to multibuild, including the archive, and update it to handle the additional parameters?

@mattip
Copy link
Contributor Author

mattip commented Dec 19, 2019

or would you prefer adding get_distutils_platform_ex here (and in openblas-libs which calls it) to do all the work?, then once the change in openblas-libs is merged we can think about removing the old function here

@matthew-brett
Copy link
Contributor

I'd prefer to avoid the situation where every user of multibuild pulls in the gfortran binaries. So - it's OK to move the code to multibuild, but maybe some on-the-fly way to get the gfortran binaries.

But - even then - it seems the we're spreading the hack around, which I'd rather avoid.

What do you think?

@mattip
Copy link
Contributor Author

mattip commented Dec 19, 2019

We have four ways to get the openblas library name.

One here, which is used in numpy-wheels and scipy-wheels to download before calling build_bdist_wheel from multibuild, but only for non-windows.

In openblas-libs/appveyor/build_openblas.sh and openblas-libs/travis-ci/build_steps.sh the names are the ones used for uploading. The travis-ci/build_steps.sh uses this repo to get most of the name for downloading uploading, but embellishes it with _64 where needed.

CI for numpy uses numpy/tools/openblas_support.py, which I created to improve the hard-coded names scattered in the CI files

CI for scipy uses hard-coded names in azure-pipelines.yml, appveyor.yml, and .travis.yml, like numpy did before it had the openblas_support.py file.

My choice would be to make everyone use the same code. How do we get there?

Are there other users of the openblas libraries?

matthew-brett added a commit that referenced this issue Dec 31, 2019
MRG: allow more platforms, add unique function name

xref gh-2

Add more platforms to the original version of `get_distutils_platform`

Also add another function that is unique to this repo, which extends the original function for the manylinux version. In time, we should
- convert downstream users `MacPython/openblas-libs` and the various `MacPython/*-wheels` to the new function
- deprecate `get_distutils_platform`
@matthew-brett
Copy link
Contributor

Sorry - where are we with this one?

@mattip
Copy link
Contributor Author

mattip commented Dec 31, 2019

Next up: make openblas-libs use the new function here. Then we need to think about how to get the download name to the wheel builders. I think I prefer the python solution in numpy/tools/openblas_support since it supports windows as well, where a bash shell script will not.

@mattip
Copy link
Contributor Author

mattip commented Jan 30, 2021

Apparently this is the root cause for needing #5

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

No branches or pull requests

2 participants