Add some checks to restoreOrgVars on startGame

[HanPrower]

My tentative and ugly attempt at fixing #1158

Simply when we're in startGame check the relevant custom-vars to see if any of the envvars that would normally be reset to original values are defined, and just don't reset them.

Probably needs more testing to be safe, but so far so good.

I wasted so much time on this 😿

[sonic2kk]

Sorry it took me so long to get around to taking a look at this!

I think I see the solution here, essentially check for some known variables that Steam will override, and if they're defined in either the Global or Per-Game Custom Environment Variables config file, don't overwrite them.

The issue in #1158 was then, based on the comment in that issue, not actually caused by Steam but by some funky SteamTinkerLaunch behaviour, which this prevents.

I think calling your solution "ugly" is a stretch 😅 It makes sense, although I think we can improve it a little bit.

We could make an array with these variables and loop through them, something like STLNOOVERRIDEENVVARS, or something with less repeating characters ideally. Something like this ABETTERVARIABLENAME=( "LD_PRELOAD" "LD_LIBRARY_PATH" "LC_ALL" "PATH" ).

Then we can loop through with something like this, where we can dynamically build the variable name to assign to using eval, which will allow us to extract the variable name rather than the value and runs it as a command. We can create a string representing the variable name we want to assign to based on the current iteration in the loop, so if LOOPVAR was LD_PRELOAD, we could write ORG_${LOOPVAR} which would become ORG_LD_PRELOAD. Then we can use eval to assign LOOPVAR, which is LD_PRELOAD, the value of ORG_${LOOPVAR}, which would be the ORG_LD_PRELOAD variable, assuming this exists and is defined (which is an assumption we can make).

Basically using eval lets us build a string command to run for some dynamic assignment. I wrote up a code snippet, I didn't test it, I just wrote it up in a text file and tested the concept in a Bash prompt. Hopefully it gives the general idea though :-)

if [ "$1" != "startGame" ]; then
    # Return early if not startGame, I think this is safe to do as nothing else in the function should run if $1 != startGame
    # This should also simplify the condition in the loop
    return
fi

# STLRESTOREVARS could be a potential name, but open to better suggestions
STLRESTOREVARS=( "LD_PRELOAD" "LD_LIBRARY_PATH" "LC_ALL" "PATH" )
for RESTOREVAR in "${STLRESTOREVARS[@]}"; do

    # e.g. 'ORG_LD_PRELOAD' for 'LD_PRELOAD' on first iteration of the loop
    BUILTORGVARNAME="\$ORG_${RESTOREVAR}"

    # I think we can use grep like this? I think sed is fussy about using variables, not sure about grep!
    if grep -q "^${RESTOREVAR}" "$GLOBCUSTVARS" || grep -q "^${RESTOREVAR}" "$GAMECUSTVARS"; then
	    # Since `eval` extracts the value and runs the command, the part inside the quotes comes out to e.g. 'LC_ALL="$ORG_LC_ALL"'
	    # The dollarsign comes from the '\$' earlier, it's also possible to add it here but I would prefer to build it all in BUILTORGVARNAME
	    #
	    # Since this runs as a command, this is equivalent to typing in a Bash prompt 'LC_ALL=$ORG_LC_ALL'
	    # Everything in the quotes is a string that gets built as a command to run, and 'eval' then runs it
	    #
	    # It's all a bit weird, I hope that makes sense. I also only tested this with some test code in a Bash prompt, so it may need some more refining
	    eval "${RESTOREVAR}=\"${BUILTORGVARNAME}\""
	fi
done

Something like the above should be a bit more concise and allow us to more easily extend this if there are other variables to include, or if we ever want to make this list user-facing.

As well as this, in the loop we should make the log a big more explicit that these are not being overridden and that, in some cases, this could be problematic. Any user overriding these should ideally know what they are doing and should be doing it for good reason (locale could be overwritten for testing purposes, for example).

---

Apart from the above potential improvement, I think this approach is fine, provided it doesn't cause any regressions for cases where the user doesn't override these. In other words, things should continue as normal if this is not defined in any of the custom environment variable files, but if they are defined, your issue in #1158 should be resolved :-)

