Wine-tkg: Refactor Proton-tkg extraction logic

sonic2kk commented 1 year ago

The TKG_EXTRACT_NAME for "Wine-tkg (Vanilla Wine)" in Lutris was set incorrectly as wine_tkg when it should be wine-tkg.

"Wine-tkg (Vanilla Wine)" folders are named like: wine-tkg-staging-fsync-git-WineVer-SHA - With a hyphen
"Wine-tkg (Valve Wine)" folders are named like wine_tkgSHA - With an underscore

(This issue was introduced by a change I made, sorry for that, but I try my best to address any issues ASAP).

This discrepancy results in "Wine-tkg (Valve Proton)" builds being removed when "Wine-tkg (Vanilla Proton)" builds are installed, as it looks for an existing archive to remove I guess.

This does mean that existing Wine-tkg builds for both Valve Proton and Vanilla Proton are removed if another version is updated, in other words it seems we can't have i.e two builds of Wine-tkg (Vanilla Wine) installed, as the existing one gets removed. I'm not sure why this is or if it affects Proton-tkg as well (since these Wine-tkg ctmods are subclasses of TKGCtInstaller). That could be investigated and potentially addressed as part of this PR too.

sonic2kk commented 1 year ago

The issue does not appear to affect standard Proton-tkg for Steam.

However in this block, install_folder is always the same, minus TKG_EXTRACT_NAME.

install_folder = f'{install_dir}{self.TKG_EXTRACT_NAME}' + data['version'].lower()
print(install_folder)

In the terminal for two different versions of Steam Proton-tkg, I for the following output:

/home/emma/.local/share/Steam/compatibilitytools.d/proton_tkgf742de35218ae2276f992ff05012adbb25724bfb
/home/emma/.local/share/Steam/compatibilitytools.d/proton_tkgf742de35218ae2276f992ff05012adbb25724bfb

Despite this, I was able to have two versions of Proton-tkg installed at the same time with ProtonUp-Qt. I would guess that this is because the name of the actual extracted Proton-tkg does not match the output of install_dir, instead the folders were named something like proton_tkg_experimental.bleeding.edge.8.0.42281.20230502.

Need to investigate some more.

sonic2kk commented 1 year ago

I have a suspicion that however the Proton-tkg builds are named has changed, which is resulting in this behaviour. TKG_EXTRACT_NAME = 'proton_tkg' does not seem to be valid anymore for the CI which it is pointing to. The archives and thus folders are named like proton_tkg_experimental.

Also, the data['version'].lower() is not really what we're looking for either when downloading Proton-tkg builds. This returns the SHA for the commit the workflow was based on, and it's right, but that doesn't help us with the filename.

Something that might need to happen here is to extract the archives to a temp dir and then get the name of that extracted temp dir, and then check if the newly extracted dir exists in the compat tool dir. If it does, remove the existing compat tool. And then move the compat tool from the temp dir to the compat tool dir for the current launcher.

This would add more complexity to the extraction logic. It might be best to break some of the logic out into separate helpers/methods if this approach should be taken, and then we can figure out how to best tackle this.

In short, I believe the issue of Tkg builds being removed is because the remove check is not working as expected, and I suspect that this may have been an upstream change rather than a ProtonUp-Qt breakage. install_folder is no longer generating a directory that matches the extracted archive for regular Steam Proton tkg builds, and is not returning the correct hash for builds where the folder name does match, i.e. even if the build does start with proton_tkg or wine_tkg, it will always append the same hash even if we're using an older build, at least as far as I can tell as data['version'].lower() doesn't seem to change.

DavidoTek commented 1 year ago

Wine-tkg (Vanilla Wine)" folders are named like: wine-tkg-staging-fsync-git-WineVer-SHA - With a hyphen "Wine-tkg (Valve Wine)" folders are named like wine_tkgSHA - With an underscore

I had to take a deeper look into it. Interesting... Yeah, we only call Valve builds this way but still assume it's the same for Vanilla Wine. Maybe we should rename both, one wine-tkg-vanilla and the other wine-tkg-valve.

I have a suspicion that however the Proton-tkg builds are named has changed, which is resulting in this behaviour

