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

Futureproofing: Removing distutils dependencies #5992

Merged
merged 15 commits into from Sep 28, 2023

Conversation

AndroxxTraxxon
Copy link
Contributor

Python has been warning that distutils will be removed for a while now, and the retirement is official in Python3.12, so this is my attempt at removing/replacing distutils functionality.

I opted to not replace the pip version check in the st2client package because Pip 6.1.0 (April 2015) was released before Python 3.6 (Dec. 2016, already EOL) existed, and we specify the pip version in the Makefile. I'm sure we can find a way to get packaging installed for the version check, or to replace it with a custom-rolled replacement comparison if the community deems it necessary.

I could also see the argument for removing that check from the fixate-requirements.py script as well, but I wanted to show the most commonly recommended replacement for distutils.version.

This change is similar to the one made for the Orquesta package here, basically just removing functionality that's been deprecated for a while and is officially removed in an upcoming version of Python: StackStorm/orquesta#253

@pull-request-size pull-request-size bot added the size/XL PR that changes 500-999 lines. Consider splitting work into several ones that easier to review. label Jun 20, 2023
@AndroxxTraxxon AndroxxTraxxon changed the title initial attempt at removing distutils Futureproofing: Removing distutils dependencies Jun 20, 2023
@AndroxxTraxxon
Copy link
Contributor Author

I had to remove the Pip version check in its entirety in order to remain compatible to Python 3.6. It does make me wonder if it should still be supported, since it's already past EOL.

Makefile Outdated
@@ -661,7 +661,7 @@ distclean: clean

.PHONY: .requirements
.requirements: virtualenv
$(VIRTUALENV_DIR)/bin/pip install --upgrade "pip==$(PIP_VERSION)"
$(VIRTUALENV_DIR)/bin/pip install --upgrade "pip==$(PIP_VERSION)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the trailing slash intentional here?

@nzlosh
Copy link
Contributor

nzlosh commented Jun 20, 2023

Overall the change looks good to me. I do wonder if the pip checks should disabled rather than completely removed as they may still be useful for other versions of python. These checks will probably end up being performed by pants, so maybe not such a big deal to remove them.

@guzzijones
Copy link
Contributor

Give this error a look please: unit test error

@AndroxxTraxxon
Copy link
Contributor Author

AndroxxTraxxon commented Sep 13, 2023

@guzzijones
Those tests failed with a warning about some race conditions being possible:

st2.st2common.services.coordination: WARNING: Coordination backend is not configured. Code paths which use coordination service will use best effort approach and race conditions are possible.

I'm not entirely sure what causes this, but I don't think this change caused those failures. You can see that these tests are the same tests that were cancelled in the last merge to master

You can see in this job run where I effectively changed nothing (just trailing space stuff to meet linting rules), but the test passed. This doesn't surprise me given the warning that comes with the error.

@AndroxxTraxxon
Copy link
Contributor Author

Overall the change looks good to me. I do wonder if the pip checks should disabled rather than completely removed as they may still be useful for other versions of python. These checks will probably end up being performed by pants, so maybe not such a big deal to remove them.

The comparison was being done to ensure an appropriate version of pip was being used in the installation, but in the Makefile, we pin that pip version to 20.3.3 during the installation, which is well past the 6.0.0 minimum that was being enforced. If these kinds of checks are going to additionally be performed by the pants system, this is all the more reason to remove it now.

@AndroxxTraxxon
Copy link
Contributor Author

The last test failure is another race condition for a test that was not modified. You can see in previous commits that it passed just fine

st2.st2common.services.coordination: WARNING: Coordination backend is not configured. Code paths which use coordination service will use best effort approach and race conditions are possible.

Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

Thanks, very helpful in the context of difficulties with the StackStorm's Python versions support StackStorm/community#103

I guess @cognifloyd 👀 you might be interested to take look too, as it's build/packaging related.

Comment on lines +35 to +37
def get_python_lib():
"""Replacement for distutil.sysconfig.get_python_lib, returns a string with the python platform lib path (to site-packages)"""
return get_path("platlib")
Copy link
Member

Choose a reason for hiding this comment

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

Just for reference: https://bugs.python.org/issue41282#msg393018 how it's a replacement for distutil.sysconfig.get_python_lib.

CHANGELOG.rst Outdated Show resolved Hide resolved
@arm4b arm4b added this to the 3.9.0 milestone Sep 15, 2023
@guzzijones
Copy link
Contributor

The last test failure is another race condition for a test that was not modified. You can see in previous commits that it passed just fine

st2.st2common.services.coordination: WARNING: Coordination backend is not configured. Code paths which use coordination service will use best effort approach and race conditions are possible.

Yes, sadly the unit tests run without the coordinator turned on. A redis container is linked to the unit tests, but it is not used. Also, it looks like some unit tests patch the NoCoordinator. I noticed this on our own fork.

Copy link
Contributor

@guzzijones guzzijones left a comment

Choose a reason for hiding this comment

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

looks good to me.

@guzzijones guzzijones merged commit 6f5ac8a into StackStorm:master Sep 28, 2023
28 checks passed
@AndroxxTraxxon AndroxxTraxxon deleted the distutils-removal branch October 11, 2023 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XL PR that changes 500-999 lines. Consider splitting work into several ones that easier to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants