Fix Release Pipeline for distutils #598
Conversation
This reverts commit 2fcd519.
| if sys.platform == 'win32': | ||
| # distutils is deprecated in Python 3.10 and removed in 3.12. However, it still works because Python defines a compatibility interface as long as setuptools is installed. | ||
| # We don't have an official alternative for distutils.ccompiler as of September 2024. See: https://github.com/pypa/setuptools/issues/2806 | ||
| # Once that issue is resolved, we can migrate to the official solution. | ||
| # For now, restrict distutils to Windows only, where it's needed. | ||
| import distutils.ccompiler |
There was a problem hiding this comment.
Can we move the import to the beginning of the file instead? So that we don't need to have two place to import the package?
There was a problem hiding this comment.
If we move the import to the beginning of the file, it will be imported everywhere. Because this is only required on Windows and is deprecated, I have restricted it to Windows.
There was a problem hiding this comment.
I mean move the
if sys.platform == 'win32':
import distutils.ccompiler
to the top of the file
There was a problem hiding this comment.
Sorry, missed it in this PR. I have created #599 to fix this.
| # on CPython, our wheels are abi3 and compatible back to 3.11 | ||
| # on CPython, our wheels are abi3 and compatible back to 3.11 | ||
| if python.startswith("cp") and sys.version_info >= (3, 13): | ||
| return "cp313", "abi3", plat |
There was a problem hiding this comment.
worth to mention that cp313 has a different abi3 (Not sure if it's right wording.)?
There was a problem hiding this comment.
Thanks, we already mention it at https://github.com/awslabs/aws-crt-python/blob/main/setup.py#L408. I will add a comment here as well.
| # Manylinux images don't contain setuptools from Python 3.13, so we need to install it. | ||
| # Install in a custom location due to access issues. | ||
| /opt/python/cp313-cp313/bin/python -m pip install --target ./local -r requirements-dev.txt | ||
| export PYTHONPATH=./local:$PYTHONPATH |
There was a problem hiding this comment.
(debatable) Move those to a prepare-manylinux.sh.
So that have one source instead of copy/paste to 4 places?
And we will remove manylinux1 sometime soon, so it's fine that not invoking it from manylinux1?
More future prove?
There was a problem hiding this comment.
I don't see any prepare-manylinux.sh script. Do you mean creating a new one? I think it's fine for now, since it's just two lines of code, and the rest of the code is different per platform. This is also ABI compatible wheel so we don't expect to do this often.
manylinux1 doesn't support Python 3.13, I believe, so we don't need to add it there.
When we dropped support for python 3.7 in #598 we forgot to update setup.py, and the README
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.