-
Notifications
You must be signed in to change notification settings - Fork 73
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
base: master
Are you sure you want to change the base?
Conversation
Woah! Thank you for making this PR!
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
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
Then skip the dependency update logic. That should prevent STL constantly trying to update itself.
I think the easiest solution here is to run If I'm missing something here let me know :-)
Yes. The reason it's only in Desktop Mode is because notifiers don't work in a GameScope session.
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.
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 :-) |
f52ca74
to
26b7f9c
Compare
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. |
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 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 😄 |
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 ! |
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 :-) |
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.
|
e3bc04f
to
f699a63
Compare
Thank you a lot for your testing. I rebased to have the Yad url fix in this PR. Not sure what went wrong, I will test it on my steam deck tonight too and see how I can prevent notification spam. |
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, The 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.) |
There was a problem hiding this 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 :-)
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: Recap of the remaining issues :
|
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 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).
EDIT: I left a comment about it, we can discuss the
Hmm, something is going wrong in It might be worth checking the |
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) |
No worries, you can work on this whenever you have time. All your work is appreciated thus far as well! :-) |
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 😄 |
No issue at all, thanks a lot for your help ! I've tested the PR and it looks ready for review ! |
1435f06
to
8ef2dec
Compare
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. |
You're welcome, thank you for your continuous help with the PR ! Yeah, no problem, there's no hurry ! |
That issue turned out to be the innoextract version needing bumped again for SteamOS 3.6 Preview which updates the Arch snapshot. False alarm! 😄 |
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. 🙂 |
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 ! |
No worries, real life always comes first! As I said there is no major hurry or anything.
Awesome news! This will be a very welcome improvement I am sure by all Steam Deck users.
I haven't gotten around to it yet so by all means take a look! If nothing materializes, it's no problem either :-) |
Co-authored-by: Eamonn Rea <[email protected]> fix: small typo between 1 and 0
Co-authored-by: Eamonn Rea <[email protected]>
Co-authored-by: Eamonn Rea <[email protected]>
+ update the PROGVERS
9b9a66a
to
4625c50
Compare
Finally fixed the place where the config was saved, sorry for the delay ! 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. |
Checked out the PR locally on my laptop and 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 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. |
As a brief follow-up, I confirmed the XWayland weirdness I was seeing is entirely unrelated to this PR, so no worries there :-) |
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 :-) |
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 !! |
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.
lastvers
with that of the incoming version (we can get the incoming version fromPROGVERS
) - managed bycheckSteamDeckSTLUpdated
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)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 dependencylastvers
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 updatedIssues:
If coming from an already existing install, we will create thelastvers
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 thelastvers
or creating it for the first time, either way it's a simple bash pipe), and makecheckSteamDeckSTLUpdated
return 0 if the file doesn't exist either.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