wrye-bash / wrye-bash

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

Python 3 Upgrade #460

Closed Infernio closed 2 years ago

Infernio commented 4 years ago

Goold old Python 2 is dead in about 3 months as of the time of writing.

What follows is a list of roadblocks:

Things that are not roadblocks:

Useful tracking regexes (make sure to enable *.py mask):

Some resources:

Added to 308, may be addressed sooner - there's lots of factors at play here. There is also no way that I caught every issue we'll encounter on the road to py3. Supersedes: #194 and #404 Follow-ups: #55

Utumno commented 3 years ago

Aand done - I started squashing what seems like manual fixups needed for Bash to run (or for some functionality not to break) in 07f0900cdf7b932ccc70e474ce656c5c06c4636e - things like 808693a6c1b312adbf66f411bd369641c6f86e4e or 25d1007f3aced222991cb5260194f40aab0236bb or c692dfa7f0ba6d43c4b4c41243bbf174f30f5fbd seem like cleanup - and there is the py3 comments cleanup afterwards - but not sure if some commits are fixups or cleanup, I leave this to you.

In the end the manual fixups (and Bump?) should land in the same commit as the 2to3 and then the rest and we are there :)

The VDATA is the only remaining thing that needs some thought

Infernio commented 3 years ago

Put some notes on the commits that are definitely required fixups as well so I remember which to squash later.

Utumno commented 3 years ago

Discovered another bugsie in parsers will add this right on dev after I test a bit more - meanwhile I updated to VDATA3 and can't downgrade - somewhere I have a backup :P - but meanwhile would be nice to add a (silent) settings backup, right by BashSettings.dat - not sure it's feasible at this point I think yes

Infernio commented 3 years ago

Tried it earlier, but it seems like we need some of the settings loaded when making the backup? That would be hard to decouple if it's true.

Edit: and we can't just do it after the settings are loaded, because that would of course back up the already converted settings. Another option may be figuring out a way to force-save on exit instead of in the load method (which is a hack, but I don't understand that code well enough to do it properly).

Utumno commented 3 years ago

Yes it's dreary in there - the whole backup/restore took me several months to fix. Well maybe just renaming the old file with a .VDATA2 at the end might be an option

Utumno commented 3 years ago

Ok pushed to dev including a fixup in restore settings - meanwhile when I (finally) restored the settings I was greeted with

``` Traceback (most recent call last): File "bash\bash.py", line 268, in main _main(opts, wx_locale) File "bash\bash.py", line 444, in _main frame = app.Init() # Link.Frame is set here ! File "bash\basher\__init__.py", line 4275, in Init self.InitData(progress) File "bash\basher\__init__.py", line 4299, in InitData bosh.bsaInfos = bosh.BSAInfos() File "bash\bosh\__init__.py", line 3373, in __init__ super(BSAInfos, self).__init__(dirs[u'mods'], factory=BSAInfo) File "bash\bosh\__init__.py", line 1549, in __init__ self._initDB(dir_) File "bash\bosh\__init__.py", line 1650, in _initDB super(FileInfos, self)._initDB(dir_) File "bash\bosh\__init__.py", line 1544, in _initDB bolt.PickleDict(self.bash_dir.join(u'Table.dat'))) File "bash\bolt.py", line 1634, in __init__ dictFile.load() File "bash\bolt.py", line 1376, in load raise PickleDict.Mold(path) Mold: Your settings in C:\Dropbox\OBLIVION\!\TESIV2\Bash Mod Data\BSA Data\Table.dat come from an ancient Bash version. Please load them in 306 so they are converted to the newer format ```

Well turns out that particular file wasn't in the backup so indeed remained in VDATA3 (note this is not actually Mold but Back from the future - and 306 would not really help)

I am inclined to drop the exception there - we don't want us to crash like that, won't help our reputation :p

maybe a solution is to rename files then display a message after load and add some convert functionality to the Settings > Backup page

Need to rerun the 2to3 as the parsers changes are in the vicinity of izip/xrange - hopefully last time

Infernio commented 3 years ago

I'll rerun 2to3 once I get the chance.

Utumno commented 3 years ago

Thanks Inf I did a rebase of the utumno-xxx-312-parsers which contains the conflict resolution for parsers of the 460 branch

Infernio commented 3 years ago

Thought about the settings a bit more. If we do a conversion via the settings dialog, then we have to block VDATA2 on Python 3, i.e. on 310. The reason for that is that we can't boot with bytestrings in the settings on py3 because of the myriad issues they (correctly) cause on py3, so we can only convert them on load (as we do right now) or reject unconverted settings entirely.

Another difficulty with that proposal would be that we need to think about what to do on exit. Should we write out unconverted settings files as VDATA2? Then we'd need to add some sort of variable to each PickleDict that marks whether or not it's been converted and have save() check that variable to determine whether to write out VDATA2 or VDATA3. And what about entirely new files, e.g. exported BP configs? VDATA2 or VDATA3?

Infernio commented 3 years ago

@Utumno d2f9e800b3469cc64ea0b8ae199da3096daee00e now makes a backup right before saving the converted settings as 'Table-vdata2.dat.bak' (with 'Table' replaced by 'BashSettings' et al as appropriate for other files).

Utumno commented 3 years ago

Thanks Inf! I am a bit burned out here hence the radio silence - now we should maybe drop the raise Mold and do a backup there too instead - then display a dialog to the user saying do not proceed if you don't want your settings eliminated (and call sys.exit if user says yes which will bypass the atexit hook - so will not overwrite the settings). Crashing is really not a good UX.

Also once 309 is out, must remain out for a while - maybe indeed for 310 we should develop a convert settings mechanism that will convert python 2 pickles (VDATA2) to py3 - so not crash, cause asking the user to load settings in 309, hey who would do that? - most people will say it's broken. And however long 309 stays out (anyway we don't want that to be forever) there will be a few people upgrading from 308 (or earlier) to 310 directly.

Infernio commented 3 years ago

From my testing so far, it seems that we can convert the settings from py2+VDATA2 to py3+VDATA3 in one go, as long as it's done on startup. Basically, unconverted settings may never leave PickleDict.load - as long as that is observed, we can convert even on py3.

Utumno commented 3 years ago

Oh then yes problem solved!! We definitely do not want to add any new settings functionality I was under the impression we can't do that (there are pickle incompatibilities between python 2 and 3 pickles). But if we can keep VDATA2 backwards compat in py3 then problem solved!

Infernio commented 3 years ago

I'll do some more testing once 309 is out, but I think it should work if we load the pickles as encoding='bytes' and then manually decode the bytestrings (which is exactly what conv_obj does).

Utumno commented 2 years ago

Well it's impressive and a bit scary - we are ready to push the button. The only remaining thing is to extend VDATA2 compat to py3 - then:

Infernio commented 2 years ago

Actually, one more item:

Infernio commented 2 years ago

I reran 2to3 and rebased my py3 branch (had to leave out the parsers commits because they conflicted and I don't know how to resolve them - affected commits are 55f95579c, 11f569c5f, 1e85d6e44, d84a85e80 and e60db0ce0).

Infernio commented 2 years ago

Note to self, File > Duplicate is broken:

Traceback (most recent call last):
  File "bash\balt.py", line 1770, in __Execute
  File "bash\balt.py", line 824, in _conversation_wrapper
  File "bash\basher\files_links.py", line 112, in Execute
TypeError: askResourcesOk() got multiple values for argument 'bsaAndBlocking'
Utumno commented 2 years ago

Ok pushed 460-py3 - up to c212fed8bd2593ae803ef031f1f4b92f84d1c0c4 is identical to xxx-inf-py3 plus the parser fixup (CsvReader was broken). I suggest we delete the xxx-inf-py3 branches and keep working on that one. I would suggest you squash py3 manual fixups to 35ab07657f9084ef505597ae8d722c64a8ff71b5 so we reduce the branch commits - anyway the py3 commit will have to be a big rush and a push and py3 land is ours. Will pushe the rest of conflicting commits asap but don't need to be merged with the py3 changes

We are almost there Inf :)

Infernio commented 2 years ago

I'm finished with #491, we'll go with PyInstaller. py2exe broke its backwards compatibility really hard and doesn't have any updated tutorials and all the other options lack a onefile option and so spit out a ton of files in a directory.

Building via CI works as well now: https://github.com/wrye-bash/wrye-bash/runs/2803035885

Infernio commented 2 years ago

Oh, and I tested VDATA2 + py2 -> VDATA3 + py3 conversion. Works.

Utumno commented 2 years ago

Turns out those parser commits are already in 460-py3 - could you proceed squashing the manual fixups in are already in 460-py3 so we can build the first py3 nightly tommorowish?

Infernio commented 2 years ago

Will do tomorrow 👍

Infernio commented 2 years ago

I squashed my manual fixups into the manual fixup commit and dropped the PickleDict.save stub commit. Built the first py3 nightly as well. Do we want to keep the other one around for now in case something goes wrong with this version?

Edit: I pushed it to a separate folder for now and made a separate post in the #wip-builds channel on Discord.

Infernio commented 2 years ago

I've been going through and fixing reported tracebacks. This one is weird, probably a wxPython issue:

Traceback (most recent call last):
  File "bash\gui\events.py", line 177, in _post
  File "bash\basher\settings_dialog.py", line 130, in _send_apply
  File "bash\basher\__init__.py", line 3938, in Restart
  File "bash\gui\top_level_windows.py", line 100, in close_win
wx._core.wxAssertionError: C++ assertion "nNew != dynamicEvents.size()" failed at ..\..\src\common\event.cpp(1917) in wxEvtHandler::SearchDynamicEventTable(): 
Traceback (most recent call last):
  File "bash\balt.py", line 1770, in __Execute
  File "bash\basher\misc_links.py", line 358, in Execute
  File "bash\gui\top_level_windows.py", line 187, in display_dialog
  File "bash\gui\top_level_windows.py", line 181, in __exit__
  File "bash\gui\base_components.py", line 326, in destroy_component
RuntimeError: wrapped C/C++ object of type Dialog has been deleted

Happens when you restart WB. May be fixed by downgrading to wx4.1.0, but a minimal reproducer I wrote doesn't cause it: https://gist.github.com/Infernio/bd260755b45122f1df94d3e0c8ebeaf3 So it might be on our side as well.

Utumno commented 2 years ago

Pushed a more squashed version of 460-py3 - no change in content - thanks for fixups - I can't believe we are so near :P

Utumno commented 2 years ago

Tried to debug above - Pycharm opens files for me in the debugger as "outside the project" - does it happen for you (moreover it stops in breakpoints but step into seems broken)? I use the same run configurations as for py2 but change the interpreter in py3

Infernio commented 2 years ago

Debugging works for me, just feels a lot slower than on py2 🤷‍♀️

Infernio commented 2 years ago

Note to self: undo the changes to record_structs.py in 9510bf06b2671a977b33ee2d0942c50050581d56, the getattrs actually are necessary.

On 15. Jun 2021, at 17:22, Utumno @.***> wrote:

 Tried to debug above - Pycharm opens files for me in the debugger as "outside the project" - does it happen for you (moreover it stops in breakpoints but step into seems broken)? I use the same run configurations as for py2 but change the interpreter in py3

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or unsubscribe.

Infernio commented 2 years ago
Traceback (most recent call last):
  File "bash\gui\events.py", line 177, in _post
  File "bash\balt.py", line 824, in _conversation_wrapper
  File "bash\basher\patcher_dialog.py", line 190, in PatchExecute
  File "bash\basher\patcher_dialog.py", line 280, in _save_pbash
  File "bash\mod_files.py", line 293, in safeSave
  File "bash\mod_files.py", line 325, in save
  File "bash\brec\record_groups.py", line 1365, in dump
  File "bash\brec\record_groups.py", line 1149, in getBsbSizes
TypeError: '<' not supported between instances of 'tuple' and 'int'

Reported on Discord, person hasn't responded to me asking whether or not this was on py3 but I think it is.

Edit: this is almost certainly due to py3:

