sonic2kk / steamtinkerlaunch

Linux wrapper tool for use with the Steam client for custom launch options and 3rd party programs
GNU General Public License v3.0
2.03k stars 69 forks source link

Steam Deck dependency enhancement #1111

Open AtomHare opened 1 month ago

AtomHare commented 1 month ago

This draft implements the base discussed in https://github.com/sonic2kk/steamtinkerlaunch/issues/859#issuecomment-2067536075.

Here what's implemented in this draft and some hurdles I noticed that we will need to fix before marking it ready.

Issues:

Note: Also while editing I saw that sometimes there are 4 spaces while in the script it's mainly tabs. Should we make some reformat to fix this later on ? Note 2: This is currently untested

sonic2kk commented 1 month ago

Woah! Thank you for making this PR!

If the Automatic Update checkbox is off, don't do any checks and skip. - not sure where it belongs

We can put this on the Global Menu, but indeed, it is tricky to know the best spot for this... Maybe under "Misc" settings?

https://github.com/sonic2kk/steamtinkerlaunch/blob/master/steamtinkerlaunch#L5664

If the version stored in lastvers is the same as the incoming version, do nothing (this will prevent the constant echo spam on each launch stating that STL needs to be updated) - Not sure if checkSteamDeckSTLUpdated already does it.

