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

ONLYPROTMAJORREDIRECT doesn't properly handle latest Proton 9.0 Beta #1094

Closed
VoodooHillbilly opened this issue Apr 27, 2024 · 9 comments · Fixed by #1101
Closed

ONLYPROTMAJORREDIRECT doesn't properly handle latest Proton 9.0 Beta #1094

VoodooHillbilly opened this issue Apr 27, 2024 · 9 comments · Fixed by #1101
Labels
bug Something isn't working

Comments

@VoodooHillbilly
Copy link
Contributor

VoodooHillbilly commented Apr 27, 2024

System Information

  • SteamTinkerLaunch version: v14.0.20240423-1
  • Distribution: EndeavourOS
  • Installation Method: git clone & install

Issue Description

The latest Proton 9.0 Beta is using proton-9.0-1-rc2 in the Proton 9.0 (Beta)/version file.
When using ONLYPROTMAJORREDIRECT="1" in settings, STL creates a compatdata-proton-9.0-1 folder instead of the expected compatdata-proton-9.0 folder.

Source of Bug

I tracked it down to line 13341 in the steamtinkerlaunch script.
VERSPROTMAJOR="${USEPROTON%-*} only removes the final -rc2 from the version.
It needs an additional step to remove potential -rc* before removing -* from the proton-9.0-1-rc2 version number.

@VoodooHillbilly VoodooHillbilly added the bug Something isn't working label Apr 27, 2024
@sonic2kk
Copy link
Owner

sonic2kk commented Apr 27, 2024

Thanks for the report.

We could possibly fix this by replacing the parameter expansion with cut -d '-' -f1. Untested though. With how the dashes work it may end up being problematic still so we could then join the result of f1 and f2, and we'll need consideration for how this may impact GE proton. Although if we assume f1 is the name of the tool and f2 is the version, this should still be fine.

Thanks for investigating the problem as well, that'll be a good hint in investigating further. Your assessment makes total sense too, which is why I think cut is probably fine.

@VoodooHillbilly
Copy link
Contributor Author

VoodooHillbilly commented Apr 27, 2024

You're welcome. I was kind of curious if I could figure out a fix myself. I'm not familiar with scripts this complex, but I've been searching for an answer anyway. I was trying to find the shortest way to crop both endings.

Turns out VERSPROTMAJOR="${${USEPROTON%-rc*}%-*}" works great in zsh but bash throws an error.

For bash, I thought about specifically cropping any -rc* or -beta* strings from the end of USEPROTON the same way before cropping the minor version and assigning it to VERSPROTMAJOR. Honestly, I've got no idea which method would be faster or reliable. So I'll let you decide. Though if there's no problems with GE or other Proton versions, the cut option sounds like it would be faster.

I'm not sure if there are any other places that need a fix for their convoluted version numbers. Valve should really switch to semantic versioning.

@sonic2kk
Copy link
Owner

sonic2kk commented Apr 28, 2024

I was kind of curious if I could figure out a fix myself

Awesome, thank you very much! I think you were pretty much on the right track.

Turns out VERSPROTMAJOR="${${USEPROTON%-rc*}%-*}" works great in zsh but bash throws an error.

Yeah, Bash is funny like that, or rather zsh is just great heh.

For bash, I thought about specifically cropping any -rc* or -beta* strings from the end of USEPROTON the same way before cropping the minor version and assigning it to VERSPROTMAJOR

This was a pretty great idea, although doing it with parameter expansion might result in a more naive check. This is a major edge-case but bear with me.

If a Proton version contained the substring -rc or -beta, we would potentially erroneously trim this out. For example let's say there was a crazy Proton version called proton-9.0-1-rccola, we'd end up mapping this to proton-9.0.

A solution I came up with, which still isn't perfect, was to use sed to a pattern that will check for, as you suggested, both -rc and -beta. I came up with this: sed "s/-\(rc\|beta\)[0-9]\+$//g"

This basically says, match anything starting with -, then followed by *either rc or beta, then match anything that follows with 1 or more numbers, and match all of this as long as this is at the end of the string. So this would match -rc1 but not -rccola. Likewise, it'll match -beta1 but not -betaller. By the way, all of the escaping is because sed is dumb and hates readability because sed treats certain characters as text by default, so you need to escape them to tell it to use them as regex syntax. Crazy.

In the case of proton-9.0-1-rc2, this would leave us with proton-9.0-1, which isn't what we want. We could use another parameter expansion assignment like we currently have to clean it up, since ShellCheck would probably complain if we tried to use another sed expression.

So we'd probably end up with something along these lines:

VERSPROTMAJOR="$( echo "${USEPROTON}" | sed "s/-\(rc\|beta\)[0-9]\+$//g" )"  # Turns Proton name from "proton-8.0-3c-rc1" (or "-beta1") into "proton-8.0-3c"
VERSPROTMAJOR="${VERSPROTMAJOR%-*}"  # Turns Proton name from "proton-8.0-3c" to "proton-8.0"

Let me know what you think :-) I came up with this based on your idea to match those two strings, and in future if there are others we need to match, we can just extend this pattern (maybe make a variable for that part of the pattern, and then insert that into the pattern if this gets longer in future.

Also, that sed pattern is untested outside of a Bash session, so feel free to paste it in and tinker around to see if it solves the use-case. It may not work or there may be some edge-cases.

# Should give "proton-9.0"
VERSPROTMAJOR="$( echo "proton-9.0-1-rc2" | sed "s/-\(rc\|beta\)[0-9]\+$//g" )"
echo "${VERSPROTMAJOR%-*}"

# Should give "proton-8.0"
VERSPROTMAJOR="$( echo "proton-8.0-4-beta16" | sed "s/-\(rc\|beta\)[0-9]\+$//g" )"
echo "${VERSPROTMAJOR%-*}"

I'm not sure if there are any other places that need a fix for their convoluted version numbers. Valve should really switch to semantic versioning.

Agreed about Valve's versioning. As for other checks, I think we're good. I thought maybe the Proton version mismatch might need a check like this but it was already able to resolve it without any additional work. Thanks for thinking that far ahead though!


Feel free to play around with this fix or if you come up with a different one, let me know here or in a PR. I'd be glad to review any PR on this and help out/give feedback.

@VoodooHillbilly
Copy link
Contributor Author

This was a pretty great idea, although doing it with parameter expansion might result in a more naive check. This is a major edge-case but bear with me.

Since zsh worked with nested expansions, I briefly though maybe there was some way to do it all in the same expansion in bash without running additional commands. I've been busy and didn't spend much more time on it.

It took me so long to reply that Proton 9.0 is out of beta and release candidate phase. It now simply has proton-9.0-1 as a version number.

sed treats certain characters as text by default, so you need to escape them to tell it to use them as regex syntax. Crazy.

Having dealt with sed and some regex occasionally, I agree.

I combined the 2 separate operations into a sed command with multiple scripts. I've been getting the same results with it so far. It should make adding and revising replacements faster. Also, ShellCheck hasn't had any complaints with this.

VERSPROTMAJOR="$( printf '%s' "${USEPROTON}" | sed -e 's/-\(rc\|beta\)\d\+$//' \
                                                   -e 's/-\w\+$//' )"

or a longer, but slightly more compatible

VERSPROTMAJOR="$( printf '%s' "${USEPROTON}" | sed -e 's/-\(rc\|beta\)[0-9]\+$//' \
                                                   -e 's/-[0-9A-Za-z_]\+$//' )"

I can make a PR for either one if you'd don't see any problems with it.

On a related note, semantic versioning follows the major.minor.patch format. For example, shouldn't VERSPROTMAJOR be trimmed from proton-9.0-1-rc2 to proton-9 instead of proton-9.0? Otherwise, separate compatdata folders will be made for 9.0, 9.1, 9.2, etc.

@sonic2kk
Copy link
Owner

sonic2kk commented May 3, 2024

I've been busy and didn't spend much more time on it.

No worries at all :-)

I combined the 2 separate operations into a sed command with multiple scripts. I've been getting the same results with it so far. It should make adding and revising replacements faster. Also, ShellCheck hasn't had any complaints with this.