PS C:\Users\Infernio> py -2
Python 2.7.18 (v2.7.18:8d21aa21f2, Apr 20 2020, 13:25:05) [MSC v.1500 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> 1 < (1,)
True
>>> exit()
PS C:\Users\Infernio> py -3
Python 3.9.5 (tags/v3.9.5:0a7dcbd, May  3 2021, 17:27:52) [MSC v.1928 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> 1 < (1,)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: '<' not supported between instances of 'int' and 'tuple'
>>> exit()
Utumno commented 2 years ago

Probably failing on bsbCellBlocks.sort(key=lambda y: y[1].cell.fid) - meaning some fids are int (short fids) and some tuples (long fids)?

Infernio commented 2 years ago

Either that or the one right after it, which would mean the BSBs sometimes have ints as their first element and sometimes tuples. I'll try to reproduce this tomorrow.

Infernio commented 2 years ago

Tried to reproduce this, but I haven't had any success so far.

Utumno commented 2 years ago

I pushed another iteration of 460-py3 squashing a bit and including a couple more commits at the end that should be ok. Should we maybe pull out a 310 beta to catch more regressions?

Current py2 nightly seems ok

Infernio commented 2 years ago

Maybe we could upload them as optional files on the Nexus, I'd rather not replace 309.1 with our super-unstable py3 beta.

Utumno commented 2 years ago

Yes whenever you find some time - is it really super unstable?

On Mon, Jul 5, 2021, 8:10 PM Infernio @.***> wrote:

Maybe we could upload them as optional files on the Nexus, I'd rather not replace 309.1 with our super-unstable py3 beta.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/wrye-bash/wrye-bash/issues/460#issuecomment-874243361, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKNIVYSIZIE7I55EML4R4DTWHRPJANCNFSM4IVAKWCA .

Infernio commented 2 years ago

Still a couple unresolved wxPython crashes with no fix in sight (wxPython repo has been stagnant since January). I do want to try compiling 4.1.0 for py3 (there are no 3.9 wheels for that version on PyPI) to see if that fixes it. Also still has that BSB sorting crash.

On 5. Jul 2021, at 20:34, Utumno @.***> wrote:

 Yes whenever you find some time - is it really super unstable?

On Mon, Jul 5, 2021, 8:10 PM Infernio @.***> wrote:

Maybe we could upload them as optional files on the Nexus, I'd rather not replace 309.1 with our super-unstable py3 beta.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/wrye-bash/wrye-bash/issues/460#issuecomment-874243361, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKNIVYSIZIE7I55EML4R4DTWHRPJANCNFSM4IVAKWCA .

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or unsubscribe.

Infernio commented 2 years ago

Won't have the time to do this for a week or so, sorry :( I'll get to it once I have free time again.

Utumno commented 2 years ago

No prob Inf :)

From my part I dug into that bsb - could not reproduce but I suspect it's the line https://github.com/wrye-bash/wrye-bash/blob/ef8ec7a757c0aac601d449c83ad6762932075320/Mopy/bash/brec/record_groups.py#L1149 as you said - now this means we have mixed Interior and exterior cells somewhere - and what would that sorting mean anyway? I kind of monkey patched in b1c643166a79954f5db51bdaf92cefdea221318f

I do not consider this a showstopper - IIUC we only had a single report with no follow up so 🤷 - the wxPython ones seem more tough but even so...

Infernio commented 2 years ago

Do we still need 460-pre-py3 for anything? It annoys me that tab completion stops at 460-p because of it 😅

Edit: found two more bugs (probably the same bug): On the Installers tab, the column sizes are not getting saved (I resized the Package column, and it got reset on next start) and the size of the Sub-Packages/Plugin Filter window does not get saved either.

Utumno commented 2 years ago

Nuke 460-pre-py3! Good luck with debugging I am on a mac (maybe we don't pass the correct settings dict in?) :P

Infernio commented 2 years ago

460-pre-py3 is gone. I'll have a look at debugging it in a bit :)

Infernio commented 2 years ago

I managed to reproduce the TypeError: '<' not supported between instances of 'MobCell' and 'MobCell' issue, should be easy to fix.

Checklist for what's still needed before a py3 merge:

Infernio commented 2 years ago

Turns out that TypeError traceback is a regression from the monkey patch for the other issue. Fix:

@@ -1145,7 +1145,7 @@ def getBsbSizes(self): ##: This is the _sort_group for MobCells
         # First sort by the CELL FormID, then by the block they belong to
         bsbCellBlocks.sort(key=lambda y: y[1].cell.fid)
         bsbCellBlocks.sort(#FIXME monkey patch for mixed interior/exterior cells
-            key=lambda t: t if type(t[0]) is tuple else ((-1, -1), t))
+            key=lambda t: t[0] if type(t[0]) is tuple else (-1, -1))
         bsb_size = {}
         hsize = RecordHeader.rec_header_size
         totalSize = hsize
Infernio commented 2 years ago

pywin32 v301 has support for SHGetKnownFolderPath now, but if we want to eventually drop pywin32 then there is of course no need to switch to this instead of our ctypes-based implementation.

Utumno commented 2 years ago

Thanks Inf - I was trying to get there on a mac - will post my findings - still won't detect string files for SSE

Add to the checklist the UDR detection uses bosh.mods_metadata.ModCleaner.scan_Many that is no more

Infernio commented 2 years ago

UDR detection uses bosh.mods_metadata.ModCleaner.scan_Many that is no more

Regression from 4b3200e79d7dab45762e26762e3556ad432eb4ed, I forgot to rewrite Mod_ScanDirty to use the new ModHeaderReader methods.

Utumno commented 2 years ago

Turns out that TypeError traceback is a regression from the monkey patch for the other issue. Fix:

@@ -1145,7 +1145,7 @@ def getBsbSizes(self): ##: This is the _sort_group for MobCells
         # First sort by the CELL FormID, then by the block they belong to
         bsbCellBlocks.sort(key=lambda y: y[1].cell.fid)
         bsbCellBlocks.sort(#FIXME monkey patch for mixed interior/exterior cells
-            key=lambda t: t if type(t[0]) is tuple else ((-1, -1), t))
+            key=lambda t: t[0] if type(t[0]) is tuple else (-1, -1))
         bsb_size = {}
         hsize = RecordHeader.rec_header_size
         totalSize = hsize

