Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

appinfo.vdf Magic Changed on Steam Client Beta (June 2024) - Needs Resolved Upstream in ValvePython/steam #415

Closed
sonic2kk opened this issue Jun 28, 2024 · 7 comments · Fixed by #426
Labels
bug Something isn't working

Comments

@sonic2kk
Copy link
Contributor

sonic2kk commented Jun 28, 2024

Relevant upstream PRs:


Please fill out following when reporting a new bug:

Describe the bug
The upstream magic for appinfo.vdf changed again on the latest Steam Client Beta, meaning parse_appinfo from ValvePython/steam is broken. This happened before (ValvePython/steam#418) and this new issue has already been reported upstream (ValvePython/steam#462).

I am making this issue for awareness for anyone who runs into this, and also for us to track this so that we can update the Steam dependency once a version with a fix is available.

To Reproduce
Steps to reproduce the behavior:

  1. Open ProtonUp-Qt while on the latest Steam Client Beta
  2. With Steam selected, it will open significantly faster than normal
  3. Open the Game List
  4. Only Steam Shortcuts will be listed

Expected behavior
ProtonUp-Qt should be able to parse the appinfo.vdf, but cannot pending an upstream fix.

Screenshots
N/A.

Desktop (please complete the following information):

  • Platform: PC
  • System: Arch Linux
  • Version: 2.9.2, but hapepens on any version
  • How did you install ProtonUp-Qt?: Discover

Additional context
Took a stab at trying to submit a patch for this upstream, but no guarantees my fix will be the one that gets merged as it is still in progress 😄

Terminal output
The parse_appinfo gives the following output because it cannot parse the magic in the file.

Error getting ctool map from appinfo.vdf: Invalid magic, got b')DV\x07'
Error updating SteamApp info from appinfo.vdf: Invalid magic, got b')DV\x07'

The fix is not as straightforward as adding the magic to the file though, as we need to skip past some parts of the file. And then there are some issues around parsing the binary VDF information in this file under data.

@sonic2kk sonic2kk added the bug Something isn't working label Jun 28, 2024
@sonic2kk sonic2kk changed the title appinfo.vdf magic changed again (needs resolved upstream in ValvePython/steam) appinfo.vdf Magic Changed on Steam Client Beta (June 2024) - Needs Resolved Upstream in ValvePython/steam Jun 28, 2024
@sonic2kk
Copy link
Contributor Author

sonic2kk commented Jun 28, 2024

A small effort by me and a much larger effort by the Protontricks maintainer have created a working implementation however changes are required for both ValvePython/steam and ValvePython/vdf. So we will need to bump both versions once updates are in place.

@sonic2kk
Copy link
Contributor Author

sonic2kk commented Jun 29, 2024

It seems like the only thing we depend on from the steam dependency is the parse_appinfo function. We could implement this function ourselves in a thirdparty/steam/utils/appcache folder and remove the Steam dependency altogether. This would also allow us to potentially make some performance-speciifc tweaks, something I have been looking at recently but have not made much headway with - the generator is the biggest bottleneck when opening with/selecting the Steam launcher. We can optimise a little bit by searching directly for the Steam Play tools AppID, roughly halving the time it takes iirc, and perhaps further optimisations could be made.

This would allow us to cut one of the larger dependencies we have, the steam dependency is approximately 10mb and it has the following dependencies:

  • cachetools (very 60kb, negligable) - Only needed by steam dependency.
  • pycrytpodomex (Cryptodome dependency is about 7mb, somewhat significant) - Only needed by steam dependency.
  • requests - We use requests anyway, so not relevant
  • six - Could not find this package in site-packages, maybe not used anymore?
  • vdf - We use vdf anyway, so not relevant

So by implementing the Steam dependency ourselves we can save about 17mb - 10mb from the dependency itself, and 7 from removing the dependency on Cryptodome.

The upstream license of steam is compatible with ProtonUp-Qt's as it is less restrictive, so there shouldn't be any issues adding this ourselves from that standpoint.


Pros:

  • Saves a nice amount of space, the AppImage could go from 91mb -> 74mb and the Flatpak could go from 108mb -> 91mb.
  • We get to make customisations ourselves and generally debloat since we only use a tiny part of a big library - Not saying the steam dependency shouldn't be the size that it is but that we're pulling in a lot for just one function.

Cons:

  • We will have to apply fixes ourselves which can be a benefit of upstream takes a while, but a drawback if we make significant changes and have to reconcile them.
  • Raises questions about where we should and shouldn't do this, we could be opening a can of worms by doing this for one dependency.

Personally I think we should only consider this if:

  1. Upstream does not merge the changes fast enough - unsure how actively maintained it is, there haven't been many commits but maybe that's because it's stable rather than anything else. But if we want to do a release and there is no fix in place it may be something to consider, mainly because we only use this part of the Steam dependency.
  2. We think we could really get a lot out of owning this dependency ourselves in terms of optimisations and other patches.

One other thing to note is that the Steam parse_appinfo changes do depend on changes to upstream ValvePython/vdf, so we would have to partially re-implement those modifications too, which could become messy. Hopefully upstream merges the changes, various other programs depend on this library and updates to it so even if they do not, hopefully someone forks it. We could also potentially fork it ourselves and use that dependency if push came to shove!

@sonic2kk
Copy link
Contributor Author

A fork of the vdf and steam projects have been created by SolsticeGameStudios.

Even if we want to continue with steam as a dependency we would need to use a patched version of the vdf package, but it has been over a month since the upstream PRs were open, and even longer since there was any activity from the maintainer at all.

So one possible route is to point to a patched VDF dependency from the fork above, or alternatively fork it ourselves and apply the patches as we see fit.

The VDF dependency is probably the most useful to keep around, but I would like to voice that I think we should consider the steam dependency separately. We are barely using any of it. Maybe we should just take the parse_appinfo function from it and maintain that ourselves, cutting out the steam dependency entirely, and then point to an updated vdf dependency, either the one I linked to or a fork specifically maintained by us?

In other words, there are forks available for the two dependcies as of now with the necessary patches. We can either use both of them or take this opportunity to cut out the steam dependency and maintain the parse_appinfo function ourselves since we only use that function and are probably not likely to use more. Since there has been little activity upstream I don't think waiting for the patches to be merged upstream is viable for now. We can either use an existing fork I linked to or we can fork it ourselves (if we cut out steam then we only need to maintain vdf).

But maybe I'm just being impatient 🙂

@DavidoTek
Copy link
Owner

Thanks for reporting.
Sorry for the delayed response.

The fix is not as straightforward as adding the magic to the file though, as we need to skip past some parts of the file. And then there are some issues around parsing the binary VDF information in this file under data.

That isn't good. My first idea was just to ignore the magic and hope they don't break backward compatibility, but apparently, that won't work.

A small effort by me and a much larger effort by the Protontricks maintainer have created a working implementation however changes are required for both ValvePython/steam and ValvePython/vdf

I saw that commit, great work!

[...] I think we should consider the steam dependency separately. We are barely using any of it. Maybe we should just take the parse_appinfo function from it and maintain that ourselves, cutting out the steam dependency entirely, and then point to an updated vdf dependency, either the one I linked to or a fork specifically maintained by us?

I see the advantages of creating our own fork/adding the parser just to our code. As you pointed out, this can speed up the process of adding patches related to the parse_appinfo function. On the other hand, we would maintain it completely on our own. Either we will be the sole maintainer of the steam dependency or we might get two incompatible forks, getting out of sync. Similar problems arise with just copying the file from the library to thirdparty/steam/utils/appcache.

I'm wondering whether the fork by Solstice Game Studios is here to stay or if it is a temporary solution. We probably need to ask @WinterPhoenix for that.
If they/the community keeps that fork maintained, I think we can use it and create our pull requests there if needed. Maybe it will even find its way onto pypi? 😄

Regarding the memory and performance: I think saving 20mb isn't worth the effort if we can have such a solution. Also, we can push our performance improvements to the "new upstream"

Adding it to the requirements.txt should be easy:

steam @ git+https://github.com/solsticegamestudios/steam@master

@WinterPhoenix
Copy link

WinterPhoenix commented Jul 27, 2024

We’ve had the question proposed a couple of times now. We’re okay maintaining it for the foreseeable future.

We rely on it for GModCEFCodecFix, that project isn’t going away anytime soon, and it’s the best supported/featured Python library for working with Steam currently, so there’s no appetite to replace it with something else.

Do be warned that I fully intend to drop Python 2 support for it though, and probably earlier 3.x versions as well. We’re not in the business of supporting well-and-truly EOL’d stuff.

@DavidoTek
Copy link
Owner

DavidoTek commented Jul 27, 2024

Thanks for the quick response. That is good to know.


I will create a PR that adds both the steam and vdf from Solstice Game Studios to the requirements.txt.


PS: I think the docstring in the appcache.py are unclear. When I run the code without specifying mapper=dict, I get an error:

>>> header, apps = parse_appinfo(open('/d/Steam/appcache/appinfo.vdf', 'rb'))
>>> header
{'magic': b'(DV\x07', 'universe': 1}
>>> next(apps)
b'(DV\x07'
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "[...]/site-packages/steam/utils/appcache.py", line 138, in apps_iter
    app['data'] = binary_load(fp, key_table=key_table, mapper=mapper)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "[...]/site-packages/vdf/__init__.py", line 345, in binary_load
    if not issubclass(mapper, Mapping):
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen abc>", line 123, in __subclasscheck__
TypeError: issubclass() arg 1 must be a class

---

>>> header, apps = parse_appinfo(open('/d/Steam/appcache/appinfo.vdf', 'rb'), mapper=dict)
>>> header
{'magic': b'(DV\x07', 'universe': 1}
>>> next(apps)
b'(DV\x07'
{'appid': 5, [...]}

@sonic2kk
Copy link
Contributor Author

On the other hand, we would maintain it completely on our own. Either we will be the sole maintainer of the steam dependency or we might get two incompatible forks

That's fair, right now at least it would be a case of just copying the parse_appinfo function and nothing else from the steam dependency, but in future we might have to extend the list of things we copy if parse_appinfo ever expanded. Although I guess in that case we could always fall back?


We have the updated steam dependency now so it's all good either way 😄 I run ProtonUp-Qt from my fork (purely a development fork) so this fix is super highly appreciated from me, I verified that #426 fixes the issue for me and I am absolutely delighted 😄

Many thanks @WinterPhoenix for maintaining these forks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants