Closed sonic2kk closed 6 months ago
Thanks!
So what is_valid_steam_install
essentially does is the same we do in Constants, line 31-36, right?
However we will always default to _POSSIBLE_STEAM_ROOTS[0], which at time of writing, is ~/.local/share/Steam.
That may sound a bit oversimplified, but I wonder
Can't we achieve the same by just initially setting _STEAM_ROOT = ''
?
This way, when there is no valid Steam installation, we get the path /compatibilitytools.d/
which should definitively not exist.
If ~/.local/share/Steam
is valid, it will be picked up by the for steam_root in _POSSIBLE_STEAM_ROOTS
loop.
constants.py re-use the is_valid_steam_install function from steamutil.py? I'm not sure if it's a good idea to import a function like this into the constant file, perhaps we'll just have to live with the duplication for now.
That would make the constants file a bit more clean. I'm less worried about the "import a function like this into the constant file" as it is a bit messy anyway, but I'm more worried about circular imports. I think Python can handle them if there are no wildcard imports, but I'd still like to prevent them.
we still gracefully handle this by showing the main menu but with an empty combobox and blank list, but in future there is an opportunity for some slightly improved UX here
Yeah, might be worth adding a label. Something like No launchers detected
.
So what
is_valid_steam_install
essentially does is the same we do in Constants, line 31-36, right?
Yup, exactly!
That may sound a bit oversimplified, but I wonder Can't we achieve the same by just initially setting
_STEAM_ROOT = ''
? This way, when there is no valid Steam installation, we get the path/compatibilitytools.d/
which should definitively not exist.
Huh, that's an excellent point. I wonder why it's set to a default in the first place, when I get home I'll try on main
with this change to see if anything blows up!
I'm more worried about circular imports
I think Python can handle them too, but I wonder if it's a good idea to rely on circular imports in this way (util
uses constants
which in turn will import from util
).
It's probably a stretch but I wonder if there's any PEP guidance on this sort of thing 🤔
Huh, that's an excellent point. I wonder why it's set to a default in the first place, when I get home I'll try on main with this change to see if anything blows up!
Hmm, I probably though an empty string could break something when I implemented that. Shouldn't be the case though.
I wonder if it's a good idea to rely on circular imports
Ideally I guess we would remove the logic from constants.py and completely do it in the util/main logic.
I wonder what would be the best way to do so. What we could do is to add all possible install locations directly to POSSIBLE_INSTALL_LOCATIONS
and then let is_valid_launcher_installation()
check them in available_install_directories()
.
That feels a bit strange as there are multiple entries for the same launcher then, but I would be fine with it. Maybe can you think of a more elegant solution?
Probably the easiest way would be to add something like the following below POSSIBLE_INSTALL_LOCATIONS = ...
(maybe in a more clear way):
POSSIBLE_INSTALL_LOCATIONS += [
{'install_dir': f'{root}/compatibilitytools.d/', 'display_name': 'Steam', 'launcher': 'steam', 'type': 'native', 'icon': 'steam', 'vdf_dir': f'{root}/config'}
for root
in _POSSIBLE_STEAM_ROOTS
]
EDIT: Sniped while writing this comment :-)
Yep, _STEAM_ROOT = ''
resolves the problem. I tested by commenting out these lines, and then setting _STEAM_ROOT = ''
on a separate line. Things worked fine at least in my tests, ProtonUp-Qt still gracefully falls back to the next available launcher (Lutris, in my case).
That feels a bit strange as there are multiple entries for the same launcher then
I wouldn't be against removing the conditional checks in constants.py
and letting that get handled elsewhere. Our loop in available_install_directories
essentially acts to filter "invalid" install paths from POSSIBLE_INSTALL_LOCATIONS
, so I don't think there's any harm in having multiple paths at the same time for the same launcher.
My concern would be that the paths in _POSSIBLE_STEAM_ROOTS
can exist at the same time. For example, on my system I have ~/.steam/root
and ~/.steam/steam
and these are symlinks to ~/.local/share/Steam
. We could end up with duplicate entries for what amounts to the same Steam installation, but the upside would be that we could accomodate a very rare case where a user might intentionally have two or more Steam installations, i.e. one at ~/.local/share/Steam
and one at ~/.steam/root
(where this is not a symlink). Normally this is not possible, but some people have exotic setups with multiple Steam installations, and this approach of storing them all in POSSIBLE_INSTALL_LOCATIONS
would allow us to detect these out of the box.
The problem becomes how to deal with the duplicate paths in an efficient way. I initially thought that we could use sets to remove duplicates, but you can't convert a list of dictionaries to a set.
So, one solution might be to first, store the os.path.realpath
in the example you have above, where root
would become os.path.realpath(root)
. This function will expand symlinks into their actual paths, so if ~/.steam/root
is a symlink it will be expanded to wherever the symlink points to, such as ~/.local/share/Steam
. Then once all paths in POSSIBLE_INSTALL_LOCATIONS
are actual paths and not symlinks, we will handle filtering out duplicate paths in util#available_install_directories
. In this function, we simply append the install path string to a list called available_dirs
(https://github.com/DavidoTek/ProtonUp-Qt/blob/6cf1def3bf1ee9775b2639ed41aad1ff96ae2afa/pupgui2/util.py#L206-L208), so it should be possible to cast this to remove duplicate entries. And since we're expanding the Steam root path to be the actual path, the realpath
, and not a symlink, once we remove duplicate entries from available_dirs
that should mean we only end up with unique Steam installation paths (and 99,999 out of 100,000, there would only be one non-Flatpak non-Snap Steam installation anyway).
Essentially we would end up with something like this:
POSSIBLE_INSTALL_LOCATIONS
stores the expanded paths for all Steam installation folders, so even if symlinks pointing to the same installation exist, we will store them as their actual path. This means we could end up with multiple paths pointing to, say, ~/.local/share/Steam
~/.local/share/Steam
exists, and ~/.steam/steam
and ~/.steam/root
exist both as symlinks pointing to ~/.local/share/Steam
: [{ 'install_dir': '/home/gaben/.local/share/Steam/compatibilitytools.d/' }, { 'install_dir': '/home/gaben/.local/share/Steam/compatibilitytools.d/' }, { 'install_dir': '/home/gaben/.local/share/Steam/compatibilitytools.d/' }]
util#available_install_directories
, we have some way of then filtering out these duplicate entries. Since we only store the path and the path is a string, and since os.path.realpath
should return the same path for two or more symlinks pointing to the same location, we would end up with available_dirs
looking something like this: [ '/home/gaben/.local/share/Steam/compatibilitytools.d/', '/home/gaben/.local/share/Steam/compatibilitytools.d/', '/home/gaben/.local/share/Steam/compatibilitytools.d/' ]
.
return list(set(available_dirs))
. We cast our list available_dirs
to a set
(a data structure that can't have duplicates) which will remove the duplicate install path strings, and then we cast it back again to a list. Alternatively, we could just do if is_valid_launcher_installation(loc) and install_dir not in available_dirs
, although these lines have be a little concerned that that approach may not be sufficient (https://github.com/DavidoTek/ProtonUp-Qt/blob/6cf1def3bf1ee9775b2639ed41aad1ff96ae2afa/pupgui2/util.py#L210-L211) - Having said that, we could always do both for an extra bit of "insurance" :-)(maybe in a more clear way)
We could do it like this maybe:
# This part is from the snippet, untested :-)
POSSIBLE_STEAM_INSTALL_LOCATIONS = [
{'install_dir': f'{root}/compatibilitytools.d/', 'display_name': 'Steam', 'launcher': 'steam', 'type': 'native', 'icon': 'steam', 'vdf_dir': f'{root}/config'}
for root
in _POSSIBLE_STEAM_ROOTS,
]
# Steam Flatpak/Snap
POSSIBLE_STEAM_INSTALL_LOCATIONS += [
{'install_dir': '~/.var/app/com.valvesoftware.Steam/data/Steam/compatibilitytools.d/', 'display_name': 'Steam Flatpak', 'launcher': 'steam', 'type': 'flatpak', 'icon': 'steam', 'vdf_dir': '~/.var/app/com.valvesoftware.Steam/.local/share/Steam/config'},
{'install_dir': '~/snap/steam/common/.steam/root/compatibilitytools.d/', 'display_name': 'Steam Snap', 'launcher': 'steam', 'type': 'snap', 'icon': 'steam', 'vdf_dir': '~/snap/steam/common/.steam/root/config'},
]
# This is our existing `POSSIBLE_INSTALL_LOCATIONS`, just with the Steam paths moved to the above list
POSSIBLE_LAUNCHER_INSTALL_LOCATIONS = [
{'install_dir': '~/.local/share/lutris/runners/wine/', 'display_name': 'Lutris', 'launcher': 'lutris', 'type': 'native', 'icon': 'lutris', 'config_dir': '~/.config/lutris'},
{'install_dir': '~/.var/app/net.lutris.Lutris/data/lutris/runners/wine/', 'display_name': 'Lutris Flatpak', 'launcher': 'lutris', 'type': 'flatpak', 'icon': 'lutris', 'config_dir': '~/.var/app/net.lutris.Lutris/config/lutris'},
{'install_dir': '~/.config/heroic/tools/wine/', 'display_name': 'Heroic Wine', 'launcher': 'heroicwine', 'type': 'native', 'icon': 'heroic'},
{'install_dir': '~/.config/heroic/tools/proton/', 'display_name': 'Heroic Proton', 'launcher': 'heroicproton', 'type': 'native', 'icon': 'heroic'},
{'install_dir': '~/.var/app/com.heroicgameslauncher.hgl/config/heroic/tools/wine/', 'display_name': 'Heroic Wine Flatpak', 'launcher': 'heroicwine', 'type': 'flatpak', 'icon': 'heroic'},
{'install_dir': '~/.var/app/com.heroicgameslauncher.hgl/config/heroic/tools/proton/', 'display_name': 'Heroic Proton Flatpak', 'launcher': 'heroicproton', 'type': 'flatpak', 'icon': 'heroic'},
{'install_dir': '~/.local/share/bottles/runners/', 'display_name': 'Bottles', 'launcher': 'bottles', 'type': 'native', 'icon': 'com.usebottles.bottles'},
{'install_dir': '~/.var/app/com.usebottles.bottles/data/bottles/runners/', 'display_name': 'Bottles Flatpak', 'launcher': 'bottles', 'type': 'flatpak', 'icon': 'com.usebottles.bottles'}
]
# Essentially this means we split the Steam paths and all other paths into their own lists, and then store the combined list as 'POSSIBLE_INSTALL_LOCATIONS'
# Doing it this way means we keep the Steam paths at the beginning of the list
# This approach be very marginally more readable since it means we keep the Steam comprehension bits separated, but really, I don't think it's a huge deal either way :-)
POSSIBLE_INSTALL_LOCATIONS = POSSIBLE_STEAM_INSTALL_LOCATIONS + POSSIBLE_LAUNCHER_INSTALL_LOCATIONS
This is all kind of off-the-top-of-my-head, I haven't sat down and tested this out much yet. It's just one approach I think could work.
In short, I think your idea of storing all the install paths is a fine approach, even if it means multiple paths and possibly multiple invalid/duplicate paths for the same launcher type. It could indirectly allow for the benefit of picking up multiple "Steam roots" (multiple "regular" Steam installs). But we'd probably need to handle removing duplicates, so we don't end up listing all of the existing symlinks on top of the actual Steam root. We have a couple of approaches on how we could handle this, and it means keeping the conditional "filtering" logic isolated to a utility function. But I haven't tested how much of this approach would work or what pitfalls we might encounter. We don't store multiple of the same path for the same launcher type, so I don't know if we would run into any Unforeseen Consequences:tm:. I imagine not if we handle duplicates properly in util#available_install_directories
, but it would need testing to find out.
I can do some testing on this over the weekend to see how having multiple paths in POSSIBLE_INSTALL_LOCATIONS
goes and how filtering out duplicates goes :-)
P.S. - If all else fails, just doing STEAM_ROOT = ''
would fix the problem. But this approach of storing all paths has that very edge-case benefit of allowing us to detect multiple "regular" Steam installations, and allows us to keep is_valid_launcher_installation
as it could be useful in future if we need more strict checks for other launchers.
I think the way I ended up explaining this sounds like we might be adding a lot of complexity, but my comment essentially boils down to incorporating all possible Steam install paths in POSSIBLE_INSTALL_LOCATIONS
and then filtering out invalid paths using util#available_install_directories
. This allows us to avoid any checks for valid installation paths in constants.py
and do it all in util
, avoiding the problem discussed of circular imports, and also the imo slightly cleaner approach of removing conditional logic from constants.py
:-)
Changes are a bit WIP across commits, I will summarize and explain them afterwards :-)
Okay, I was planning to make a couple of other changes, but I think they're better for a separate PR.
Across d0e05ce and 6423f52, the following changes were made:
constants.py
Changes:_STEAM_ROOT
variable altogether as it is no longer needed_POSSIBLE_STEAM_ROOTS
slightly differently, using a list comprehension to join the paths with our HOME_DIR
constant. We also use os.path.realpath
to expand out any symlinks
~/.local/share/Steam
, they become i.e. /home/gaben/.local/share/Steam
- Using a list comprehension just allowed me to do this without having to be repetitive with fstrings or os.path.join
for every path in the list. I put the comprehension on multiple lines for readibilityos.path.realpath
would turn a potential symlink, such as /home/gaben/.steam/root
, into for example '/home/gaben/.local/share/Steam'._POSSIBLE_STEAM_ROOTS
might look something like this, assuming ~/.steam/root
and ~/.steam/steam
are symlinks to ~/.local/share/Steam
(they are on my Arch PC and my Steam Deck):
['~/.local/share/Steam', '~/.steam/root', '~/.steam/steam', '~/.steam/debian-installation']
['/home/gaben/.local/share/Steam', '/home/gaben/.local/share/Steam, '/home/gaben/.local/share/Steam', '/home/gaben/.steam/debian-installation']
~
, and how the symlinked paths got expanded. This might result in duplicates, which we filter out.list(dict.fromkeys(_POSSIBLE_STEAM_ROOTS))
. This turns the values into the list into dictionary keys, and since dictionaries can't have duplicate keys, we filter out the duplicate paths.
['/home/gaben/.local/share/Steam', '/home/gaben/.local/share/Steam, '/home/gaben/.local/share/Steam', '.steam/debian-installation']
['/home/gaben/.local/share/Steam', '/home/gaben/.steam/debian-installation']
list(set())
trick I described in my earlier comment because this does not preserve list order! I was heartbroken tbh, but StackOverflow saved the day.list(set())
, but since we're only working with a handful of strings, I don't think this is a big deal for us.POSSIBLE_INSTALL_LOCATIONS
first with a list of possible Steam install paths. We do this using a dictionary comprehension, setting the install_dir
and vdf_dir
values in the dictionary based on the _STEAM_ROOT
s in _POSSIBLE_STEAM_ROOTS
.
os.path.join
to join the _STEAM_ROOT
paths, just to ensure we always build a valid path._STEAM_ROOT
if it exists, so we don't end up adding paths that exist.POSSIBLE_INSTALL_LOCATIONS
this way, combined with the new way we build _POSSIBLE_STEAM_ROOTS
to store the realpath
for a Steam root and filter out any duplicates, means our POSSIBLE_INSTALL_LOCATIONS
at this point will only have a list of unique Steam install paths that actually exist. However, the installation paths may still be invalid, but we will now use util#is_valid_launcher_installation
to check for a valid installation!HOME_DIR
instead of ~/
, which would make those remaining paths consistent with how the Steam paths are now following this change, but I decided that that would be best left to a follow-up PR. So for now, all other launchers will still use ~
, but in the near-future I'll make a PR to change this so they also use HOME_DIR
.POSSIBLE_INSTALL_LOCATIONS
if they actually exist. Since Steam now only has paths that actually exist, it could make sense to do this for the other launcher paths as well i.e. don't store ~/.var/app/net.lutris.Lutris/data/lutris/runners/wine/
if it doesn't exist. This might be trickier imo, as it is possible that the launcher will be installed, but the folder that we would install tools to does not exist. Probably unlikely and possibly this is an acceptable behaviour, but yeah, something for a different PR. Just a thought I had while working on this :-)util.py
Changes:available_install_directories
, only append install_dir
to available_dirs
if the path isn't already in the list.return available_dirs
, but we could do the same kind of list(dict.fromkeys())
trick to remove any duplicate paths, although none should slip through.The result of these changes in these two commits is that now we don't have the duplicated check for a valid Steam installation directory, and also that if a user has two (non-Flatpak, non-Snap) Steam installations, we can detect this automatically (a very niche use-case imo, but a possible benefit to come out of this!)
And the overall thing that this PR addresses is making sure that we only read from Steam installation directories that have the files we need (config.vdf
and libraryfolders.vdf). The above two changes were mainly "cleanup" to avoid the duplicated check and make the logic a bit cleaner. Now
constants.pydoesn't make any assumptions about whether a Steam root is valid, we just store ones that exist and then check if the actually exist when we check our
available_install_directories`. This also more closely matches what we do for other launchers.
I kept these changes across commits to hopefully make it a bit easier to follow the changes and the rationale behind them. All feedback welcome on this adjusted approach!
Something has went wrong somewhere and I pushed some bad changes, will fix...
It looks like 6423f52e48a982d79eefbb2552e86141435c4c6e is causing the problem. I narrowed the issue down to using os.path.join
for the paths in POSSIBLE_INSTALL_LOCATIONS
, as all of the other changes from that commit work except for that. Removing this os.path.join
from both install_dir
and vdf_dir
fixes the problem.
For some reason, using os.path.join
in this way gives this error: RecursionError: maximum recursion depth exceeded while calling a Python object
Hm, os.path.join
isn't the issue. The issue appears even if I manually modify POSSIBLE_INSTALL_LOCATIONS
to have the Steam path, like this:
POSSIBLE_INSTALL_LOCATIONS = [
{'install_dir': '/home/emma/.local/share/Steam/compatibilitytools.d', 'display_name': 'Steam', 'launcher': 'steam', 'type': 'native', 'icon': 'steam', 'vdf_dir': '/home/emma/.local/share/Steam/config'},
{'install_dir': '~/.var/app/com.valvesoftware.Steam/data/Steam/compatibilitytools.d/', 'display_name': 'Steam Flatpak', 'launcher': 'steam', 'type': 'flatpak', 'icon': 'steam', 'vdf_dir': '~/.var/app/com.valvesoftware.Steam/.local/share/Steam/config'},
{'install_dir': '~/snap/steam/common/.steam/root/compatibilitytools.d/', 'display_name': 'Steam Snap', 'launcher': 'steam', 'type': 'snap', 'icon': 'steam', 'vdf_dir': '~/snap/steam/common/.steam/root/config'},
{'install_dir': '~/.local/share/lutris/runners/wine/', 'display_name': 'Lutris', 'launcher': 'lutris', 'type': 'native', 'icon': 'lutris', 'config_dir': '~/.config/lutris'},
{'install_dir': '~/.var/app/net.lutris.Lutris/data/lutris/runners/wine/', 'display_name': 'Lutris Flatpak', 'launcher': 'lutris', 'type': 'flatpak', 'icon': 'lutris', 'config_dir': '~/.var/app/net.lutris.Lutris/config/lutris'},
{'install_dir': '~/.config/heroic/tools/wine/', 'display_name': 'Heroic Wine', 'launcher': 'heroicwine', 'type': 'native', 'icon': 'heroic'},
{'install_dir': '~/.config/heroic/tools/proton/', 'display_name': 'Heroic Proton', 'launcher': 'heroicproton', 'type': 'native', 'icon': 'heroic'},
{'install_dir': '~/.var/app/com.heroicgameslauncher.hgl/config/heroic/tools/wine/', 'display_name': 'Heroic Wine Flatpak', 'launcher': 'heroicwine', 'type': 'flatpak', 'icon': 'heroic'},
{'install_dir': '~/.var/app/com.heroicgameslauncher.hgl/config/heroic/tools/proton/', 'display_name': 'Heroic Proton Flatpak', 'launcher': 'heroicproton', 'type': 'flatpak', 'icon': 'heroic'},
{'install_dir': '~/.local/share/bottles/runners/', 'display_name': 'Bottles', 'launcher': 'bottles', 'type': 'native', 'icon': 'com.usebottles.bottles'},
{'install_dir': '~/.var/app/com.usebottles.bottles/data/bottles/runners/', 'display_name': 'Bottles Flatpak', 'launcher': 'bottles', 'type': 'flatpak', 'icon': 'com.usebottles.bottles'}
]
Strange issue, I'll keep investigating...
d0e05ce seems to work, so it's something in 6423f52 causing the issue...
It seems like it is os.path.join
causing the problems in 6423f52e48a982d79eefbb2552e86141435c4c6e. I'll push a commit that removes that.
af05168 goes back to using f-strings for POSSIBLE_INSTALL_LOCATIONS
, as using os.path.join
was causing the recursion crash. Now things are working again.
I also removed the os.path.realpath
call from the conditional in the POSSIBLE_INSTALL_LOCATIONS
dictionary comprehensiion, as we do that in _POSSIBLE_STEAM_ROOTS
, so we don't need to do it twice.
Noticed a couple of outdated and unclear comments, I will rework them a bit later
I think with ed709d9 I'm good. If there's anything outstanding here let me know and I'll try fix this up!
So now overall the changes in this PR are:
constants.py
POSSIBLE_INSTALL_LOCATIONS
, i.e. we will store ~/.local/share/Steam
and ~/.steam/root
if they both exist, and are not symlinksis_valid_launcher_installation
to determine if the install paths in POSSIBLE_INSTALL_LOCATIONS
are actually valid based on conditions specific to each launcher
~/.local/share/Steam
and ~/.steam/root
might exist, but ~/.steam/root
might be empty, or contain only empty directories, or otherwise it will be missing the VDF files we need. But it exists, so it's in POSSIBLE_INSTALL_LOCATIONS
. Our function is_valid_launcher_installation
will catch that even though the directory exists, it doesn't contain the VDF files we need, and thus ProtonUp-Qt will not use it, resolving the problem that occurs in #353 - the empty directory structure existed but the VDF files are gone because Steam was uninstalled, but this PR now introduces checks to make sure the VDF files exist.constants.py
we will always default to _POSSIBLE_STEAM_ROOTS[0]
(which is ~/.local/share/Steam
). So if in our loop we never found a directory with the VDF files we wanted, i.e. there was no valid Steam install found, we would still select ~/.local/share/Steam
. The initial fix in this PR re-used the logic from constants.py
and put it in a separate function, and the rest of the work in this PR was to refactor that duplicated logic out of constants.py
so that we only do it in the one new function, is_valid_launcher_installation
.I wonder if it's worth extending our Steam check to determine if the VDF files are > 0 bytes, or some other magic-btye value, like 16 bytes? I dunno if it's too valuable. In most places we just check if a file exists, although there's no reason we couldn't create a general utility function to check if a file is valid, and as part of that check, include an optional parameter to check the size of a file. For example we could create a util function:
def is_file_valid(file_path: str, min_bytes: int = 0):
return os.path.isfile(file_path) and os.path.getsize(file_path) >= min_bytes
There might already be a builtin way to do this or a better way, and introducing this across the codebase would probably be a multi-PR initiative, but if we want to do this kind of check, we could introduce the function here or in a follow-up PR :-)
Thanks for the elaborate explainaiton! Code looks good.
For some reason, using os.path.join in this way gives this error: RecursionError: maximum recursion depth exceeded while calling a Python object It seems like it is os.path.join causing the problems in 6423f52. I'll push a commit that removes that.
Strange... issue says something about ConfigParser.
We don't use the list(set()) trick I described in my earlier comment because this does not preserve list order! I was heartbroken tbh, but StackOverflow saved the day.
Sadly Sets aren't the way to go in most cases. They are neither fast nor deterministic...
You solution list(dict.fromkeys(_POSSIBLE_STEAM_ROOTS))
seems quite good, I think I've seen this approach already in some Python code.
Right now, we only check for (non-Flatpak and non-Snap) Steam, and we determine if it is valid by checking if it contains the VDF data files.
I'm glad that with Flatpak/Snap they "introduced" some standard paths.
I wonder if it's worth extending our Steam check to determine if the VDF files are > 0 bytes, or some other magic-btye value, like 16 bytes?
I'm not sure how likely it is that the file exists but doesn't have any content. I wouldn't do the magic checks as they could change in the future and that would break the app. Let's keep this idea in mind for a future in case a similar issue is reported by someone
Should fix #353.
All mentions to just "Steam" in this issue refer to Steam installed i.e. via package manager, not Steam Flatpak or Steam Snap :-) The problem could still apply to them I think, but I don't have them on-hand to test.
Overview
This PR changes the check in
util#available_install_directories
to be a bit stricter when checking for Steam installs. It now makes sure that the configuration files we need exist before marking a Steam installation as valid.Problem
The core of the problem is that we can mark an invalid Steam installation as available because we only check if the directories that we need exist, not if the actual files we need exist. We need
libraryfolders.vdf
andconfig.vdf
to perform various actions, but inutil#available_install_directories
, we only checkos.path.exists
. This causes the crash in #353, because all the directories exist, but the files do not.The existing check loops through all the paths in
POSSIBLE_INSTALL_LOCATIONS
, which is a list of dictionaries with the installation paths for compatibility tools being under theinstall_dir
key (this distinction is important later). However it only checks ifinstall_dir
exists. In the case of Steam installations, this means it only checks if, say,~/.local/share/Steam/compatibiltytools.d
exists. But that is not sufficient for Steam at least. This is because this path can exist even if Steam itself - and more importantly, the data files we need such asconfig.vdf
- do not. So because the path tocompatibilitytools.d
can exist, we might mark this Steam installation as valid by adding it toavailable_dirs
. But then if we try to use this Steam directory, we'll be unable to read the files we need such asconfig.vdf
because the other data files are missing.We already encountered this problem in a different form a while back, which we solved in #231. However that issue was about selecting the correct Steam installation path, assuming that at least one of them existed. It's a similar idea as the problem was with selecting a Steam installation path that existed without the data files we needed, but this time we need to solve for a case when the path exists, but no Steam installation exists!
In
constants.py
we do a check when selecting our_STEAM_ROOT
path to make sure we only pick one that hasconfig.vdf
andlibraryfolders.vdf
. However we will always default to_POSSIBLE_STEAM_ROOTS[0]
, which at time of writing, is~/.local/share/Steam
. This means even if our check for these VDF files fails inconstants.py
, we'll always default to using~/.local/share/Steam
. And in the case of #353, this path exists but is not valid. In other words, all the directories exist, but we don't have the VDF files we're looking for. Then, inPOSSIBLE_INSTALL_LOCATIONS
, we set our Steam path tof'{_STEAM_ROOT}/compatibilitytools.d/'
. So when we get toutil#available_install_directories
, we only check if this path exists, and if it does, we just mark it as available.Solution
The fix I came up with was to make a util function,
is_valid_launcher_installation
. This takes a dictionary fromPOSSIBLE_INSTALL_LOCATIONS
and then based on the launcher'sdisplay_name
, performs any launcher-specific checks to see if the installation is actually valid, and if no launcher specific checks are required we just fall back to theos.path.exists
check. Right now we only check "regular" Steam to solve for the problem outlined above, but this could be expanded in future.For Steam, I created a Steam utility function creatively named
is_valid_steam_install
. This takes theinstall_path
and performs checks to make sureconfig.vdf
andlibraryfolders.vdf
actually exist. This is the same check we perform inconstants.py
, actually pretty much entirely copied from there, just with the Steam path variable name changed.In short, the previous flow was:
POSSIBLE_INSTALL_LOCATIONS
,install_path
existsNow, the flow has become a bit more involved:
POSSIBLE_INSTALL_LOCATIONS
:is_valid_launcher_installation
to determine if the installation path is valid.is_valid_launcher_installation
,'Steam'
, useis_valid_steam_install
to determine if we have a valid Steam installation by passing itinstall_path
.is_valid_steam_install
.install_dir/..
(since for Steam,install_dir
always points tocompatibilitytools.d
, so we go one directory up to get the base Steam path to check against).config.vdf
andlibraryfolders.vdf
), otherwise return False.os.path.exists(install_path)
as we have no other launcher-specific logic for now.install_path
toavailable_dirs
ifis_valid_launcher_installation
is True.Concerns
I don't know if there's a better way to do this; could (or rather, should)
constants.py
re-use theis_valid_steam_install
function fromsteamutil.py
? I'm not sure if it's a good idea to import a function like this into the constant file, perhaps we'll just have to live with the duplication for now.Other than that, this was just a solution I came up with some days ago at the coffee shop. I tried to make this flexible and extensible, in case we want to extend this to the other Steam flavours or other launchers.
is_valid_steam_install
should work for Steam Flatpak and Steam Snap, but I don't have those installed to test that out, which is why I left them out of this PR. Andis_valid_launcher_installation
should be easily extensible to other launchers as well, possibly with the use of otherlutrisutil
orheroicutil
methods. Wrapping all of the launcher-specific checks inis_valid_launcher_installation
helps keep the check inavailable_install_directories
cleaner imo, and further wrapping the launcher-specific logic in helper methods would keepis_valid_launcher_installation
clean too.But perhaps this is unnecessary decomposition / over-engineering for this problem. I don't know if this is the best approach to take, so please let me know if you'd prefer a different approach! There is possibly opportunity to do stricter checks for the VDF files we need elsewhere in the code, so that we don't try to use them if they don't exist, but I don't necessarily think these approaches are mutually exclusive.
Future Work
In my tests, this didn't break detection of my Steam installation, and if the
steam_path
given tois_valid_steam_install
is invalid, then Steam will not be added to the dropdown. In this case it gracefully defaults to Lutris, but if all paths given tois_valid_launcher_installation
are invalid, we still gracefully handle this by showing the main menu but with an empty combobox and blank list, but in future there is an opportunity for some slightly improved UX here I think (not that I think many users will run into this, I think it's unlikely a user will install or use ProtonUp-Qt with no available launchers installed).Other launchers could be handled in follow-up PRs, I'm not sure if this issue affects them as much but it is not impossible I don't think. I think we're just a bit better at making sure these files exist for other launchers elsewhere, or rather, there is less dependence on these files (from my memory at least) than there is for Steam.
Took me a while to finally get around to this after my comment on the linked issue, but this was the approach I had in mind back then. I hope I explained the problem as I understand it and the solution I came up with well enough. All feedback is welcome on this! There was a lack of coffee while I was working on this but hopefully it's not a total disaster :-)
Thanks!