util: Check if Steam configuration files exist before marking as available

[sonic2kk]

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 and config.vdf to perform various actions, but in util#available_install_directories, we only check os.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 the install_dir key (this distinction is important later). However it only checks if install_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 as config.vdf - do not. So because the path to compatibilitytools.d can exist, we might mark this Steam installation as valid by adding it to available_dirs. But then if we try to use this Steam directory, we'll be unable to read the files we need such as config.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 has config.vdf and libraryfolders.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 in constants.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, in POSSIBLE_INSTALL_LOCATIONS, we set our Steam path to f'{_STEAM_ROOT}/compatibilitytools.d/'. So when we get to util#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 from POSSIBLE_INSTALL_LOCATIONS and then based on the launcher's display_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 the os.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 the install_path and performs checks to make sure config.vdf and libraryfolders.vdf actually exist. This is the same check we perform in constants.py, actually pretty much entirely copied from there, just with the Steam path variable name changed.

In short, the previous flow was:

• For each location in POSSIBLE_INSTALL_LOCATIONS,
• If install_path exists
• Mark the launcher path as available (i.e. assume this launcher is installed solely based on whether the installation directories exist)

Now, the flow has become a bit more involved:

• For each location in POSSIBLE_INSTALL_LOCATIONS:
• Use is_valid_launcher_installation to determine if the installation path is valid.
  • In is_valid_launcher_installation,
  • If the current launcher is 'Steam', use is_valid_steam_install to determine if we have a valid Steam installation by passing it install_path.
    • In is_valid_steam_install.
    • Set our base path as install_dir/.. (since for Steam, install_dir always points to compatibilitytools.d, so we go one directory up to get the base Steam path to check against).
    • Return True if we have (config.vdf and libraryfolders.vdf), otherwise return False.
  • For all other launchers, default to os.path.exists(install_path) as we have no other launcher-specific logic for now.
• Only add the current install_path to available_dirs if is_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 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.

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. And is_valid_launcher_installation should be easily extensible to other launchers as well, possibly with the use of other lutrisutil or heroicutil methods. Wrapping all of the launcher-specific checks in is_valid_launcher_installation helps keep the check in available_install_directories cleaner imo, and further wrapping the launcher-specific logic in helper methods would keep is_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 to is_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 to is_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!

[DavidoTek]

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.

[sonic2kk]

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 🤔

[DavidoTek]

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
]

[sonic2kk]

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).

ProtonUp-Qt/pupgui2/constants.py

Lines 29 to 36 in 6cf1def

	_STEAM_ROOT = _POSSIBLE_STEAM_ROOTS[0]
	for steam_root in _POSSIBLE_STEAM_ROOTS:
	ct_dir = os.path.join(os.path.expanduser(steam_root), 'config')
	config_vdf = os.path.join(ct_dir, 'config.vdf')
	libraryfolders_vdf = os.path.join(ct_dir, 'libraryfolders.vdf')
	if os.path.exists(config_vdf) and os.path.exists(libraryfolders_vdf):
	_STEAM_ROOT = steam_root
	break

---

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 (

ProtonUp-Qt/pupgui2/util.py

Lines 206 to 208 in 6cf1def

	install_dir = os.path.expanduser(loc['install_dir'])
	if os.path.exists(install_dir):
	available_dirs.append(install_dir)

), 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
  • To visualise, this might be something like this if ~/.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/' }]
• In 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/' ].
  • The simplest way to do this might just be to change this line (

    ProtonUp-Qt/pupgui2/util.py

    Line 212 in 6cf1def

    return available_dirs

    ) to 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 (

    ProtonUp-Qt/pupgui2/util.py

    Lines 210 to 211 in 6cf1def

    	if install_dir and os.path.exists(install_dir):
    	available_dirs.append(install_dir)

    ) - Having said that, we could always do both for an extra bit of "insurance" :-)
  • The set casting thing is possibly not very efficient, but even if all install paths were used, and account for a few extra ones in future if something like PlayOnLinux was added, we're probably never going to be working with more than 20 strings at most for the foreseeable. Rright now with all four Steam paths and allowing for 3 hypothetical extra for PlayOnLinux - a Flatpak, a Snap (not even sure one exists) and an AppImage/system install, we would end up with 17 paths. I don't see this being a huge performance issue :-)
  • Sets are I think more than just a data structure that doesn't allow duplicates, they're a mathsy concept that I don't know a whole lot about (they were covered at uni but it's been a couple years), but for our purposes they would just serve to remove duplicates.
  • Here are the docs specific to sets in Python: https://docs.python.org/3/tutorial/datastructures.html#sets

(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 :-)

[sonic2kk]

Changes are a bit WIP across commits, I will summarize and explain them afterwards :-)

[sonic2kk]

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:

• Removes the valid Steam installation paths logic checks
• Remove setting a _STEAM_ROOT variable altogether as it is no longer needed
• Build _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
  • This way, instead of storing the roots as i.e. ~/.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 readibility
  • os.path.realpath would turn a potential symlink, such as /home/gaben/.steam/root, into for example '/home/gaben/.local/share/Steam'.
  • The above changes essentially mean _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):
    • Before: ['~/.local/share/Steam', '~/.steam/root', '~/.steam/steam', '~/.steam/debian-installation']
    • After: ['/home/gaben/.local/share/Steam', '/home/gaben/.local/share/Steam, '/home/gaben/.local/share/Steam', '/home/gaben/.steam/debian-installation']
  • Note how the paths are all expanded to use the full path instead of ~, and how the symlinked paths got expanded. This might result in duplicates, which we filter out.
• Filter out any duplicates we might end up with above (while preserving the order of the Steam root paths!) using 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.
  • This will transform our list from the example above as follows:
    • Before: ['/home/gaben/.local/share/Steam', '/home/gaben/.local/share/Steam, '/home/gaben/.local/share/Steam', '.steam/debian-installation']
    • After: ['/home/gaben/.local/share/Steam', '/home/gaben/.steam/debian-installation']
  • 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.
  • This trick was picked up from this StackOverflow answer, which does note that this approach is slower than list(set()), but since we're only working with a handful of strings, I don't think this is a big deal for us.
• Build 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_ROOTs in _POSSIBLE_STEAM_ROOTS.
  • This also uses os.path.join to join the _STEAM_ROOT paths, just to ensure we always build a valid path.
  • We also only use a _STEAM_ROOT if it exists, so we don't end up adding paths that exist.
• Building 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!
• Append the remaining launcher list of dictionaries with POSSIBLE_INSTALL_LOCATIONS += [...]`
  • I did consider making all of the paths use 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.
  • There is a possibility for a future refactor where, we could only add paths to 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:

• In available_install_directories, only append install_dir to available_dirs if the path isn't already in the list.
• I didn't add any duplicate removal when we 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 ouravailable_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!

[sonic2kk]

Something has went wrong somewhere and I pushed some bad changes, will fix...

[sonic2kk]

It looks like 6423f52 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

[sonic2kk]

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...

[sonic2kk]

It seems like it is os.path.join causing the problems in 6423f52. I'll push a commit that removes that.

[sonic2kk]

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.

[sonic2kk]

Noticed a couple of outdated and unclear comments, I will rework them a bit later

[sonic2kk]

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:

• Remove the VDF file check from constants.py
• Store all unique (no duplicates) Steam installation directories that actually exist on the filesystem in POSSIBLE_INSTALL_LOCATIONS, i.e. we will store ~/.local/share/Steam and ~/.steam/root if they both exist, and are not symlinks
• Use is_valid_launcher_installation to determine if the install paths in POSSIBLE_INSTALL_LOCATIONS are actually valid based on conditions specific to each launcher
  • 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.
  • For example, ~/.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 Crash if non-flatpak steam is missing #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.
  • The reason our check for VDF files from before was not sufficient is because in 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 :-)

[DavidoTek]

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

[DavidoTek]

Just wondering: With this check not install_dir in available_dirs in place, do we still need list(dict.fromkeys(_POSSIBLE_STEAM_ROOTS)) in constants.py?

[sonic2kk]

In short, no, but it should help catch any case where we might end up with duplicate paths in POSSIBLE_INSTALL_LOCATIONS.

Most probably not for functionality at least for Steam, but for consistency, list(dict.fromkeys(_POSSIBLE_STEAM_ROOTS)) (and the earlier checks to remove non-existent paths from _POSSIBLE_STEAM_ROOTS) should ensure that POSSIBLE_INSTALL_LOCATIONS will only contains Steam install paths that exist on the filesystem. Whether or not they're valid is another question, and that's what our function handles here.

The check might also be good in case other install paths in future could have duplicates. This probably won't ever happen, but it just makes sure at this level that we don't list the same path twice. If we really wanted it to be stricter, we could make sure all paths in available_dirs and install_dir itself are always expanded to their realpath, and also we could filter out duplicates by returning list(dict.fromkeys(available_dirs)) in this function, but I think that's overkill.

For the changes in this PR, the not install_dir in available_dirs condition is not strictly needed, it's just an extra guard.

Also, I guess the comment here is wrong, we don't list duplicate paths.

[DavidoTek]

I guess we could use is_valid_launcher_installation here too.
But let's leave it as is, maybe the user has some very strange Steam setup and wants to add it like this.