If you can give some explicit testing steps I would also be happy to test this out, just to make sure we're on the same page.

Thanks a lot for your work and patience here to figure this out!

[HanPrower]

Don't apologise for taking some time to review things. It's a hobby project, you likely have better things to do and stressing over replying here will probably lead to burn out :)

My fix was ugly it was just a throw-my-hands-up-and-get-something-that-works-and-go-cry fix. What's funny is I asked my g/f how she'd fix it (she's a better coder than me) and she said she'd build an array and check against it 😅 Probably not quite the same as what you came up with, but still.

---

Just to address something: I specifically put in a check for startGame just to make sure it worked for me specifically without causing issues whilst launching a game (because of the gamescope issue).

However, restoreOrgVars is actually called 10 other times:

• 8 of them are in extProtonRun just before a custom program is run.
• • I'm not sure we want to restore all the envvars here as a user may expect custom vars to be loaded for whatever custom programs they're running.
• 1 is in checkFWS (for running FlawlessWideScreen)
• • Again, probably don't want to restore them here as is this launching proton and fws, so the user again will likely expect what they set to survive here
• 1 is in installWinetricksPaks
• • We may want to restore them to original here... I'm not sure what winetricks cares about as environment, but as it's trying to do something specific within a wine prefix it may want to be "default". Then again... if there's an edge issue that's fixed by overriding the envvar... 🤷‍♂️

So instead of $1 being "startGame" it may want to be "checkCustomVars" or, inversely, "forceOriginal".
Then if checkCustomVars is set that's the only time we check the greps and if the greps are true we just continue otherwise the original is restored.

Alternatively we don't have an $1 and always set things as the user defines. Then have a message in the log about the user has overridden things and it may cause issues.

I may lean towards no $1, as always stated STL is for advanced users, and if people are overriding those sorts of envvars they must have a good reason. It's not like things will explode... at worst locales will be odd, or the Steam overlay will break, or programs simply won't be found and just deleting or commenting the line or closing STL and relaunching will reset the envvars to Steam's defaults anyway.

---

As for a way to test you can simply try LD_PRELOAD="" in a custom-vars conf file. That will cause the Steam Overlay to not function even if enabled in Steam 😄
You'll also notice the lack of this spam in your system's journal when you launch a game / program from STL:
Sep 03 10:33:17 smokey steam[20078]: ERROR: ld.so: object '/home/hanfox/.var/app/com.valvesoftware.Steam/.local/share/Steam/ubuntu12_32/gameoverlayrenderer.so' from LD_PRELOAD cannot be preloaded (wrong ELF class: ELFCLASS32): ignored.

[sonic2kk]

My fix was ugly it was just a throw-my-hands-up-and-get-something-that-works-and-go-cry fix. What's funny is I asked my g/f how she'd fix it (she's a better coder than me) and she said she'd build an array and check against it 😅 Probably not quite the same as what you came up with, but still.

Heh, I like that expression. 😄

And well, it is still an array, we just dynamically assign them. In other languages you could probably use a hash/dictionary or something similar, but in Bash we can make do with this :-)

---

You raise some good points here about when we should and should not use this logic. I think in most cases a user is going to expect what they define in their custom environment variables file to carry over, the benefit of this PR is to allow those variables to survive and to not have SteamTinkerLaunch override them.

I'm not sure we want to restore all the envvars here as a user may expect custom vars to be loaded for whatever custom programs they're running.

First, I guess the 8 calls are for different code paths? Or is this called 8 times consectutively when starting a custom program? If so, it's not something for this PR but yikes... I'll need to investigate that!

Apart from that, I would agree that custom environment variables should be passed to custom commands and that this logic should apply for them as well.

If a user runs into problems here, there is a warning log that the values aren't being restored because they're already defined in the custom environments variable file.

Again, probably don't want to restore them here as is this launching proton and fws, so the user again will likely expect what they set to survive here

I agree with you here as well.

We may want to restore them to original here... I'm not sure what winetricks cares about as environment, but as it's trying to do something specific within a wine prefix it may want to be "default". Then again... if there's an edge issue that's fixed by overriding the envvar... 🤷‍♂️

This is a tricky one for sure. And there have historically been cases where Winetricks has benefitted from overriding, for example, locale. On the other hand this could cause unexpected behaviour with Winetricks that is actually needed for a game.

I think for now we should keep still allow the current behaviour of this PR to apply to Winetricks, where variables won't get restored if they're defined in the custom environment variables file. This means a user can apply those overrides if they need to.

I may lean towards no $1, as always stated STL is for advanced users, and if people are overriding those sorts of envvars they must have a good reason. It's not like things will explode... at worst locales will be odd, or the Steam overlay will break, or programs simply won't be found and just deleting or commenting the line or closing STL and relaunching will reset the envvars to Steam's defaults anyway.

I am leaning towards this as well, based on the above. I think updating the wiki to document this will help too. Honestly, I don't think many people read the wiki (a couple people complained before when I mentioned the wiki) but still, I think making a note of this is useful. I also refer to the wiki as well if I'm ever looking at code and wondering the use-case.

I think having no conditional check for what is calling restoreOrgVars is the best approach. Anywhere it is called we should just follow the logic to restore the original variables unless they are defined in the Global or Per-Game config file. We can documented this and call out the cases you have mentioned, in case users actually want to take advantage of this for, say, Winetricks and override some environment stuff.

I am with you on users that do this must have a good reason. This is something users probably aren't going to do lightly, and I think it is valuable to override these. The more I consider it the more I agree that SteamTinkerLaunch not doing this is already a bug, as I would have also expected the override to work. Although SteamTinkerLauch probably didn't do it before because we want to restore those original variables since we might modify them (in particular, PATH on SteamOS we probably modify (not on disk, just the environment variable)).

As for a way to test you can simply try LD_PRELOAD="" in a custom-vars conf file. That will cause the Steam Overlay to not function even if enabled in Steam 😄
You'll also notice the lack of this spam in your system's journal when you launch a game / program from STL:

I will give it a try ASAP, does it work on your end out of interest?

---

The code looks good to me, if testing goes well I don't have any comments on the code itself. I'm glad you were able to implement it from my horrid explanation heh.

ShellCheck is grumpy because now it thinks the ORG_ variables are unused in saveOrgVars. The "real" fix is to apply similar logic to what you implemented in this PR to that function also, although because that introduces duplicate logic it would probably be best to make a function to do it, and I think that's way out of scope for this PR. For now, we can ignore the ShellCheck warning with a directive comment # shellcheck disable=2034. This tells ShellCheck to be quiet because we know better than it does 😉 you might have to put it above each line, it might get grumpy if you try to put it beside each line, but you can try putting it beside first and seeing if that works.

If you could also add a comment in saveOrgVars: # TODO resolve the unused warnings by dynamically assigning the variables like we do in 'restoreOrgVars'.

---

Thanks again for your work on this! Provided it works in my own testing (which from looking at the code I expect it should), I have no trouble merging this. Resolving the ShellCheck complaints would be great but it won't block merging.

[sonic2kk]

Tested this PR, works as expected with LD_PRELOAD="" in the Per-Game and the Global config file., as this is the most visually obvious to test. There is no reason to expect the other variables don't work as well. Thanks a lot for your investigation work to discover this issue and fix it! 😄

Once the ShellCheck warnings are addressed, and a TODO is added to saveOrgVars to clean it up later this PR can be merged.

[sonic2kk]

Needs a rebase because of the version bumps in the commits made since this PR was opened. I can do the rebasing if you want, or feel free to do it when adding the ShellCheck directives. 🙂

[HanPrower]

First, I guess the 8 calls are for different code paths? Or is this called 8 times consectutively when starting a custom program? If so, it's not something for this PR but yikes... I'll need to investigate that!

Basically from https://github.com/sonic2kk/steamtinkerlaunch/blob/master/steamtinkerlaunch#L7247 onwards there are a load of if blocks that all restoreOrgVars and then emptyVars "O" "X". It's already marked as a TODO to refactor though.

---

Changes done. Hopefully right 🤞

[sonic2kk]

The new changes address the comments above, thanks for bumping the version as well!

This can be merged.

[sonic2kk]

Thanks again for the work on your recent PRs! It's still a part of being a "maintainer" that I'm getting used to, I really appreciate you working with me :-)