From a quick glance at the logic here (https://github.com/sonic2kk/steamtinkerlaunch/pull/1111/files#diff-77c6d4bb1bf7c1988ff5a068c856b6c24e113663f2e61cb3a2c98723c0fafc04R25928) it seems like it would already handle it.

The if check is if [ -f "$(command -v "$DEPCMD")" ] && [ "$CHECKCMD" = "OK" ] && [ "$( checkSteamDeckSTLUpdated )" -eq 0 ]; then - And if all of these are true, then we assume we don't need to install any dependencies.

Then skip the dependency update logic. That should prevent STL constantly trying to update itself.

If coming from an already existing install, we will create the lastvers file ⇒ leading to not update the libs on the first stl update after this PR, maybe we need to create the lastvers after the installation of the libs each time (overwrite the lastvers or creating it for the first time, either way it's a simple bash pipe), and make checkSteamDeckSTLUpdated return 0 if the file doesn't exist either.

I think the easiest solution here is to run clearDeckDeps if there is no lastvers; assume all dependencies are bad and need wiped, and pull the fresh ones. Then everytime a user updates they'll get fresh dependencies.

If I'm missing something here let me know :-)

Is this PR also related with the notification spam of STL at the launch of a game ? (Only in desktop mode)

Yes. The reason it's only in Desktop Mode is because notifiers don't work in a GameScope session.

Already downloaded archives and installed libs may cause issues. Maybe we should clean up the downloaded archives after their successful extraction ? The handling probably needs to be updated too, I'm not very familiar with the codebase.

Yeah, there's going to be a conflict here.

SteamTinkerLaunch explicitly does NOT clean up archives because I want the ability to use it entirely offline eventually. So the idea was to allow users to manually place the archives and have them be extracted.

But with automatically updating the dependencies, I'm not sure how to serve both needs. The complexity around solving this is very off-putting.

Also while editing I saw that sometimes there are 4 spaces while in the script it's mainly tabs. Should we make some reformat to fix this later on ?

Some consistency around this would be good, but making sure it doesn't interrupt places where spaces are used deliberately is important. Unless we go with using spaces instead of tabs rather than the other way around.

I guess it's something I can look into at another time :-)

AtomHare commented 1 month ago

This draft is still UNTESTED.

What's new :

Does everything look good to you ? Before jumping on the ship in order to test it, I wanted to know if "theoretically" all the logic is in place.

sonic2kk commented 1 month ago

I left a couple of comments, but overall this makes sense to me. This is very well implemented from reading and following the code. If might see just how difficult it would be to set up a SteamOS VM to test this, because I'd like to give this the proper testing and review it deserves.

I appreciate being able to follow the commits for the new changes too. The only one I left a comment about really was the clearDeckDeps addition. I think it's fine but I wanted to call out a small concern I had with it, because that might be good to double-check in testing. While writing I kinda went back and forth if it was worth noting but in the end I thought, better to say it than not :-)

Overall, freaking excellent job :+1: I think once we resolve the open comments this is good to start testing. I don't really remember what the problems were with a SteamOS VM that others reported but I guess there's only one way to find out the problem and that's to tinker around with it for myself :smile:

AtomHare commented 1 month ago

Thank you a lot for all your explanations on the different part of the code. I will start testing on an SteamOS VM very soon. I found this guide which I will use as a base to try the installation !

sonic2kk commented 1 month ago

Resolved all the comments, thanks for your explanations.

Also thanks for the guide. I wonder if it works without VirtualBox. It doesn't seem like any of the steps here are specific to it (the Guest Additions can probably be replaced with Spice). Most of it seems like general Arch installation steps with a little extra flavour to set the default boot session and such :-)

lolcatplays commented 4 weeks ago

Thank you a lot for all your explanations on the different part of the code. I will start testing on an SteamOS VM very soon. I found this guide which I will use as a base to try the installation !

I have tested this on my steam deck but so far seems to not be working. I added the yad URL as listed in the master commit as it downloaded a 0B file but I'm still getting the update spam. On the "Main" (bleeding-edge branch) of SteamOS.

steamtinkerlaunch vortex winetricks
Preparing to install SteamTinkerLaunch on Steam Deck
Downloading latest SteamTinkerLaunch

Installing dependency 'innoextract'
Downloading dependency 'innoextract-1.9-8-x86_64.pkg.tar.zst'...
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  214k  100  214k    0     0   564k      0 --:--:-- --:--:-- --:--:--  563k
Successfully downloaded innoextract-1.9-8-x86_64.pkg.tar.zst!
Extracting dependency 'innoextract-1.9-8-x86_64.pkg.tar.zst'...
Successfully extracted 'innoextract-1.9-8-x86_64.pkg.tar.zst' to '/home/deck/stl/deps'
Dependency 'cabextract' already installed, nothing to do.
/home/deck/stl/prefix/steamtinkerlaunch: line 26219: [: : integer expression expected
Dependency 'yad' already installed, nothing to do.

Finished updating SteamTinkerLaunch ('v14.0.20240601-1 (steamdeck-dependency-handling)')!
AtomHare commented 4 weeks ago

Thank you a lot for your testing. I rebased to have the Yad url fix in this PR. it looks like it failed the checkSteamDeckSTLUpdated check there https://github.com/sonic2kk/steamtinkerlaunch/blob/f699a630d47fd96bdf299dec47d011ba65b6b6bf/steamtinkerlaunch#L26217.

Not sure what went wrong, I will test it on my steam deck tonight too and see how I can prevent notification spam.

sonic2kk commented 4 weeks ago

Since we're checking if it's equal to one, perhaps we can simply do this:

function updateSteamDeckLastVers {
    # This function updates the 'lastvers' file after a dependency update if there was a version change
    if checkSteamDeckSTLUpdated ; then
        echo "$PROGVERS" > "$STLSTEAMDECKLASTVERS"
    fi
}

This is how we use, for example, checkSGDbApi and, more recently, haveAnySteamShortcuts. We'd need to rework how updateSteamDeckLastVers is checked in any other places we use it. Also, we'd need to check the return for checkSteamDeckSTLUpdated as well. Returning 0 is a success status, so if STL updated, we'd want checkSteamDeckSTLUpdated to return 0.

The return didn't work because using a subshell with $( func_name ) will only capture the echo/printf/etc, I guess. Which I'm a bit surprised about to be honest.

I'll do a scan of the PR later to see if we need to fix this up in other places. This is my bad for not realising, I'm too used to convenience in other languages 😅 Both in not catching it in this PR and not capturing it when I prototyped this logic before.

(I didn't run the workflow again because afaict the last force-push was just a rebase, so I'll run the workflow again when this is marked ready for review.)

AtomHare commented 4 weeks ago

I just tried your suggestions, we're close to making it work but it looks like we need to make some tweaking !

And I noticed other things: log spam still occurs: "preparing to install SteamTinkerLaunch", "Downloading latest SteamTinkerLaunch", "Add steamtinkerlaunch as compatibility tool", "Finished updating SteamTinkerLaunch". Maybe if the lastvers file is detected, we could remove all of those notifications ?

Recap of the remaining issues :

sonic2kk commented 4 weeks ago

Maybe if the lastvers file is detected, we could remove all of those notifications ?

Yes, we should update notifier logic I think, but I think all the remaining notifier-related "problems" can be done in a separate PR.

So we can remove two notifiers related to automatic updates once that logic is stripped out. If the notifier logic for installing and finished installing is incorrect, that should be fixed to only show on initial installation (i.e. when the script is executed to install STL initially, or when installed from ProtonUp-Qt which just runs ./steamtinkerlaunch as well). Finally, the notifier for adding SteamTinkerLaunch as a compatibility tool should be updated to be inside of the CompatTool function and should only run when the compat tool symlink is added/updated. And all of these changes can go in separate PRs, so no need to stress about them here. You're already working on enough in this PR. If you'd like though you can contribute to improving the notifier in a future PR!

Users that are bothered by the notification spam are probably not users who STL is aimed at, they tend to be the non-developers that bought Steam Decks in my experience. And STL on Steam Deck is mainly an enthusiast/developer curiosity, not something to be used seriously in its current form, hence the wiki's emphasis that SteamTinkerLaunch on SteamOS is in early stages, yet you are one of the few devs who actually stood up to contribute, there aren't many contributors on the desktop side and virtually none for SteamOS, so it's nice to see someone that cares enough to actually improve the situation. So to that end feel free to pick up the notifier work in a future PR, but there is absolutely no obligation on you to do so in this PR (it is probably cleaner in a separate PR anyway).


I will take a look at the if check and your comment above when I have some time, I am curious as to why it isn't working properly myself :-)

EDIT: I left a comment about it, we can discuss the if formula further there. I probably shouldn't have even mentioned it here.

Avoid redownloading the libs on each update if they don't change, (it looks like it redownloads the libs, even though they were downloaded before?)

Hmm, something is going wrong in installDependencyVersionFromURL then? I'm not sure why from looking at the code...

It might be worth checking the steamtinkerlaunch.log file to see if there's any indication of where things are going sideways.

AtomHare commented 3 weeks ago

Thank you for your comments, sadly I won't be able to look and tinker with the code until Monday but will as soon as I'm available!

I will also make a separate PR for the steam deck notifications.

And I may look into a debug var that enables the steam deck behavior to make it easier to test. I don't need it to run the libs but mainly to test the codes/logic! (I'll probably add a dev check to prevent people wanting the steam deck behavior in desktop mode)

sonic2kk commented 3 weeks ago

No worries, you can work on this whenever you have time. All your work is appreciated thus far as well! :-)

sonic2kk commented 3 weeks ago

I went ahead and linked this PR to #859, as this is the last thing discussed in that issue related to Steam Deck improvements.

You've expressed interest in doing more work, and you are absolutely welcome to contribute any improvements in the future. But as far as that issue goes, this PR would implement all that was left specific to that issue. So once this is merged, that issue would be done, even if there is more work you or anyone else may want to do!

Of course that doesn't mean there is any rush on this either. I've just been doing a little "issue cleanup" and this was part of it. You can work on this whenever you have time and motivation. If we hit any major blockers in a worst-case and we can't merge this (which I don't foresee happening, imo this is already most of the way there, once again fantastic work on your part), linking the issues gives a good paper-trail on what went on during development. Development transparency is very important to me with this project and giving everyone insight into what decisions were made, where discussion took place, etc is something I strive for (even if I don't always live up to that in the way I want to).

So yeah, please don't feel any additional pressure here. Just some repo management :smile:

AtomHare commented 1 week ago

No issue at all, thanks a lot for your help !

I've tested the PR and it looks ready for review !

sonic2kk commented 1 week ago

Thank you! I will look into this PR. I am not sure how much time I will have next week as it is unusually busy but I'm grateful for all your work.

AtomHare commented 1 week ago

You're welcome, thank you for your continuous help with the PR ! Yeah, no problem, there's no hurry ! I'll try to make some followup PRs to continue improving the Steam Deck and overall experience too

sonic2kk commented 1 day ago

Just a heads up; As per #1135, it seems some dependencies are now broken on SteamOS. That issue is specifically where Game Mode can't find extract archives properly. If you can replicate the issue it may be a bug in SteamOS that could block this PR for a bit. 😦

That issue turned out to be the innoextract version needing bumped again for SteamOS 3.6 Preview which updates the Arch snapshot. False alarm! 😄