Closed sonic2kk closed 11 months ago
Thanks!
I think it is more straightforward to just rename the folder after extraction is finished, though this does mean the folder name will not be updated until extraction completes (could be about 10 seconds or more).
I think that is fine. There probably is some way to do that with tarfile but is seems the solution you choose is way more clean.
Considerations
Good idea. Having that would probably also simplify adding new launchers or allow for different ways of detecting the launcher in case we would need that in the future.
I'll take a look at adding a util function for getting the Launcher type from the install_dir.
What's our minimum Python version these days? If it's >= 3.10 we could use a match
for this, but a standard if/else is fine if not :-)
Okay, I created a new util function called get_launcher_from_installdir
. For safety it uses str.lower
on the install_dir
given, which is mainly helpful when matching Steam, since the path have an upper or lowercase Steam
, for example .local/share/Steam
or .root/steam
(there are other lowercase paths as well).
Usage is like this, but I need to do further testing on it so this is just an example:
# Returns a new Enum, DataStructure#Launcher
launcher = get_launcher_from_installdir(install_dir) # i.e. /home/gaben/.local/share/lutris/runners
if launcher == Launcher.STEAM:
print('No good, it's full of Steam!')
elif launcher == Launcher.LUTRIS:
print('Remember winesteam?')
elif launcher == Launcher.HEROIC:
print('When did I get so many GOG games...')
elif launcher == Launcher.BOTTLES:
print('Tamika approves')
We could easily extend this in future for, say, PlayOnLinux by adding another launcher to the Launcher
enum and then adding a check.
A match statement might be slightly cleaner in my personal opinion, but I'm not sure if they're faster or anything, so it isn't a huge deal. If we could use it though it might be a nice little bit of syntax sugar :-)
I'll take a look at adding a util function for getting the Launcher type from the install_dir.
Great, thanks.
What's our minimum Python version these days? If it's >= 3.10 we could use a match for this, but a standard if/else is fine if not :-) A match statement might be slightly cleaner in my personal opinion, but I'm not sure if they're faster or anything, so it isn't a huge deal. If we could use it though it might be a nice little bit of syntax sugar :-)
I agree that a match
would be cleaner, but...
We're currently still bound to Python 3.8 due to the AppImage build which uses packages from Ubuntu 20.04 (Focal).
It should be possible to update this, but I would prefer to stay on a slightly older version for compatibility reason, at least as long as it is officially supported by Canonical (non-ESM support goes until April 2025). Though I don't think anyone who runs games will be using an old operating system (most likely non-LTS or rolling release).
EDIT: I just checked, Ubuntu 22.04 (Jammy) is 18 months old by now, this would be an option and it also includes Python 3.10.
-> I would leave it as is right now. If we update the AppImage to also use Python 3.10 we could do a general refactoring of the code I guess.
For safety it uses str.lower on the install_dir given
Ah, that makes sense.
Usage is like this, but I need to do further testing on it so this is just an example:
Looks good.
We could easily extend this in future
Good, that saves us quite some manual work then
We're currently still bound to Python 3.8 due to the AppImage build which uses packages from Ubuntu 20.04 (Focal). It should be possible to update this, but I would prefer to stay on a slightly older version for compatibility reason, at least as long as it is officially supported by Canonical (non-ESM support goes until April 2025). Though I don't think anyone who runs games will be using an old operating system (most likely non-LTS or rolling release). EDIT: I just checked, Ubuntu 22.04 (Jammy) is 18 months old by now, this would be an option and it also includes Python 3.10. -> I would leave it as is right now. If we update the AppImage to also use Python 3.10 we could do a general refactoring of the code I guess.
I think leaving as-is is fine for now, we can revisit in future like you said. I was just curious :-) Thanks for explaining
I should note that I'm testing this right now, but having weird difficulties with GitHub when trying to download (also happens when trying to leave comments and push commits), but in the couple of tests I've done so far, this PR appears to still work as expected with the new refactor :-)
I will also need to test the ctmods I updated to use the new util function (DXVK, D8VK, vkd3d)
Tested DXVK and vkd3d-proton, both work as expected.
D8VK is broken as the main branch has not had any commits in so long that all artifacts have expired for that branch (https://github.com/AlpyneDreams/d8vk/actions?query=branch%3Amain). Removing the check in the D8VK ctmod's fetch_releases
for artifact['expired']
, ProtonUp-Qt will list all of the workflows, but it can't download them, as the artifact has expired and when we pass it to nightly.link, it tells us as much.
D8VK does have releases now that we could fall back to, but if the main branch got a commit right after I posted this comment, it would start working again. There is a PR to merge D8VK into DXVK (doitsujin/dxvk#3411), but it's a work-in-progress at the moment, so it may be worth adding a "normal" D8VK ctmod (since the current one is "nightly").
I haven't had the time yet (hence the lack of PRs here :sweat_smile:) but at some point I would like to make a "common" DXVK ctmod similar to what we have for Proton-tkg. Until then we could probably just have a separate ctmod for a non-nightly D8VK.
That is, entirely, a separate issue to this PR though, as this behaviour is also on master :-)
I made two changes to the code:
Launcher
enum (UNKNOWN = 0
) to match the other enumsor original_name
in get_launcher_extract_dirname
of Wine-GELooks very good and will be merged. Thanks.
D8VK is broken as the main branch has not had any commits in so long that all artifacts have expired for that branch
Right, that could be fixed if the merger of D8VK is delayed.
D8VK does have releases now that we could fall back to, but if the main branch got a commit right after I posted this comment, it would start working again
I haven't had the time yet (hence the lack of PRs here 😅) but at some point I would like to make a "common" DXVK ctmod similar to what we have for Proton-tkg. Until then we could probably just have a separate ctmod for a non-nightly D8VK.
I see, currently only DXVK Async is based on the DXVK ctmod. Seems like D8VK is using the same release format as DXVK so this seems quite trivial to do. There's no hurry :smile:
One thing I noticed is that the function get_launcher_from_installdir
is kinda redundant as we already have the function get_install_location_from_directory_name
which returns a dict with an attribute "launcher" that contains a string with the launcher name. We also lose the ability remember custom set install directories here.
I noticed that when testing this PR and it wouldn't work when I set a random directory as a custom Lutris install directory (Lutris is currently not installed).
This would be something for a future PR though. We probably need to refractor get_install_location_from_directory_name
and maybe change the CtInstaller parameter install_dir
to install_loc
(dict/struct).
Thanks for the updates! They make sense.
I see, currently only DXVK Async is based on the DXVK ctmod. Seems like D8VK is using the same release format as DXVK so this seems quite trivial to do. There's no hurry
Yup and that's due for a refactor with #302, though it still uses the subclass format. I structured DXVK in that PR to be compatible with both GitHub and GitLab, and tried to keep in mind nightly.link
builds when structuring the function to fetch and filter releases. It might not be perfect, but I did keep in mind making all of this as flexible as possible to allow for more of these common ctmods in a way that is still maintainable, without a whole bunch of conditional logic.
It should be pretty straightforward still to make the subclass for D8VK, as that PR's refactor is structured in such a way that the implementation of how we download should be irrelevant to the subclasses. I'll probably look at it after that PR though in case there are further refactors needed to how we handle sessions.
I am not sure yet how we will handle DXVK Nightly, perhaps we can just override some methods, I haven't looked too deeply yet :-)
This would be something for a future PR though. We probably need to refractor
get_install_location_from_directory_name
and maybe change the CtInstaller parameter install_dir to install_loc (dict/struct).
This makes sense, that way ctmods are more aware of launcher information. Then we can check on the launcher
name instead of the path, which is much better imo. I wonder if we could then set the launcher
type to the Launcher
enum, though that might require a pretty significant refactor depending on how it's used, I recall a few places in the games list I think off the top of my head...
Should be feasible either way, just a bit of work, though I think using install_loc
in ctmods overall is cleaner than using install_dir
:-)
Thanks!
Implements #294.
Overview
This adds an extra step to Wine-GE installation which renames the extraction folder to be consistent with what the current launcher would name it.
Screenshots for Lutris and Heroic respectively:
Implementation
We currently keep the name of the top-level archive (
lutris-GE-ProtonX-YY-x86_64
), but Lutris and Heroic do not use this name. For Lutris specifically, this apparently breaks auto-updating. For Heroic, I figured if we're making a change to be consistent for one launcher (Lutris) we should do it for all of them :-)The launcher check is similar to what we use for DXVK (and elsewhere I believe), essentially checking for strong clues in the path to infer the launcher.
I am not sure what Bottles uses, as I don't use it, but if we need to make a change there it should be easy enough to add.
I could not see a straightforward way to set the extract name for the top-level folder with
tarfile
, it seems the general idea for that is read and extract each file into a specific folder, which I thought was overkill. I think it is more straightforward to just rename the folder after extraction is finished, though this does mean the folder name will not be updated until extraction completes (could be about 10 seconds or more). I doubt any users would see this, though.Considerations
I'm thinking since we've re-used this snippet quite a bit that it could be its own utility function, probably taking
install_dir
and returning an enum value so we could do a check likeif get_launcher_from_install_dir(install_dir) == Launchers.HEROIC
. We do some checks like this already, checking on strings withinstall_loc.get('launcher')
, but in ctmods we just pass downinstall_dir
. Though we could allow theseinstall_loc
checks to use the enum as well, it may be slightly nicer than checking directly on the string :slightly_smiling_face:Not really a big deal either way though, just a thought I had during development that I wanted to share.
Thanks!