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

Minimum Python version #5

Closed
cimes-isi opened this issue May 18, 2022 · 12 comments
Closed

Minimum Python version #5

cimes-isi opened this issue May 18, 2022 · 12 comments

Comments

@cimes-isi
Copy link

Hello again,

My software runs on an older system that has Python 3.7, but py-build-cmake declares a minimum version of 3.8, so installation fails. Is Python>=3.8 a hard constraint, or is it straightforward to support older versions?

Thanks.

@tttapa
Copy link
Owner

tttapa commented May 18, 2022

The code is written for >=3.8. I just installed Python 3.7 and tried to run it, and it failed on the walrus operator, which was introduced in 3.8. There are probably other incompatibilities as well. I'll give it a try and see how far I get by just splitting all walrus operators into separate statements.

@tttapa
Copy link
Owner

tttapa commented May 18, 2022

I created a py3.7 branch with a version that should work with older versions of Python.

Python 3.7 had different ABIs, and it looks like distlib.wheel doesn't detect the correct ABI on Windows:

https://github.com/tttapa/py-build-cmake/runs/6498522175?check_suite_focus=true

The ABI in the Wheel tag should be cp37m, not cp37. I'm not sure what's going on there, and I don't have much time to look into it right now. But they do have code to do some ABI detection: https://github.com/pypa/distlib/blob/052787b7abcecc5498c1443f4f0ee2867d6a3db0/distlib/wheel.py#L59

It should be noted that the last bugfix release of Python 3.7 was almost two years ago, and in about a year, it will no longer receive security fixes either. Other projects like NumPy, SciPy, etc. already dropped support for Python 3.7 last year.

@cimes-isi
Copy link
Author

I really appreciate you making the effort to look at this! I realize 3.7 is a little older and supporting old software can be annoying, but unfortunately, 3.7 is still out there (on Debian buster in my use case), and in fact so is 3.6 (e.g., on Ubuntu 18.04 LTS systems). Those other projects you mention may have dropped support, but they still have older releases available that support earlier Python versions.

I've been testing your py3.7 branch this morning. I ran into a different issue (see #6), but after forcing a newer cmake version than the system has (3.13.4), I got past it. The branch appears to work correctly on Linux with Python 3.7.3 and macOS with Python 3.8.9. I don't have a Windows environment to try.

@cimes-isi
Copy link
Author

I've been trying to look into this a bit, though my knowledge of Python packaging is limited. I see a related issue at pypa/packaging#181 which references _cpython_abi() in pypa/packaging, similar to what you linked to above. Compare this with the more recent implementation. Perhaps this is a bug in distlib?

The GH Action build is using a slightly outdated pip, so we might consider upgrading it first, though I don't have reason to believe it will resolve the issue.

Cheers.

tttapa added a commit that referenced this issue May 28, 2022
@tttapa
Copy link
Owner

tttapa commented May 28, 2022

Thanks for looking into this!

I've opened an issue with distlib: pypa/distlib#172

I've now used packaging to determine the ABI, which seems to work as expected: 68e751c

I'm not entirely sure about the robustness, perhaps I shouldn't rely on the order of the tags returned by packaging.tags.sys_tags(), the documentation just says

The order of the sequence corresponds to priority order for the
interpreter, from most to least important.

The other relevant functions like _cpython_abis are private.

The GH Action build is using a slightly outdated pip, so we might consider upgrading it first

Pip already gets updated before actually running the tests:

session.install("-U", "pip", "build", "pytest")

The warning originates from the installation of Nox in the global environment, the actual Nox test environment uses the latest version of pip.

tttapa added a commit that referenced this issue May 29, 2022
@cimes-isi
Copy link
Author

I've opened an issue with distlib: pypa/distlib#172

I'm glad we could identify the source - thank you for reporting it upstream. I've subscribed to the issue, hopefully we'll get some feedback soon.

I've now used packaging to determine the ABI, which seems to work as expected: 68e751c

I'm not entirely sure about the robustness, perhaps I shouldn't rely on the order of the tags returned by packaging.tags.sys_tags(), the documentation just says

The order of the sequence corresponds to priority order for the
interpreter, from most to least important.

That's promising. How can we resolve the robustness concern (and, if needed, decide what a better approach should be)? It looks like there's an IRC channel: #pypa on Libera.Chat, where somebody might be able to answer authoritatively.

@cimes-isi
Copy link
Author

I see that distlib 0.3.5 is now released which is supposed to fix the ABI issue in Windows. If we update the distlib version dependency, would that then complete Python 3.7 support? Thanks!

@tttapa
Copy link
Owner

tttapa commented Aug 2, 2022

Release 0.0.11.post1 adds support for Python 3.7 (and 3.6 seems to work as well).

@tttapa tttapa closed this as completed Aug 2, 2022
@cimes-isi
Copy link
Author

Thanks! I see you made this a post release, which appears to be made out of a branch. Will you merge this support into main before your next semantically-versioned release?

@tttapa
Copy link
Owner

tttapa commented Aug 3, 2022

Indeed. The post release doesn't change the behavior of the package, it only makes it compatible with Python 3.6 and 3.7.

I mostly just wanted to have a PyPI version of the package with 3.6 support because RHEL 8 is still on Python 3.6. Unless I make drastic changes or serious bug fixes to py-build-cmake, I don't think it's worth keeping the py3.6 branch up-to-date, though.

For 3.7, I'll definitely keep it updated until its EOL next year, and probably longer for Buster. The differences between main and py3.7 are minor, so that shouldn't be an issue, I might as well merge them when I have a bit more time on my hands.

@cimes-isi
Copy link
Author

Great, thank you! I think it makes sense to merge support for both Python 3.6 and (definitely) 3.7 into main and be done with the branches. At least for the foreseeable future, it doesn't seem like py-build-cmake itself would need to depend on language features in newer Python versions. I expect at some point you may have to update py-build-cmake's dependencies to minimum package versions that drop support older Python version(s), at which point you might be forced to also drop support. But as long as it's easy to support older Python versions, you might as well do so!

@tttapa
Copy link
Owner

tttapa commented Aug 14, 2022

Python 3.7 support has been merged into main (#9).

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

No branches or pull requests

2 participants