It looks like the names havent changed, at least not in the last 3 months.

Something that might need to happen here is to extract the archives to a temp dir and then get the name of that extracted temp dir, and then check if the newly extracted dir exists in the compat tool dir. If it does, remove the existing compat tool. And then move the compat tool from the temp dir to the compat tool dir for the current launcher

We already get the name of the archive here I guess: https://github.com/DavidoTek/ProtonUp-Qt/blob/c5d44efa331c478d0a0c93d9b281886e667935b5/pupgui2/resources/ctmods/ctmod_protontkg.py#L229-L234

We extract the archive first to the install_dir and then rename it if the name is usr. Maybe we can do the same if we have Valve Wine (folder isn't called usr but the same as the tar)? We can just cut off the .tar from the archive name and then rename archive_name.replace('.tar', '') to wine-tkg-valve-<sha>.

sonic2kk commented 1 year ago

Sorry, I didn't see this until earlier tonight and spent a little time getting back into the context of this issue. Caught myself back up now!

I did some more digging and will give a bit of a recap on the initial issue and what I found:

Initial Problem

This is what was causing the initial issue here: Since Wine-tkg (Valve Wine) builds had the same extract_name, we were creating the wrong install_folder variable value and thus removing wine_tkg folders. Since the commit hashes could be the same:

if a build of Vanilla Wine and Valve Wine were based on commit 1234,
and we had a Vanilla Wine build called wine_tkg1234 already installed,
when downloading Valve Wine builds, it would build an install_dir set to wine_tkg1234
This means it would incorrectly see our existing Vanilla Wine builds as an already existing installation of the Valve Wine build we just downloaded, and remove it

As an additional note, as far as I am aware, when we're dealing with .tar.zst extraction (Wine-tkg builds), the install_folder doesn't appear to get removed...

Initial fix

This is why using wine-tkg "fixed" it. It isn't a true fix, it's just building a different archive name, which is still incorrect, meaning it still can't accurately remove duplicate Tkg builds except for wine_tkg, because that's the only time the install_dir and actual installed archive name would match. Every other time it's checking for a folder name that would never actually exist.

`install_folder` usage

Here's how we actually handle the archive extraction:

.tar.gz files are just extracted to the current install_dir, we don't set the archive name.
.zip files (which are the GitHub workflow artifacts, such as wine-tkg-build.zip) are extracted to install_folder (the one that tries to mimic an actual archive name incorrectly i.e. proton_tkg<sha>), which is created if it doesn't exist. A .zip is assumed to contain either a .tar.zst (Wine-tkg) or a .tar (Proton-tkg).
- .tar.zst files use glob to get the actual nested archive name from inside of install_folder (we check when a zip is extracted to see if the folder it was extracted into, install_folder, contains a .tar.zst using glob, and that's how we know the real archive name) - So we don't set the archive name here when extracting, we just extract the actual archive based on the name we already have from glob.
- .tar files are extracted pretty much the same as .tar.gz. They're extracted to install_dir (i.e. Lutris runers folder) and then install_folder is removed. We get the name of the archive to extract once again using glob but this time to get a .tar file instead of a .tar.zst.

So install_folder serves two purposes:

To check if the compat tool we're downloading has already been extracted
To serve as a "temp" dir for extracting the .zip files, as in both cases they are extracted to this folder and glob is used to get the real archive names.

At the risk of being too repetitive, the reason using this as a temp folder to begin with is that:

it incorrectly causes two different versions of Wine-tkg builds (i.e. two "Wine-tkg (Vanilla Wine)" bui;lds) to be flagged as duplicates when they are not, because install_folder may be the same for both. The sha between two different builds may not actually change, so data['version'] won't change and so the go-between archive name can be the same even if the actual builds are different. Example workflows with the same SHA:
- https://github.com/Frogging-Family/wine-tkg-git/actions/runs/4963151374
- https://github.com/Frogging-Family/wine-tkg-git/actions/runs/4957334434
Duplicates may not actually be properly detected, as the extracted archive name does not match install_folder. Example: Proton-tkg may not be properly flagged as a duplicate since if os.path.exists(install_folder) will never be true for this (unless the archive was manually renamed I guess).

Solution

I think the solution to this problem is to:

Use a different, more generic name for the install_folder variable, since it makes little sense to try and base it off an incorrect archive name, and the folder isn't actually used for the archive installation, just as a temp dir. We may be able to replace this extract dir entirely and point directly to the temp dir, but I feel it would be a bit safer to extract it into an enclosed directory.
Use the temp dir to extract the .zip files. We should never have a standalone .zip that can be extracted to a folder for a compat tool, .zips should always house additional archives that we need to extract. The current logic with the if/else block implicitly assumes this anyway (if tar.zst else tar is basically how we do it right now). If there is ever a case for Tkg where the .zip would extract to a regular compat tool, we could use an if/elseif/else instead here. But because of how Tkg workflows are set up I don't anticipate this happening.
- afaik GitHub enforces the .zip artifact, and the Tkg build workflow itself produces the archives which are then zipped as an artifact by GitHub. TkG uses these archives to save space, particularly the .tar.zst which is very heavily compressed, so I don't see this changing anytime soon.
Add some duplicate archive detection logic to the actual archive extraction itself. For example before extracting the .tar.zst or .tar, since we have the actual archive name, check if a folder without the extension exists and remove that. If I understood properly, this is what you proposed. It took me a while to circle back to that part :sweat_smile:

install_folder would point to the temp dir instead of {install_dir}, and hopefully the rest of the extraction logic should work since it points to install_dir. We can keep the existing os.path.exists(install_folder) check too as it could help prevent conflicts when extracting for the temp dir.

I also think it would be nice to move this extraction logic or at least part of it into a separate method for generally extracting .tar.zst files, or maybe having a generic extraction helper method which can call various different methods to handle extracting different file types. But that's out of scope for this PR and could be handled separately; it's more of a "maintenance" change than a functionality change.

I'm going to play around and see if the ideas I have for a solution work.

sonic2kk commented 1 year ago

Merged main to get the fixes for fetching Tkg versions :frog:

sonic2kk commented 1 year ago

Okay, https://github.com/DavidoTek/ProtonUp-Qt/pull/238/commits/0b8833a9b9e31a9b8ae3111f7915e9d4d2b49b2b seems to be a disaster :sweat_smile: It removed all my Lutris Wine versions. This PR needs some extra work, I will mark as a draft :-)

sonic2kk commented 1 year ago

Okay, downloading Tkg builds should now work as expected even in edge cases.

Wine-tkg builds should never conflict, duplicate folders will be removed. It is possible for two downloaded workflows to have the same name, but now it will check for an exact folder name match based on what the extracted archive folder name will be
As a result, duplicate detection will now work
The temp_dir is used in place of simply extracting to a go-between folder in the runner folder.

Wine-tkg builds were tested for now with Lutris. Heroic should work, and Proton-tkg should work as well with Steam and Heroic, but I have not tested just yet :-)

That was a fun refactor and a bit more involved than I initially thought when I just renamed the TKG_EXTRACT_NAME var. Aside from being used to name the temp dir (which it doesn't need to be), this variable is actually now completely unused and we could probably get rid of it.

sonic2kk commented 1 year ago

Tested Proton-tkg and Proton-tkg (Vanilla Wine) with Steam and it seemed to extract OK.

Think this is ready for a good review now! Sorry it took so long.

sonic2kk commented 1 year ago

Removed TKG_EXTRACT_NAME as it is not needed anymore :-)

DavidoTek commented 1 year ago

Sorry for the late reply. The fixes look good, I will do some testing. Thanks!

The sha between two different builds may not actually change, so data['version'] won't change

I actually didn't consider this one.

Duplicates may not actually be properly detected, as the extracted archive name does not match install_folder.

Yeah, that seems to have been the original issue.

install_folder would point to the temp dir instead of {install_dir}, and hopefully the rest of the extraction logic should work since it points to install_dir

Sounds good.

Wine-tkg builds should never conflict, duplicate folders will be removed. It is possible for two downloaded workflows to have the same name, but now it will check for an exact folder name match based on what the extracted archive folder name will be

Great!

DavidoTek / ProtonUp-Qt