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

Bugfix #3

Merged
merged 2 commits into from
Sep 6, 2019
Merged

Bugfix #3

merged 2 commits into from
Sep 6, 2019

Conversation

stnoh
Copy link
Contributor

@stnoh stnoh commented Sep 6, 2019

I recently tried your repository and suffered from the similar problem in #1 .
After some checking, I found that lapack-blas DLLs (libblas.dll, ...) were missing at the installation.
I guess, these DLLs were located in your system path but they weren't in the other guys system.

Anyway, the simplistic solution of #1 is just copy lapack-blas DLLs (libblas.dll, ...) to sksparse directory after the installation, but I think that it is quite annoying. Here I complement setup.py by using data_files option to copy DLLs at the installation (0329c1e).

Replacing Cython (d49d172) is just my preference, but I believe it is reasonable since this repository only aims to support Windows.

- remove Cython-0.25.2 for Linux
- add Cython-0.29.13 for Windows
- update SOURCES.txt due to this change
- setup.py: copy DLLs in lapack_blas_windows by using "data_files"
- SOURCES.txt: add those DLLs (automatically updated)
@xmlyqing00 xmlyqing00 merged commit 8811a4f into xmlyqing00:master Sep 6, 2019
@xmlyqing00
Copy link
Owner

Thank you @stnoh ! Good job! DLLs parts are clear.

I'm not familiar with Cython. Is it another compile option? Could you explain more what benefits does Cython bring in this package? Maybe writing some sentences in the README.md could be better. Thank you!

@stnoh stnoh deleted the bugfix branch September 7, 2019 02:09
@stnoh
Copy link
Contributor Author

stnoh commented Sep 7, 2019

AFAIK, Cython is a kind porting option from C to Python. Since C is binary-dependent, Linux version Cython won't match with Windows'. I know there are few other options for C-to-Python porting (pybind11, for example), but I do not want to change many things from the original one...
Anyway, here it is used for creating sksparse/cholmod.cp36-win_amd64.pyd (You can check it from egg file) from scikit-sparse-0.4.4/sksparse/cholmod.c . CHOLMOD is built by Python, but it still wants to use binary version of lapack-blas DLLs. This was the reason of DLL problem in #1 .

@xmlyqing00
Copy link
Owner

I see. It's clear. Thank you for your contributions.

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.

2 participants