wrye-bash / wrye-bash

A swiss army knife for modding Bethesda games.
https://wrye-bash.github.io
GNU General Public License v3.0
455 stars 79 forks source link

Python 3.12 Upgrade #669

Closed Infernio closed 4 months ago

Infernio commented 7 months ago

Some very nice stuff in here (PEP 701, for example). And performance-wise, the inlined comprehensions and smaller strings should be really nice too.

Blockers:

Things to do once blockers are cleared:

lojack5 commented 7 months ago

One thing to note (and I'm having trouble finding where it's mentioned) but the default behaviour of object.__new__ changes in py 3.12 - it used to silently ignore extra arguments passed in, in 3.12 it causes errors. I know we don't explicitly use __new__ much but there are a few cases, so keep an eye out for that. An example of me having to work around this: https://github.com/lojack5/structured/blob/d74ccfca56148bd1d7fafd6e52b2609f7a012440/structured/serializers/structs.py#L112

lojack5 commented 6 months ago

Looks like pygit2 wheels are available on pip for 3.12 now, should be nothing more blocking us from moving to 3.12.

Infernio commented 6 months ago

Great, CI is now failing because of the deprecation warnings for getdefaultlocale. Let's try this: c89593c3e59797b39f41326550ad06576f9aca04

Edit: and the wxPython site is also dead now, so the https://discuss.wxpython.org/t/wxpython4-1-1-python3-8-locale-wxassertionerror/35168/3 link doesn't even work anymore. Archived version: https://web.archive.org/web/20221001122844/https://discuss.wxpython.org/t/wxpython4-1-1-python3-8-locale-wxassertionerror/35168

Infernio commented 6 months ago

I give up, the deprecation warnings are gone but it's still failing. If anyone wants to figure out why, be my guest: https://github.com/wrye-bash/wrye-bash/actions/runs/6786993238/job/18448790305

Edit: oh, just noticed this:

C:\hostedtoolcache\windows\Python\3.12.0\x64\python.exe: can't open file 'C:\\hostedtoolcache\\windows\\Python\\3.12.0\\x64\\Tools\\i18n\\msgfmt.py': [Errno 2] No such file or directory
localize.py  164 setup_locale: Error loading translation file:
Traceback (most recent call last):
  File "D:\a\wrye-bash\wrye-bash\Mopy\bash\localize.py", line 161, in setup_locale
    with open(mo, u'rb') as trans_file:
         ^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: 'D:\\a\\wrye-bash\\wrye-bash\\Mopy\\bash\\l10n\\de_DE.mo'

That's... what? Does 3.12 no longer package msgfmt?

Edit 2: yup, sure enough:

inf@Infernio-VM MSYS ~
$ find /c/Python311/ -iname "msgfmt.py"
/c/Python311/Tools/i18n/msgfmt.py

inf@Infernio-VM MSYS ~
$ find /c/Python312/ -iname "msgfmt.py"
Infernio commented 6 months ago

Found it: https://github.com/python/cpython/issues/97649

Guess we'll need to bundle those with WB now.

Edit: very cool that it's not documented anywhere in the What's New documents...

lojack5 commented 6 months ago

Yeah that's a surprise, all I could find was "Some Python distrubutions package these" in the docs. Looks like python-gettext on PyPI might work though, I check it out.

Infernio commented 6 months ago

It's much easier for us to vendor them, drops some ugly code as well.

Edit: now what https://github.com/wrye-bash/wrye-bash/actions/runs/6787567948/job/18450648558

  <unknown>:1: SyntaxWarning: invalid escape sequence '\D'

What's that supposed to mean???

lojack5 commented 6 months ago

Probably a \D in a regex somewhere and it's not a raw-string. Or an escape sequence in a translatable string and/or the translation?

lojack5 commented 6 months ago

I've been running Bash on 3.12 for months now, guess I should have tried getting the tests running to find these.

lojack5 commented 6 months ago

Found it: it's a translation that dropped a slash:

#: bash\basher.py:8229
msgid "File must be selected from Oblivion Data Files directory"
msgstr "Datei muss aus dem Oblivion\Data Ordner ausgew�hlt werden"

Edit: Very annoying that a non-Python file is being affected like this by Python. I wonder if there's a way to divorce the langauge from it. Hmm.

lojack5 commented 6 months ago

I'm guessing this is the line that causes Python-isms to leak into the string: https://github.com/wrye-bash/wrye-bash/commit/bbd4478a8e478df057e57a60a7d83c98b458862a#diff-4f6fa9e0b2861caf7efd971724c45c0728ea87fa1bde196d4b447008f4be7c9aR192

