-
Notifications
You must be signed in to change notification settings - Fork 629
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
Polyfill distutils.copy_tree because it was removed #3229
base: master
Are you sure you want to change the base?
Polyfill distutils.copy_tree because it was removed #3229
Conversation
ff12562
to
07eccc6
Compare
I force pushed to also polyfill it in download.py so it fixes the windows cmake build too when python 3.12 is used.
I also verified that make_release.py creates the exact same release on linux
|
d3a0f8f
to
f436914
Compare
f436914
to
f35345a
Compare
Python 2 support could probably be dropped at this point. It went end of life 2019-12-31, more than four years ago. |
Yes agree. But is that really related to this pr? This pr has nothing to do with python 2. But with distutils that got removed in 3.12 which released in 2023. |
I was under the impression that Python 2.7 support is related to to the polyfilling instead of calling |
Because I am using the But yes if we do not need that option then we could just use shutils and never import distutils. I wanted to not change any behavior at all cost. Because I am not familiar with all the details why copy_tree was even needed and what exactly it should be able to do. If someone feels comfortable saying that we do not need |
Python 3.7 is end-of-life since 2023-06-27, more than half a year ago: https://devguide.python.org/versions/. |
Yes end of life is bad. But I would still prefer not to drop its support. That if statement is worth it imo. What do you think? |
For what system do you want to continue support that ships Python ≤ 3.7? |
single core potato PCs of third world countries maybe. |
I think a poll might solve that problem. Add command to that poll to determine the current version. for both, python 2 and python 3. |
I have no particular system in mind. I just like back compat especially if its low cost. And half a year end of life is not very old if you ask me. Considering how many python 2 fanbois are still out there. If I had to come up with a scenario check this: Someone uses a python version manager to adjust the python version. That is commonly happening to run projects that depend on old versions. Such old projects exist. And not everyone uses venv's some use a user wide python manager such as pyenv. Now that person still has the version 3.7 set from working on some legacy code base that no one has ported to new python yet. Suddenly the teeworlds cmake build fails with the following non obvious error
It might not be that exact error. I did not test the unhappy path of 3.7. This is 3.12 without distutils. But I assume that the unsupported option could have similar results. |
Handwaving. Please be more concrete.
I don't think any operating system still doesn't ship Python 3. |
I think operating systems that have not been updated for ages still have old versions. |
Polyfill distutils.copy_tree because it was removed teeworlds#3229
what and why
The GitHub actions macOS CI pipeline is failing on this step if python 3.12 is used. (which is not the case on current master but at some point it would be nice to update to this or a newer version #3223)
teeworlds/.github/workflows/build.yaml
Line 161 in a1911c8
I managed to reproduce the issue on my mac locally. By uninstalling the setuptools pip package. Sadly I did not manage to fix the CI by installing the setuptools pip package. Anyways to my knowledge none of the scripts in this repo depend on external pip packages so that would be a sloppy fix anyways.
The whole distutils package is being removed from python. So for newer versions it has to be replaced with something else. As far as I understood the migration advice, users have to implement copy_tree them selfs. So I decided to polyfill it.
Some dude on stackoverflow recommended to use shutils.
It is using a python 3.8 option. So the poly fill only kicks in for versions that have it. Older versions should still have distutils.
is it still working correctly?
I wanted to make sure that nothing broke. So my first test was running client and server and comparing the file in the volume by looking at it with my eyes. That was a success. The second test i did was comparing the sha1sum of the dmg files. That was a failure. Then I realised that .dmg file creation is non deterministic. And the old and new copy_tree would produce a new sha1sum of the dmg file on every run. So the test was flawed.
So that is why I decided to mount the volume and sha1sum every single file. And then diff all sha1sums of both volumes. And that showed no diff. So to my knowledge the same release is being created but the code is now more portable. And supports python version 3.12 and newer.