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

Steam Deck dependency enhancement #1111

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

AtomHare
Copy link

@AtomHare AtomHare commented May 20, 2024

This draft implements the base discussed in #859 (comment).

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

  • 1. STL starts up on Steam Deck.
  • 2. If the Automatic Update checkbox is off, don't do any checks and skip.
  • 3. Otherwise, we check if SteamTinkerLaunch has updated by comparing the value in lastvers with that of the incoming version (we can get the incoming version from PROGVERS) - managed by checkSteamDeckSTLUpdated
  • 4. 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)
  • 5. If the version in lastvers does not match the incoming version string, we should try to update our dependencies, as the newer STL version may specify a newer version of a given dependency
  • 6. Once everything is complete, update the version string in lastvers so that we know we already checked for dependency updates for this version. - See in issues, I think we could force the overwrite each time the libs are updated

Issues:

  • 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.
  • Is this PR also related with the notification spam of STL at the launch of a game ? (Only in desktop mode)
  • 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.

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
Copy link
Owner

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.

  • `-f "$(command -v "$DEPCMD")"`` - If the dependency file exists.
  • "$CHECKCMD" = "OK" - If the dependency can actually be ran.
  • "$( checkSteamDeckSTLUpdated )" -eq 0 - If the SteamTinkerLaunch version has not changed since the last run.

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 AtomHare force-pushed the steamdeck-dependency-handling branch from f52ca74 to 26b7f9c Compare June 1, 2024 12:21
@AtomHare
Copy link
Author

AtomHare commented Jun 1, 2024

This draft is still UNTESTED.

What's new :

  • Clear deps on the first run if the lastvers file doesn't exist, fix any weird issues in the case of an existing install
  • Added the autoupdater settings - if you have any naming recommendation feel free to tell me I will correct it
  • Set the PROGVERS to differentiate it from the master branch

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.

steamtinkerlaunch Outdated Show resolved Hide resolved
steamtinkerlaunch Outdated Show resolved Hide resolved
steamtinkerlaunch Outdated Show resolved Hide resolved
steamtinkerlaunch Outdated Show resolved Hide resolved
@sonic2kk
Copy link
Owner

sonic2kk commented Jun 1, 2024

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 👍 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 😄

@AtomHare
Copy link
Author

AtomHare commented Jun 2, 2024

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
Copy link
Owner

sonic2kk commented Jun 2, 2024

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

lang/english.txt Outdated Show resolved Hide resolved
@lolcatplays
Copy link

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 AtomHare force-pushed the steamdeck-dependency-handling branch from e3bc04f to f699a63 Compare June 5, 2024 06:03
@AtomHare
Copy link
Author

AtomHare commented Jun 5, 2024

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
Copy link
Owner

sonic2kk commented Jun 5, 2024

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

Copy link
Owner

@sonic2kk sonic2kk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should've caught this before, very sorry I only realised these now 😓

Give these a try locally and see if they pan out. If so push them up and the update checks should be fine :-)

steamtinkerlaunch Outdated Show resolved Hide resolved
steamtinkerlaunch Outdated Show resolved Hide resolved
steamtinkerlaunch Outdated Show resolved Hide resolved
steamtinkerlaunch Outdated Show resolved Hide resolved
@AtomHare
Copy link
Author

AtomHare commented Jun 6, 2024

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 :

  • Tweak the if formula
  • Prevent the log spam -> in a separate PR
  • 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?)

@sonic2kk
Copy link
Owner

sonic2kk commented Jun 6, 2024

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.

  • Notifier for installing SteamTinkerLaunch, including finishing installation
    • We should keep this, because when a user double-clicks the script to install STL we will want to show this visual feedback. If it is not running only during installation, we should fix that, but it can go in a separate PR, nothing to worry about here :-)
  • Notifier for updating SteamTinkerLaunch, including finishing updating
    • We can remove the notifiers around preparing to install SteamTinkerLaunch, as the auto-update logic (which doesn't really work) will be removed shortly. Users can just manually update STL, and when they do, this PR will update the dependencies automatically!
  • Notifier for adding SteamTinkerLaunch as a compatibility tool

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
Copy link
Author

AtomHare commented Jun 7, 2024

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
Copy link
Owner

sonic2kk commented Jun 7, 2024

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

@sonic2kk sonic2kk linked an issue Jun 8, 2024 that may be closed by this pull request
4 tasks
@sonic2kk
Copy link
Owner

sonic2kk commented Jun 8, 2024

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 😄

@AtomHare
Copy link
Author

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

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

@AtomHare AtomHare marked this pull request as ready for review June 23, 2024 18:45
@AtomHare AtomHare force-pushed the steamdeck-dependency-handling branch from 1435f06 to 8ef2dec Compare June 23, 2024 18:48
@sonic2kk
Copy link
Owner

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
Copy link
Author

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

steamtinkerlaunch Outdated Show resolved Hide resolved
@sonic2kk
Copy link
Owner

sonic2kk commented Jul 3, 2024

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! 😄

@sonic2kk
Copy link
Owner

sonic2kk commented Jul 26, 2024

Hello, it has been a while since the last commit. Did you have a chance to investigate the suggested change, and test this out on SteamOS? From what I remember aside from the PR comment I left this PR was in good shape and really just needed that change and testing.

To make testing easier, I had a thought of introducing a way to force the SteamOS code path in SteamTinkerLaunch in a debug mode of sorts. This would be intended to run on, say, a general Linux VM (so as not to disturb any existing SteamTinkerLaunch installation configuration). I have not gotten around to this yet and there is no ETA but if you have not gotten around to testing and don't wish to test on your own device (which is understandable) then we can park this PR for a little bit pending that change.

I am not in any hurry for a v14.0 release, there's still some loose ends on my side to tie up that will take a while, but this PR is important enough that I would wait for it to be merged anyway. 🙂

@AtomHare
Copy link
Author

Hey, sorry, I've been quite busy lately and wasn't much on GitHub. I don't mind testing on my own device, that's no issue.

About this PR, everything works fine (and is tested by editing manually the conf). I only need to edit the config save to point to the global conf instead of the game one.

I also thought of having a special env variable that we could use to force the STL steam deck behavior to make tests easier. I might take a look at it soon !

@sonic2kk
Copy link
Owner

No worries, real life always comes first! As I said there is no major hurry or anything.

About this PR, everything works fine (and is tested by editing manually the conf). I only need to edit the config save to point to the global conf instead of the game one.

Awesome news! This will be a very welcome improvement I am sure by all Steam Deck users.

I also thought of having a special env variable that we could use to force the STL steam deck behavior to make tests easier. I might take a look at it soon !

I haven't gotten around to it yet so by all means take a look! If nothing materializes, it's no problem either :-)

@AtomHare AtomHare force-pushed the steamdeck-dependency-handling branch from 9b9a66a to 4625c50 Compare August 19, 2024 15:55
@AtomHare
Copy link
Author

Finally fixed the place where the config was saved, sorry for the delay !
The PR is rebased too, it's now ready to test !

The only untested part (on my end) of this PR is not the behavior of the autoupdater logic but mainly if the config gets updated or not and if the global settings works too.

@sonic2kk
Copy link
Owner

Checked out the PR locally on my laptop and STEAMDECK_AUTOUP is created in the right place with the expected default value of 1. As far as I'm concerned, the global setting is being created as intended.

I will investigate testing this on SteamOS, on hardware or otherwise, in particular for testing the global setting, but feel free to test if you have time and are willing as well!


Thanks a ton for your hard work on this. I don't envision any major problems from looking at the code, the remaining work for this PR is really just a testing effort to confirm that things are fine, and updating the rest of the langfiles (and before merging we can bump the version to exclude the branch name 😄). I will also test this on my laptop and PC just to make 100% sure this doesn't impact anything outside of SteamOS.

If you're happy with the wording of the new strings in english.txt, feel free to copy and paste those into the other langfiles.

I did notice that for some reason with this PR, Yad is using XWayland, but I don't think that's related to this PR and is probably just some Yad weirdness, I'll double-check to see what is up there though just in case.


Once again, super excellent work, I'm really looking forward to seeing this merged!! I don't see any remaining blockers other than testing and making any fixes needed, and if any fixes are needed they're probably not going to require major refactors, I think the changes here is sound.

@sonic2kk
Copy link
Owner

As a brief follow-up, I confirmed the XWayland weirdness I was seeing is entirely unrelated to this PR, so no worries there :-)

@sonic2kk
Copy link
Owner

If you have tested this on your own Steam Deck and are happy that it works, I think this can be merged. It does not interrupt any of the Linux Desktop usage, and the code looks good to me. I don't have any way to test SteamTinkerLaunch on Steam Deck right now (I would really prefer to keep SteamTinkerLaunch off my OLED, and I don't have access to the LCD Steam Deck that I had gotten permission to test on right now).

I have left this in limbo for far longer than I would've liked and I feel bad about it, so if you're happy that it works, I can merge this. I am planning to note that Steam Deck is not actively supported prior to v14.0, so there won't be any responsibility or expectation here. The work here looks solid and I appreciate all the effort, and if you are happy that it works, I'll get this merged :-)

@sonic2kk sonic2kk mentioned this pull request Sep 19, 2024
14 tasks
@AtomHare
Copy link
Author

AtomHare commented Oct 4, 2024

Sorry for the late reply, the past month has been quite busy.

I consider this PR finished and everything should work as expected (Considering the only thing untested was the wrongly placed configuration).

However, I should be able to test in two weeks to verify the PR state !

P.S: Don't worry about the time scale of this PR, I lost a bit of momentum + life work, no issue at all regarding this !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Steam Deck Experience Improvements [Community Contributions Wanted]
3 participants