teeworlds-community / teeworlds

A community fork of teeworlds. Trying to have more active maintainers and merge prs faster.
Other
11 stars 1 forks source link

Polyfill distutils.copy_tree because it was removed #3229 #85

Closed teeworlds-mirror closed 8 months ago

teeworlds-mirror commented 9 months ago

upstream: https://www.github.com/teeworlds/teeworlds/pull/3229

ChillerDragon commented 9 months ago

Fuck yeah macOS-latest is passing.

ChillerDragon commented 9 months ago

As @MilkeeyCat reported. The windows pipeline suffered from the same issue. I was so tunnel visioned on fixing macOS make_release.py first. I forgot to think about copy_tree() being used in other places. One other place was download.py which then fails the windows pipeline if python 3.12 is used. So I moved the polyfill to twlib.py and now all pipelines are passing ✅✅✅✅✅✅✅ 🚀

ChillerDragon commented 9 months ago

Okay so thinking about this again. The code seems to work fully but it is quite some code and it has some weird parts. If the maintainers upstream request changes then our teeworlds-community code is different if we merge it before. So I would love to get in some feedback of the usual nitpickers before this gets merged here. If one of these: heinrich, robsti, dune or oy. Has a look and requests no changes I am okay with merging it here. But I would say its worth the wait for their feedback. Because their feedback will likely block the pr being merged upstream.

Things I would consider possible nitpicks:

Any chance you could review this @heinrich5991, @Robyt3 that would be so cute of you :3

MilkeeyCat commented 9 months ago

you can also use smth like

try:
    from distutils.dir_util import copy_tree
except ModuleNotFoundError:
    print("use smth else")

instead of if statement

ChillerDragon commented 9 months ago

you can also use smth like

try:
    from distutils.dir_util import copy_tree
except ModuleNotFoundError:
    print("use smth else")

instead of if statement

I like that. Seems smoother than my code. Will probably force push that later.

MilkeeyCat commented 9 months ago

Also about imports, i could make it work like

import twlib
from twlib import copy_tree

and it works just fine, so u dont need make make that many changes. if something doesn't work, lemme know.

ChillerDragon commented 9 months ago

Thanks @MilkeeyCat I applied all your suggestions. I also reduced the comment text. I personally could not find any nitpicks anymore. But still lets give robsti and heinrich a bit time to respond. If they have no time we just merge it.

ChillerDragon commented 8 months ago

Okay it has been a week. Heinrich had a quick look at it. I would say its time to finally get all pipelines passing and merge this :rocket: