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

Polyfill distutils.copy_tree because it was removed #3229

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ChillerDragon
Copy link
Contributor

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)

sudo python3 scripts/make_release.py master macos

0s Run sudo python3 scripts/make_release.py master macos
Traceback (most recent call last):
  File "/Users/runner/work/teeworlds/teeworlds/scripts/make_release.py", line 2, in <module>
    from distutils.dir_util import copy_tree
ModuleNotFoundError: No module named 'distutils'
Error: Process completed with exit code 1.

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?

macos_tw_release_no_distutils

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.

macos_tw_release_no_diff
ChillersMacBookPro:teeworlds chillerdragon$ git checkout pr_polyfill_copy_tree 
Switched to branch 'pr_polyfill_copy_tree'
ChillersMacBookPro:teeworlds chillerdragon$ python scripts/make_release.py master macos
cleaning target
download and extract languages
trying http://github.com/teeworlds/teeworlds-translation/archive/master.zip
download and extract maps
trying http://github.com/teeworlds/teeworlds-maps/archive/master.zip
adding files
making disk image
*** cleaning ***
done
ChillersMacBookPro:teeworlds chillerdragon$ mv teeworlds-master-macos.dmg pr_polyfill_copy_tree.dmg
ChillersMacBookPro:teeworlds chillerdragon$ git checkout master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
ChillersMacBookPro:teeworlds chillerdragon$ python scripts/make_release.py 
wrong number of arguments
scripts/make_release.py VERSION PLATFORM
ChillersMacBookPro:teeworlds chillerdragon$ python scripts/make_release.py master macos
cleaning target
download and extract languages
trying http://github.com/teeworlds/teeworlds-translation/archive/master.zip
download and extract maps
trying http://github.com/teeworlds/teeworlds-maps/archive/master.zip
adding files
making disk image
*** cleaning ***
done
ChillersMacBookPro:teeworlds chillerdragon$ mv teeworlds-master-macos.dmg master.dmg
ChillersMacBookPro:teeworlds chillerdragon$ open master.dmg 
ChillersMacBookPro:teeworlds chillerdragon$ open pr_polyfill_copy_tree.dmg 
ChillersMacBookPro:teeworlds chillerdragon$ cd /Volumes/Teeworlds
ChillersMacBookPro:Teeworlds chillerdragon$ find . -exec sha1sum {} \; 2>&1 | sort > /tmp/master.txt
ChillersMacBookPro:Teeworlds chillerdragon$ cd /Volumes/Teeworlds\ 1
ChillersMacBookPro:Teeworlds 1 chillerdragon$ find . -exec sha1sum {} \; 2>&1 | sort > /tmp/pr_polyfill_copy_tree.txt
ChillersMacBookPro:Teeworlds 1 chillerdragon$ git diff --no-index /tmp/master.txt /tmp/pr_polyfill_copy_tree.txt
ChillersMacBookPro:Teeworlds 1 chillerdragon$ sha1sum /tmp/master.txt /tmp/pr_polyfill_copy_tree.txt 
8a53e5b8e5bbf40296e1ed739d30354b1690af2b  /tmp/master.txt
8a53e5b8e5bbf40296e1ed739d30354b1690af2b  /tmp/pr_polyfill_copy_tree.txt
ChillersMacBookPro:Teeworlds 1 chillerdragon$ 

@ChillerDragon ChillerDragon force-pushed the pr_polyfill_copy_tree branch 2 times, most recently from ff12562 to 07eccc6 Compare February 25, 2024 14:45
@ChillerDragon
Copy link
Contributor Author

ChillerDragon commented Feb 25, 2024

I force pushed to also polyfill it in download.py so it fixes the windows cmake build too when python 3.12 is used.
Now all uses of distutils are polyfilled.

$ grep -r distutils
scripts/twlib.py:       # https://docs.python.org/2/distutils/apiref.html#distutils.dir_util.copy_tree
scripts/twlib.py:       from distutils.dir_util import copy_tree

I also verified that make_release.py creates the exact same release on linux

main@debian:~/Desktop/git/teeworlds$ git checkout pr_polyfill_copy_tree 
Switched to branch 'pr_polyfill_copy_tree'
Your branch is up to date with 'origin/pr_polyfill_copy_tree'.
main@debian:~/Desktop/git/teeworlds$ python scripts/make_release.py master linux_x86_64
cleaning target
download and extract languages
trying http://github.com/teeworlds/teeworlds-translation/archive/master.zip
download and extract maps
trying http://github.com/teeworlds/teeworlds-maps/archive/master.zip
adding files
making tar.gz archive
*** cleaning ***
done
main@debian:~/Desktop/git/teeworlds$ tar xf teeworlds-master-linux_x86_64.tar.gz 
main@debian:~/Desktop/git/teeworlds$ mv teeworlds-master-linux_x86_64 pr_polyfill_copy_tree
main@debian:~/Desktop/git/teeworlds$ rm teeworlds-master-linux_x86_64.tar.gz 
main@debian:~/Desktop/git/teeworlds$ git checkout master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
main@debian:~/Desktop/git/teeworlds$ python scripts/make_release.py master linux_x86_64
cleaning target
download and extract languages
trying http://github.com/teeworlds/teeworlds-translation/archive/master.zip
download and extract maps
trying http://github.com/teeworlds/teeworlds-maps/archive/master.zip
adding files
making tar.gz archive
*** cleaning ***
done
main@debian:~/Desktop/git/teeworlds$ tar xf teeworlds-master-linux_x86_64.tar.gz 
main@debian:~/Desktop/git/teeworlds$ mv teeworlds-master-linux_x86_64 master
main@debian:~/Desktop/git/teeworlds$ cd master/
main@debian:~/Desktop/git/teeworlds/master$ find . -exec sha1sum {} \; 2>&1 | sort > /tmp/master.txt
main@debian:~/Desktop/git/teeworlds/master$ cd ../pr_polyfill_copy_tree/
main@debian:~/Desktop/git/teeworlds/pr_polyfill_copy_tree$ find . -exec sha1sum {} \; 2>&1 | sort > /tmp/pr_polyfill_copy_tree.txt
main@debian:~/Desktop/git/teeworlds/pr_polyfill_copy_tree$ cd ..
main@debian:~/Desktop/git/teeworlds$ wc -l /tmp/master.txt
688 /tmp/master.txt
main@debian:~/Desktop/git/teeworlds$ wc -l /tmp/pr_polyfill_copy_tree.txt
688 /tmp/pr_polyfill_copy_tree.txt
main@debian:~/Desktop/git/teeworlds$ sha1sum /tmp/master.txt
011c25ed7e12cde3e959ee202b1e91ae01ff9254  /tmp/master.txt
main@debian:~/Desktop/git/teeworlds$ sha1sum /tmp/pr_polyfill_copy_tree.txt
011c25ed7e12cde3e959ee202b1e91ae01ff9254  /tmp/pr_polyfill_copy_tree.txt

@ChillerDragon ChillerDragon force-pushed the pr_polyfill_copy_tree branch 3 times, most recently from d3a0f8f to f436914 Compare February 26, 2024 08:24
@heinrich5991
Copy link
Contributor

Python 2 support could probably be dropped at this point. It went end of life 2019-12-31, more than four years ago.

@ChillerDragon
Copy link
Contributor Author

ChillerDragon commented Feb 26, 2024

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.

@heinrich5991
Copy link
Contributor

heinrich5991 commented Feb 26, 2024

I was under the impression that Python 2.7 support is related to to the polyfilling instead of calling shutil.copytree directly. If that's not the case, why not call shutil.copytree directly?

@ChillerDragon
Copy link
Contributor Author

I was under the impression that Python 2.7 support is related to to the polyfilling instead of calling shutil.copytree directly. If that's not the case, why not call shutil.copytree directly?

Because I am using the dirs_exist_ok option that was introduced in 3.8 which closer replicates distutils.copy_tree according to https://stackoverflow.com/a/64340026

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 dirs_exist_ok then I will happily remove the distutils and dirst_exist_ok flag. Otherwise I rather be safe than sorry.

@heinrich5991
Copy link
Contributor

Python 3.7 is end-of-life since 2023-06-27, more than half a year ago: https://devguide.python.org/versions/.

@ChillerDragon
Copy link
Contributor Author

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?

@heinrich5991
Copy link
Contributor

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?

@jxsl13
Copy link
Contributor

jxsl13 commented Feb 27, 2024

single core potato PCs of third world countries maybe.
or for people with restricted access to the internet who cannot easily update to a newer version?

@jxsl13
Copy link
Contributor

jxsl13 commented Feb 27, 2024

I think a poll might solve that problem.
"What is your current python version".

Add command to that poll to determine the current version. for both, python 2 and python 3.

@ChillerDragon
Copy link
Contributor Author

ChillerDragon commented Feb 27, 2024

For what system do you want to continue support that ships Python ≤ 3.7?

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

 -- ******** Teeworlds ********
-- Target OS: windows 64bit
-- Compiler: C:/Program Files/Microsoft Visual Studio/2022/Enterprise/VC/Tools/MSVC/14.38.33130/bin/HostX64/x64/cl.exe
-- Build type: Debug
-- Dependencies:
--  * Freetype not found
--  * OpenSSL Crypto not found
--  * Pnglite not found (using bundled version)
--  * Python3 found
--  * SDL2 not found
CMake Error at CMakeLists.txt:364 (message):
--  * Wavpack not found (using bundled version)
--  * Zlib not found (using bundled version)
  You must install Freetype to compile the Teeworlds client


-- Automatically downloading GTest to be able to run tests
CMake Error at CMakeLists.txt:367 (message):
  You must install SDL2 to compile the Teeworlds client

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.

@heinrich5991
Copy link
Contributor

single core potato PCs of third world countries maybe.
or for people with restricted access to the internet who cannot easily update to a newer version?

Handwaving. Please be more concrete.

I think a poll might solve that problem. "What is your current python version".

Add command to that poll to determine the current version. for both, python 2 and python 3.

I don't think any operating system still doesn't ship Python 3.

@jxsl13
Copy link
Contributor

jxsl13 commented Feb 27, 2024

I think operating systems that have not been updated for ages still have old versions.

github-merge-queue bot pushed a commit to teeworlds-community/teeworlds that referenced this pull request Mar 2, 2024
Polyfill distutils.copy_tree because it was removed teeworlds#3229
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.

None yet

3 participants