Personally I think either-or is fine. I would normally have a slight preference to using \d and \w (not sure why I didn't in my earlier reply) but there doesn't seem to be "precedent" for this in the codebase. This likely doesn't really matter, I think that syntax is pretty widely compatible with most distros especially those that would be doing any sort of gaming, but I think we can go with the slightly more compatible option.

Also, good catch with the extra sed, forgot to trim the increment version in my examples.

On a related note, semantic versioning follows the major.minor.patch format. For example, shouldn't VERSPROTMAJOR be trimmed from proton-9.0-1-rc2 to proton-9 instead of proton-9.0? Otherwise, separate compatdata folders will be made for 9.0, 9.1, 9.2, etc.

I never thought about this before, although this has only happened three times for Valve Proton; once for Proton 3 (3.7 to 3.7.16) and then with Proton 4 (4.0 to 4.11), and then again with Proton 5 (5.0 to 5.13). So I think it is fine to leave this as-is. I realise this doesn't match what GE-Proton does, where we only have one symlink per "major" version of GE-Proton. However, GE-Proton is based on the current Valve Proton major version, so if there was another Proton "point release", it would become GE-Proton9.X-Y (assuming the historical naming scheme applies).

Also, since Valve Proton releases are much less frequent, it makes more sense in my head to have separate compat data redirects. This is different than GE-Proton, which updates much more frequently, often with hotfixes, so it makes more sense in my head for that to have a single compat data redirect.

Valve Proton is usually quite "fixed", whereas GE-Proton is much more "rolling". You wouldn't necessarily want symlinks for GE-Proton9-1, 2, 3, 4, and so on. But if Valve released, in several months from now, Proton 9.7, it's much more likely for someone to go back and forth between Proton 9.0 and 9.7, than for someone to go between GE-Proton9-1 and GE-Proton9-4.

So in the above example, I think it makes more sense to have proton-9.0 and proton-9.7, but not as much sense to have GE-Proton9-1. I believe each release of Valve Proton here to be "major versions" even if that isn't quite in line with semantic versioning. Since there could still be a case for wanting proton-9.0 and proton-9.7, but not at the cost of also having proton-9.0-7a, b, and c (which is the behaviour without this feature enabled, #866).

I hope I've explained why I think that well enough.

However, I am not a frequent user of this feature. You may have used it more than me, and if you think I'm being silly, please let me know! I am not shutting this approach down, only explaining why I don't think it's necessary. If you think it is, we can discuss it further :-)


For the moment, feel free to open a PR with the slightly more compatible example whenever you have time. We can continue to discuss here or on the PR about whether we should trim down to proton-9 instead of proton-9.0.

@sonic2kk
Copy link
Owner

sonic2kk commented May 3, 2024

With #1101 merged, we can close this issue if you think it's resolved, or we can continue discussing if we want to change how we strip the version, such as making the directory compatdata-proton-9.

@sonic2kk sonic2kk linked a pull request May 3, 2024 that will close this issue
@VoodooHillbilly
Copy link
Contributor Author

VoodooHillbilly commented May 3, 2024

I would normally have a slight preference to using \d and \w

So do I, mainly because there's less typing involved.

I think that syntax is pretty widely compatible with most distros

That seems to be the case with bash from what I can tell. There was some uncertainty online with which UTF-8 characters are covered by \d. The longer version is a little clearer at a glance about which characters are being removed.

Also, good catch with the extra sed, forgot to trim the increment version in my examples.

You didn't actually forget anything. Your examples all used "${VERSPROTMAJOR%-*}" as the second step of the conversion. I just moved the increment version removal from the shell expansion to a second sed script.

I just realized my PR didn't include /g in either sed script like your examples. Though it would only be an issue if someone bizarrely includes -beta or -rc multiple times in their Proton version.

Since there could still be a case for wanting proton-9.0 and proton-9.7, but not at the cost of also having proton-9.0-7a, b, and c

The possibility of minor breaking changes between point releases (e.g., 5.0 to 5.13) is a good reason for keeping the current functionality. I was mainly thinking about keeping the concept of "major" versions consistent with the name of the feature. However Valve's non-standard versioning seems to either treat the first two numbers as a major version or treats the second number as an afterthought. So it's a mess either way.

However, I am not a frequent user of this feature. You may have used it more than me, and if you think I'm being silly, please let me know!

I've been (slowly) converting all my Steam games to use symlinked compatdata and steamuser folders. It's saving me a ton of storage. It makes moving a game to a new Proton version easier too. Having a few centralized Windows registry files, it's a lot easier to transfer game settings when there aren't 100+ sets of registries to search through.

With #1101 merged, we can close this issue if you think it's resolved,

It's resolved the original issue. The continued version stripping would likely need a new setting and could possibly introduce problems if there are game-breaking changes between minor versions. So I don't really see much of a need for it, unless Valve implements semantic versioning with more frequent minor changes in the future.

Unrelated, but I'm really not a fan of how git documents all my typos, skipped words, and indecisiveness.

Also unrelated, but I didn't want to open an issue for what may be a dumb question. I've been using USESUSYM="1" and USEGLOBSUSYM="1" for a global symlinked steamuser folder. What's the correct directory to contain AppData, Documents, etc.?

/steamtinkerlaunch/proton/steamuser/global
or
/steamtinkerlaunch/proton/steamuser/global/steamuser

Games usually access the global folder directly, but I think the "fix symlinks" option creates a steamuser folder inside steamuser/global. If the second directory is correct, I'm not sure why my current settings for games use the first one.

@sonic2kk
Copy link
Owner

sonic2kk commented May 5, 2024

I just realized my PR didn't include /g in either sed script like your examples. Though it would only be an issue if someone bizarrely includes -beta or -rc multiple times in their Proton version.

If it ever comes up, we can patch it, but I think that's fine :-)

However Valve's non-standard versioning seems to either treat the first two numbers as a major version or treats the second number as an afterthought. So it's a mess either way.

Yeah, I think part of it comes from trying to match Wine's versioning and then putting in their own revision number on top (as Proton versions changes are supposed to be rebased on top of a given Wine version, although the Valve Wine fork likely already includes backported patches 😄)

The continued version stripping would likely need a new setting and could possibly introduce problems if there are game-breaking changes between minor versions. So I don't really see much of a need for it, unless Valve implements semantic versioning with more frequent minor changes in the future.

Yeah I was also considering it would need to be an additional setting to handle cleanly. A neat but potentially overkill solution would be to make the pattern customizable but that would probably be more effort than it would be worth, but it would allow handling this case and Proton versions with complex names (like Proton-tkg Wine Master builds).

What's the correct directory to contain AppData, Documents, etc.?

This should be steamuser if I'm understanding correctly. The steamuser folder is the "username". In a Wine prefix, the user folder is normally the current user's username. But for Proton prefixes, it uses steamuser.

It's my understanding that AppData, Documents, etc will go into the user folder. I think there can be a higher level AppData folder outside of the user folder. So in a regular prefix, drive_c/users/steamuser/AppData may exist and also drive_c/AppData may exist too. The user folder one may be a symlink to the latter. I'm not at my PC at the moment to check, sorry 😅

There is not a whole lot of consistency for where games write their data, kind of like on the Linux Desktop heh. It can be tricky to cover all cases but if you need more help please ask or open a separate issue if you think it's worth it!


Glad the main issue here is resolved at least. Thanks for your work on this!

@VoodooHillbilly
Copy link
Contributor Author

I was aware of the standard Windows/WINE/Proton folder structure. The problem seems to have been with the symlink itself. For some reason the symlinks in the prefixes, e.g. .config/steamtinkerlaunch/proton/compatdata/compatdata-proton-9.0/pfx/drive_c/users/steamuser, had been pointing to .config/steamtinkerlaunch/proton/steamuser/global as the target folder. Games loaded and saved data there without problems.

The "Fix symlinks" option creates .config/steamtinkerlaunch/proton/steamuser/global/steamuser, but it never updates the steamuser symlink inside the prefix to point to this location. Leaving me with AppData, Documents, etc. inside both /proton/steamuser/global and /proton/steamuser/global/steamuser.

I manually updated the pfx/drive_c/users/steamuser symlinks to point to .config/steamtinkerlaunch/proton/steamuser/global/steamuser and moved all the data to that subfolder. Everything seems to be working so far. I have no idea why the symlinks were pointing to the wrong location.

Glad the main issue here is resolved at least. Thanks for your work on this!

You're welcome.

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.

2 participants