Now to figure out the purpose of sending the strings to the AST when they should just be text...

Infernio commented 6 months ago

No good, still failing: https://github.com/wrye-bash/wrye-bash/actions/runs/6787930427/job/18451837437

The deprecation message is gone though, so that's nice I guess :sweat_smile:

lojack5 commented 6 months ago

Tried running the tests locally, looks like we still have a sys.getdefaultlocale in the tests. What's the correct change for that, I hadn't realized it was...removed/deprecated?

Infernio commented 6 months ago

Deprecated, with the proposed replacement being locale.getlocale. But that's broken on Windows: https://github.com/python/cpython/issues/82986

Pretty sure that's what breaks the whole wx mess as well.

I did this as a workaround: https://github.com/wrye-bash/wrye-bash/commit/c89593c3e59797b39f41326550ad06576f9aca04#diff-016121937b15d32411bb6ffe956e84939225619912e9e177998e8890502383beR67-R75

lojack5 commented 6 months ago

So we don't have a good replacement for getdefaultlocale? Was just about to suggest a warnings filter, lol - push the problem off til Python 3.15?

Infernio commented 6 months ago

The Python guys wanted to remove it in 3.13, people protested (https://github.com/python/cpython/issues/90817#issuecomment-1345455713), so it was pushed back to 3.15 (https://github.com/python/cpython/issues/111187).

lojack5 commented 6 months ago

I'm noticing strange behavior on the tests locally (via VSCode). This is just an example of running a single test:

c:\Users\Lojack\Desktop\code\wrye-bash\Mopy\bash\tests\test_localize.py::TestLocalize::test_setlocale_fr_CM failed with error:  
 The python test process was terminated before it could exit on its own, the process errored with: Code: 3221225477, Signal: null

Although the output log shows:

 CLIENT: Server listening on port 58880...
Received JSON data in run script
============================= test session starts =============================
platform win32 -- Python 3.12.0, pytest-7.4.2, pluggy-1.3.0
rootdir: c:\Users\Lojack\Desktop\code\wrye-bash
collected 1 item

Mopy\bash\tests\test_localize.py 

******* Testing loc='fr_CM' *******
getlocale: (None, None)
localize.py  124 setup_locale: Wrye Bash does not support the language family 'fr', will however try to set locale to 'fr_CM'
localize.py  136 setup_locale: Set wxPython locale to 'fr_CM'
******* Tested loc='fr_CM' *******
.                                       [100%]

============================== 1 passed in 0.90s ==============================
Finished running tests!

And the VSCode UI initially shows the test as passed, until it updates at the end and marks it failed. It seems like some sort of weird IPC issue? Perhaps something similar is happening on the CI, since the output log there also doesn't show any errors until the end:

============================ 186 passed in 11.62s =============================
Error: Process completed with exit code 1.

Edit: Invoking pytest from the CLI shows tests passing locally (except the archive test, which I still haven't figured out how to configure properly) - so something weird definitely going on with IPC between VSCode and pytest.

Utumno commented 6 months ago

I went ahead and merged the fixups in dev to declutter nightly a bit. Re: pytest, looks like a bug in the IPC of pytest indeed, also in Pycharm on mac there is a SEGFAULT raised in /Applications/PyCharm.app/Contents/plugins/python/helpers/pycharm/_jb_pytest_runner.py right. inside sys.exit(pytest.main(args, plugins_to_load + [Plugin])) where pytest.main returns an exitcode of one (archives test...) - however pycharm displays:

Process finished with exit code 130 (interrupted by signal 2: SIGINT) edit: when running from the debugger - when running normaly I get Process finished with exit code 139 (interrupted by signal 11: SIGSEGV), after I click ok to the fr_CM test (but disabling that test does nothing) - seems like it's crashing when returning the results to pycharm to display them - weird

Infernio commented 4 months ago

I have a feeling that the issue we're hitting here is https://github.com/wxWidgets/Phoenix/issues/2479 Apparently fixed in newer snapshot builds, so we could try grabbing one of those and rehosting it, like we did with 4.2.1 before.

Regardless, that's 313 territory.

Infernio commented 4 months ago

Hah, it was indeed wxPython: https://github.com/wrye-bash/wrye-bash/actions/runs/7415230265/job/20177965150

Utumno commented 4 months ago

Well on 313 - someone has been busy I see 😋

Utumno commented 4 months ago

We should merge this to be able to rebase wip branches to use 3.12 goodies

Infernio commented 4 months ago

Yeah, I can do that tomorrow.

Utumno commented 4 months ago

Currently reading release commit message :coffee: