BLD: avoid using latest setuptools for building 1.8.x#1481
BLD: avoid using latest setuptools for building 1.8.x#1481sgillies merged 8 commits intoshapely:maint-1.8from
Conversation
There was a problem hiding this comment.
It seems that setuptools is hitting a bit of a rough patch getting PEP 660 to work for everything, so it's a good idea to limit this version. The shapely 1.8 series will probably not need anything new from future versions of setuptools. The suggested Cython version capping is also intended to future-proof this release series.
Update: oh wait! I forgot to see the recent build attempts where there are difficulties. Suggest to use setuptools<64 to avoid other recent breaking changes
.github/workflows/tests.yml
Outdated
| run: | | ||
| python -m pip install --disable-pip-version-check --user --upgrade pip | ||
| pip install --upgrade wheel setuptools | ||
| pip install --upgrade wheel "setuptools<65" |
There was a problem hiding this comment.
My guess is that this line is unnecessary, as this is specified in pyproject.toml. It was probably necessary a while ago for dependant projects.
There was a problem hiding this comment.
That was my initial guess as well, but because it didn't work with only editing pyproject.toml, also added this. Now the reason it didn't work with just pyproject.toml is probably because I specified the wrong version ;)
There was a problem hiding this comment.
With tests now passing, it seems that the main fix was with pyproject.toml. Perhaps either remove or leave this line alone. The version of setuptools specified here probably does not make it to the editable install.
There was a problem hiding this comment.
Yep, just pushed a clean-up. We should probably also just remove the installation of wheel and setuptools here, as it should no longer be needed with pyproject.toml (but let's maybe leave such a clean-up for the main branch?)
There was a problem hiding this comment.
A fix to one or both branches is fine with me. (But probably both, so it no longer draws any attention). It's a harmless and mostly useless line that once served a purpose.
Co-authored-by: Mike Taves <mwtoews@gmail.com>
Pull Request Test Coverage Report for Build 2869733035
💛 - Coveralls |
|
@mwtoews thanks for catching that it needed to be <64 and not <65 for having the version without the new editable support! That seems to have done the trick. |
|
I've fixed this in #1479, which I am looking to merge today. |
|
@sgillies I think this PR is the better fix, we don't want to require for users to have to specify that env variable? |
|
@jorisvandenbossche I went the other way for maint-1.8, use of the setuptools feature in combination with a fix in setuptools 65.0.1 works. I'll close this. A different approach for the main branch would be fine with me. |
Regardless of what you merged in the other PR, we can still merge this, I think? (it doesn't conflict, I actually updated this PR after your PR was merged)
As mentioned above, AFAIK this still only works if you specify that env variable? That means that users installing shapely (not using the wheel) should have to do that as well. While this PR limiting the setuptools version doesn't need any additional user action. Of course, in practice it is only for editable installs I think, so the number of affected users might not be that big. But as @mwtoews mentioned above, putting a max setuptools version here also prevents from potential failures with a future setuptools release. |
The main branch doesn't seem to have this problem, there using the latest setuptools did work (#1475) |
|
@jorisvandenbossche 🙏 sounds good! Merging. |
The windows tests on maint-1.8 are currently failing since (I think) the latest setuptools release. That version has quite some changes to how editable installs work (see eg also #1475 that @mwtoews initially did).
Checking here if limiting ourselves to older setuptools versions fixes the problem (to confirm it is indeed triggered by setuptools 65 release). This wouldn't really be a good long-term solution, but for 1.8.x it might also be fine.