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

WIP: use skbuild to reduce build complexity #189

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Erotemic
Copy link

The scikit-build project has a goal of reducing the difficulty running Python setup with compiled extension modules. It does this by acting as a seamless glue between setup.py and CMakeLists.txt.

Of course all of the complexity then moves to CMakeLists, however that's a good thing because CMake helps to some degree. CMake is inherently cross platform and there is a large ecosystem of package support. However, writing a CMakeLists.txt can still be tricky, which is why this is a WIP.

The reason for this PR is because pygraphviz has always been a pain when it comes to pip installing it. Its easy to pip install opencv and PyQt5 these days, so it should be easy to install pygraphviz too. My hope is that by moving to skbuild it becomes easy to make a robust installable package.

@Erotemic
Copy link
Author

@dschult This is currently "working on my machine" (tm). I'm currently trying to figure out why its not working on Travis. In the meantime, a code review may be worthwhile.

@dschult
Copy link
Contributor

dschult commented Feb 18, 2019

I'm afraid this is beyond my expertise. Not even sure how to test it to see if it works on my machine.
Any hints on good ways to check if it works on a local install?

@Erotemic
Copy link
Author

Erotemic commented Feb 22, 2019

I was able to get the build to work in a conda 3.7 environment. I installed the graphviz libraries through conda. I haven't yet been able to make it work with system graphviz libs, but I also didn't look into it too deeply (the error I saw was probably due to a local issue on my system).

Adding the --verbose to pip in travis just makes the travis error more visible. I haven't figure it out yet, but I'm slowly working on it.

EDIT: it seems like adding setup.py clean before running bdist_wheel did the trick. This isn't the first time I've seen an issue where running skbuild twice causes an error. Perhaps if @jcfr might have some insight if he is interested?

The good news is that all versions except pypy are now working on travis.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 81.578% when pulling b77dc6c on Erotemic:dev/add_skbuild into 9deff5d on pygraphviz:master.

@jarrodmillman jarrodmillman added this to the 1.6 milestone Jul 8, 2020
@jarrodmillman jarrodmillman marked this pull request as draft July 18, 2020 01:55
@dschult
Copy link
Contributor

dschult commented Aug 2, 2020

PyPy fails during setup because it can't find a python_library (python_library is None here. And there is a TODO comment about it here). I don't know enough about skbuild or python libraries to see how this code should be changed to work with pypy.

It looks like skbuild and pypy are incompatible at this time. (Their names and overall goals suggest they will not become compatible... one tries to connect python with C and the other tries to remove python from C :)

@dschult
Copy link
Contributor

dschult commented Aug 2, 2020

All non-pypy tests are passing. The pypy tests fail installing skbuild.

@Erotemic
Copy link
Author

Erotemic commented Aug 3, 2020

Did the pypy versions ever build and hook into the C extensions? Very quickly, looking at the code it seems pypy does have support for C-extensions? So I guess this is a failure of scikit-build.

If this is a scikit-build issue, perhaps we can patch that to include pypy support, in which case this could be triaged until that is ready. Looks like there is an issue here: scikit-build/scikit-build#419

I may look into getting that fixed if I have some time.

@dschult
Copy link
Contributor

dschult commented Aug 3, 2020

Yes, the immediate problem is that skbuild does not set a return value for python_library if one is not found. The resulting value of None leads to:
TypeError: unsupported operand type(s) for +: 'str' and 'NoneType'
I believe the value is supposed to hold a path to the libpython library file. I looked for such a library file for pypy on my system and couldn't find one. I'm not sure what from pypy would be able to play the role of libpython from CPython. Sorry for this brain-dump without any real information, but hope it helps.

@jarrodmillman jarrodmillman removed this from the 1.6 milestone Aug 5, 2020
Base automatically changed from master to main February 18, 2021 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants