-
Notifications
You must be signed in to change notification settings - Fork 2
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
Insist on setup file #58
Conversation
There seems to be a minor issue with the windows python 3.10 test. Poetry grabs numpy 1.21.1 which doesn't have a wheel for windows, but the CI struggles to build it. But all the rest of the tests work, so I think I'm happy for this PR to now be reviewed. |
I added a little bit more to this PR. To summarise, what this PR does is:
|
{version = ">=1.20", python = ">=3.7,<3.10"}, | ||
{version = ">=1.23", python = ">=3.10"} | ||
] | ||
setuptools = ">=51.1.2" | ||
|
||
[tool.poetry.dev-dependencies] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure why these need to be changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed setuptools (and other things) to be >= rather than ~ as I feel the upper bound constraints are too restrictive (see https://iscinumpy.dev/post/bound-version-constraints/). For example, the latest version of setuptools is 67.1.0 and the exact-cover package works absolutely fine with that. But the current pyproject.toml insists one downgrades to 51.3.3 or earlier as it is written ~51.1.2
The higher numpy versioning is for easier install on more recent pythons. There are no binary wheels for numpy 1.20 for python 3.10 and up. When poetry does its dependency resolving it fetches an old version of numpy that it has to compile from source (which caused difficulty on the CI windows builds). My change here was just to force the newer python to use newer numpy where binary wheels are available.
@@ -6,8 +6,8 @@ jobs: | |||
strategy: | |||
fail-fast: false | |||
matrix: | |||
python-version: [3.7, 3.8] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the purpose of this build is to check that the build sdist can correctly be installed.
it would be nice to be sure that both the sdist and the wheel that we distribute can be installed correctly, as the problems you have had were introduced without breaking the build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this issue is now being picked up by the CI. For example, in your PR #59 from yesterday, the sdist builds are failing with the same error I saw on google colab:
https://github.com/jwg4/exact_cover/actions/runs/4088835028/jobs/7050902881
ERROR tests/test_exact_cover.py - ModuleNotFoundError: No module named 'exact_cover_impl'
I think the sdist build problems stem from a breaking change in poetry-core
python-poetry/poetry-core#318
which as of last week's release of poetry-core 1.5.0
https://github.com/python-poetry/poetry-core/releases
no longer generates the setup.py needed for building the binary extension module without us explicitly setting generate-setup-file
to true (which is what this PR does, and why the tests are passing in this PR but not PR #59).
The breaking change to poetry-core was only released a week ago, which is why it's only messing up the CI now. In the comments of the poetry PR, one person comments "I am fairly certain releasing this will break for a few folks building extensions using setuptools, just not sure how large the number is going to be." I think it has indeed broken the exact-cover package, but this PR fixes it.
Hi @johnrudge sorry about the delay in looking at this. If you agree with the very minor point about the version numbers, I think we should merge this. Would you agree that we need to open an issue to remove the dependency on setuptools? Since it seems poetry is now making the setup.py route opt-in, but will in the future remove it altogether. And there are no further new issues thrown off by this work? |
I commented above as to why I changed some of the version constraints. Of course feel free to merge and then change version numbers how you see fit. You're right for the future it would be good to have a simple way to build the binary extension using poetry alone without setuptools. I haven't seen any new issues generated by my changes here. |
I think this small change may fix #57. At least on my machine I seem now to be able to create .tar.gz files that are installable with pip.