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

ShellCheck: Fix warnings since ShellCheck v0.10.0 #1064

Merged
merged 4 commits into from
Mar 16, 2024

Conversation

sonic2kk
Copy link
Owner

@sonic2kk sonic2kk commented Mar 15, 2024

Prerequisite to #1056.

Overview

Fixes ShellCheck warnings not caught in v0.8.0 that are caught now that we can use v0.10.0.

ShellCheck v0.9.0 added a new data flow analysis engine, but this sucks up ram on certain scripts (not necessarily tied to length) including SteamTinkerLaunch, so we were stuck on v0.8.0, which is a couple years out of date.

Recently, ShellCheck v0.10.0 came out, and added an option to disable the new data analysis engine introduced in v0.9.0 with a directive. SteamTinkerLaunch now has this directive (#1061), so we can now use ShellCheck v0.10.0! Woohoo!

ShellCheck v0.10.0 flags up quite a lot of warnings that v0.8.0 didn't. They are all existing warnings but it's better at catching them. This PR will aim to address as many as possible, and from a quick glance, I think all of them can be addressed.

Warning Types

The three main warning types we're running into now are:

  • SC2086: Use quotes to prevent word globbing.
  • SC2028: Echo may not expand escape sequences.
  • SC2004: $/${} is unnecessary for arithmetic variables.

I will address each of these in their own commit.


Opening now but only SC2086 has been dealt with so far. I confirmed that adding the quotes was safe.

SC2004 should be straightforward enough too, but I am concerned about solving 2028 without breaking functionality. It will probably be sorted out last.

@sonic2kk sonic2kk force-pushed the shellcheck-0-10-0-fixes branch from 027e490 to fae36c5 Compare March 15, 2024 20:51
@sonic2kk
Copy link
Owner Author

Fixed the SC2004 warnings with fae36c5. I was a bit concerned about these, but I tested in the "debug" commandline option with gameScopeArgs and it worked fine so I think it's okay.

@sonic2kk
Copy link
Owner Author

Fixed the SC2028 warnings. However, there is a change in behaviour for the ModOrganizer 2 INI logic. See below the difference in what echo gives and what printf gives.

echo "download_directory=${GLOBZMOIN}\\\\downloads"  # download_directory=Z:\\home\\username\\.config\\steamtinkerlaunch\\mo2\\compatdata\\pfx\\drive_c\\users\\steamuser\\AppData\\Local\\ModOrganizer\\\\downloads

printf "download_directory=%s\\\\downloads\n" "${GLOBZMOIN}"  # download_directory=Z:\\home\\username\\.config\\steamtinkerlaunch\\mo2\\compatdata\\pfx\\drive_c\\users\\steamuser\\AppData\\Local\\ModOrganizer\\\downloads\n   

I'm pretty sure the behaviour printf is giving is correct, it looks strange to me to have the two sets of \\. So I don't really have any concerns.

For the ReShade changes to use printf, there is no change in behaviour.


That's all the warnings fixed, ShellCheck is happy after this. I will do some more manual review and testing of this and merge once I have confidence everything is fine.

@sonic2kk sonic2kk force-pushed the shellcheck-0-10-0-fixes branch from 9151dc0 to ee39dcc Compare March 16, 2024 22:34
@sonic2kk
Copy link
Owner Author

Some of the backslash counts look a little odd, but nothing that was changed in this PR, so I will leave them as-is for now. One example is:

printf "mod_directory=%s\\\mods\n" "${GLOBZMOIN}"

This has three backslashes instead of four for some reason. But, since it's been like that for a while, it's probably fine. If using printf breaks this we can try and fix it by adding another backslash.

@sonic2kk
Copy link
Owner Author

This is ready to merge!!

@sonic2kk sonic2kk merged commit 34a0639 into master Mar 16, 2024
@frostworx
Copy link
Collaborator

the weird count of backslashes should be correct - at least it was when I wrote it.
I don't remember any details, but I had to write it to make "the windows part" of MO2 happy.
so it was/is some confusing combination of linux backslash escaping in quotes and additional one or two backslahses for the windows path.

@sonic2kk sonic2kk deleted the shellcheck-0-10-0-fixes branch March 17, 2024 19:04
@sonic2kk
Copy link
Owner Author

Working with backslashes when doing things with Wine is really weird and confusing, at least for me it is 😄 So if this becomes a problem, I'll add extra backslashes to compensate for printf (potentially) removing them.

MO2 may also be smart enough to just account for this itself.

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.

2 participants