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

Enable pep517 builds #27

Merged
merged 2 commits into from
Feb 8, 2023
Merged

Enable pep517 builds #27

merged 2 commits into from
Feb 8, 2023

Conversation

jonathanberthias
Copy link
Contributor

The python ecosystem is slowly deprecating the setup.py file, and moving towards a more flexible build system based on PEP 517. Some tools such as Poetry have decided to enable PEP 517 by default, which means packages which are not compatible and do not provide wheels for every possible architecture cannot be installed (see e.g. #25).

This PR allows pip install --use-pep517 to work by adding a minimal pyproject.toml file.

@CLAassistant
Copy link

CLAassistant commented Oct 13, 2022

CLA assistant check
All committers have signed the CLA.

@ErikBjare
Copy link

ErikBjare commented Oct 24, 2022

Also bumping into this issue, would be great to see this merged!

@vineetbansal Plz merge ❤️

@ErikBjare
Copy link

I've switched to using qdldl from @jonathanberthias branch and can confirm it works great.

@bendajam
Copy link

bendajam commented Dec 2, 2022

I would also like this to be merged it would be fantastic!

@vineetbansal
Copy link
Contributor

vineetbansal commented Dec 2, 2022

Sorry for not getting to this sooner. This looks good to me. Thanks @jonathanberthias. @imciner2, @bstellato - any concerns? This is the setup we're using in osqp-python for a little while now as well.

@imciner2
Copy link
Member

imciner2 commented Dec 2, 2022

Adding the pyproject to support pep-517 is good, and my only point is a question: Why do we need to remove the setup_requires from setup.py for this? Will that break any installs that try to use the older setup.py-based toolchain instead of the new pep-517 one?

@jonathanberthias
Copy link
Contributor Author

Indeed it does not have to be removed. However, the setup_requires keyword is officially deprecated, as is using the setup.py based workflow.
If you want to keep it, I will revert that change.

@vineetbansal
Copy link
Contributor

vineetbansal commented Dec 5, 2022

@imciner2, @jonathanberthias - my recommendation would be to not include it (i.e. keep things as-is). An explicit failure on python setup.py <command> is actually a good indication to the user that they're doing something wrong. It is trivial for a user to update their pip using pip install --upgrade pip in the unlikely scenario that their pip is too old to understand pep517.

@imciner2
Copy link
Member

imciner2 commented Dec 6, 2022

What version of Python/pip was this support added in? My main concern is that QDLDL is built as a scientific/mathematical library, and some users could be on clusters that aren't regularly updated (and that they don't have permissions on). I'm fine adding the parallel build system to modernize it, but I am less sure about intentionally breaking the old build.

@jonathanberthias
Copy link
Contributor Author

@imciner2 PEP517 support was added in pip 19: https://pip.pypa.io/en/stable/news/#v19-0, which was released in Jan 2019.

@JacobHayes
Copy link
Contributor

I coincidentally made #29 before seeing this PR. That one leaves setup_requires alone if the maintainers want to just go with that (otherwise, feel free to close!)

@vineetbansal
Copy link
Contributor

vineetbansal commented Feb 7, 2023

Sorry for the delay here. If we're going to keep setup_requires to support pre-Jan 2019 versions of pip, I'd propose at least changing the min. required version of setuptools in pyproject.toml to be setuptools>=49.5.0, since that version (correctly) ignores the setup_requires section, so that these build-time dependencies don't end up in the runtime environment.

@jonathanberthias - would you mind putting back the setup_requires and adding the setuptools>=49.5.0 to pyproject.toml? I can merge this in once done.

If I'm missing/misunderstanding any issue here, let me know.

Copy link
Contributor

@JacobHayes JacobHayes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just adding @vineetbansal's requests as GH suggestions to make it easier to apply

pyproject.toml Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@vineetbansal vineetbansal merged commit 8211ee6 into osqp:master Feb 8, 2023
@jonathanberthias jonathanberthias deleted the pep517 branch February 8, 2023 07:33
@jonathanberthias
Copy link
Contributor Author

Thanks for taking care of this @vineetbansal @JacobHayes!

@bperkins24
Copy link

I'm not seeing this available on pypi.org, am I missing something?

@vineetbansal
Copy link
Contributor

This is related to issue #23. It may be time to make a new qdldl-python release. Let's discuss it there.

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.

8 participants