My idea here was that this t is either a tuple of two ints or two tuples - was wrong

Pushed some drive error handling for linux/mac

Infernio commented 2 years ago

UDR detection is rewritten and works again as of 6c79d10ceef6b17153599d275eefdc624d813e4c.

Utumno commented 2 years ago

Ok re the Cell traceback I managed to reproduce (I loaded the strings - I was missing the ini duh) - it happens for instance on loading the Tamriel WRLD (SSE !) after a b'RNAM' subrecord - adding:

@@ -592,12 +592,17 @@ def getDefault(cls, attr):
     def loadData(self, ins, endPos):
         """Loads data from input stream. Called by load()."""
         loaders = self.__class__.melSet.loaders
         # Load each subrecord
         ins_at_end = ins.atEnd
+        if self._rec_sig == b'WRLD':
+            print()
+        prev_type = None
         while not ins_at_end(endPos, self._rec_sig):
             sub_type, sub_size = unpackSubHeader(ins, self._rec_sig)
+            if sub_type == b'\x18\x00\xef\xff':
+                print()
             try:
                 loaders[sub_type].load_mel(self, ins, sub_type, sub_size,
                     self._rec_sig, sub_type)# *debug_strs
             except KeyError: # loaders[sub_type]
                 # Wrap this error to make it more understandable
@@ -605,10 +610,11 @@ def loadData(self, ins, endPos):
                     f'Unexpected subrecord: ' ##: can this fail? {sub_type!r} ?
                     f'{self.rec_str}.{sub_type.decode("iso-8859-1")}',
                     ins, sub_type, sub_size)
             except Exception as error:
                 self.__handle_load_error(error, ins, sub_type, sub_size)
+            prev_type = sub_type

     def __handle_load_error(self, error, ins, sub_type, sub_size):
         eid = getattr(self, u'eid', u'<<NO EID>>')
         bolt.deprint(u'Error loading %r record and/or subrecord: %08X' %
                      (self.rec_str, self.fid))

as seen we get a sub_type == b'\x18\x00\xef\xff' - a size calculation that goes wrong?

See https://discord.com/channels/537706885965676554/537710082755133460/870386238048313394 for the report - the mod listed there is not needed my patch config is simply:

Bashed Patch, CELL.esp
•  Overview
•  Import Cells

Overview

Date/Time
•  Sun Aug 8 17:03:43 2021
•  Elapsed Time: 0:00:05.635

Active Mods
•  00 Skyrim.esm
•  01 Update.esm
•  02 Dawnguard.esm
•  03 Hearthfires.esm
•  04 Dragonborn.esm

Import Cells
Source Mods
•  Unofficial Skyrim Special Edition Patch.esp

Cells/Worlds Patched
•  Skyrim.esm: 73
•  HearthFires.esm: 6
•  Dragonborn.esm: 11

That's the trickiest one - and the wx - is that last one this here: https://github.com/wrye-bash/wrye-bash/issues/460#issuecomment-860260851

Just to say that I am mostly offline for a while - will try and help out when near a computer :)

BashBugDump.log