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

Fix Release Pipeline for distutils #598

Merged
merged 15 commits into from
Oct 1, 2024
Merged

Fix Release Pipeline for distutils #598

merged 15 commits into from
Oct 1, 2024

Conversation

waahm7
Copy link
Contributor

@waahm7 waahm7 commented Sep 26, 2024

Description of changes:

Copy link
Contributor

@TingDaoK TingDaoK left a comment

Choose a reason for hiding this comment

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

fix&ship

Comment on lines 329 to +334
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean move the

if sys.platform == 'win32':
   import distutils.ccompiler

to the top of the file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

worth to mention that cp313 has a different abi3 (Not sure if it's right wording.)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment on lines +23 to +26
# 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
Copy link
Contributor

Choose a reason for hiding this comment

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

(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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@waahm7 waahm7 merged commit 5a67c68 into main Oct 1, 2024
64 checks passed
@waahm7 waahm7 deleted the fix-release-setuptools branch October 1, 2024 16:25
@graebm graebm mentioned this pull request Oct 2, 2024
graebm added a commit that referenced this pull request Oct 2, 2024
When we dropped support for python 3.7 in #598
we forgot to update setup.py, and the README
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.

2 participants