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

feat(detect): if steam and yad not found stop the program #1059

Merged
merged 7 commits into from
Mar 19, 2024
Merged

feat(detect): if steam and yad not found stop the program #1059

merged 7 commits into from
Mar 19, 2024

Conversation

Mte90
Copy link
Contributor

@Mte90 Mte90 commented Mar 15, 2024

Fix #1058

@sonic2kk
Copy link
Owner

Thanks!

I don't think we should exit if Steam is missing, I think we should just more explicitly warn.

In its current state as well, I have a few concerns:

  • If we're going to check for Steam this way, we need to also make sure we're not on Flatpak.
  • The echo isn't correct for Steam being missing, it should just say echo "ERROR: Steam not found
  • Exiting if we can't find Yad is a bit excessive I think, since there is perfectly valid commandline usage. We already log if Yad is missing, but I think we can improve it to more clearly state that a Yad binary is missing in checkIntDeps.
    • The logging you showed in Check if steam is installed #1058 isn't great.
    • Also, users should already know to install Yad based on reading the installation documentation.
    • If the commandline usage on Desktop still complains about a missing Yad, that ideally should be fixed, but that's a larger effort for something that I think can be fixed by making the logs clearer.

I'll leave some of these as review comments where appropriate for easier tracking :-)

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

This is my first time doing the "request changes" thing on GitHub so let me know if I did anything wrong :-)

I ran ShellCheck v0.10.0 against your branch, there were general warnings not present in v0.8.0, and none specific to your PR, so no worries there.

@Mte90
Copy link
Contributor Author

Mte90 commented Mar 18, 2024

You did in the right way but I missed those notifications the other day.

Anyway I think that check if it is running in CLI mode for yad detection can be a way.

@sonic2kk
Copy link
Owner

No problem, easy to miss :-) Before I learned how to do this properly, I left comments in review without actually submitting them for a day or two 😄

@sonic2kk
Copy link
Owner

sonic2kk commented Mar 18, 2024

Apparently I cannot leave a comment on it, but if you want to fix up the Yad error messaging, you can change this line:

writelog "ERROR" "${FUNCNAME[0]} - '$YAD' was not found! Check '${PROGCMD} --help' for alternatives and/or read '$PROJECTPAGE/wiki/Yad'" "E"

To

writelog "ERROR" "${FUNCNAME[0]} - Yad dependency ('$YAD') was not found! Check '${PROGCMD} --help' for alternatives and/or read '$PROJECTPAGE/wiki/Yad'" "E"

There's no obligation to, but as you pointed out, the error doesn't make much sense, so this would be a nice win to include in this PR :-)

@sonic2kk
Copy link
Owner

Sorry for the triple-commenting, but just wanted to mention: This PR doesn't have any conflicts, and I don't think it will as I'm hoping to merge it soon, but if it does end up with conflicts (or any if your future PRs) and you want assistance rebasing, feel free to ask :-) I'm not an expert but I'll do my best to help out anyone generous enough to help out.

Rebasing is still kinda magic to me, and I don't think we'll be in a situation with a large difficult-to-rebase PR (if there was ever a large PR to come in, I would prioritise merging it first over my own commits to make life easier for contributors).

@sonic2kk sonic2kk added shellcheck Run ShellCheck workflow against PR and removed shellcheck Run ShellCheck workflow against PR labels Mar 18, 2024
@sonic2kk
Copy link
Owner

sonic2kk commented Mar 18, 2024

Sorry for the labelling noise, was testing out the ShellCheck CI (adjusted it a little with #1068).

I expected it to fail as it wasn't rebased against master, but it didn't fail, which was a bit odd... Locally it's eating up tons of ram still, and the fixes aren't in place from #1064. I have no idea why the workflow is passing, because it should be failing for this PR...

EDIT: The Action is working, see #1069. Not sure why it's passing here but I'll take it I suppose.

@sonic2kk sonic2kk removed the shellcheck Run ShellCheck workflow against PR label Mar 18, 2024
@Mte90
Copy link
Contributor Author

Mte90 commented Mar 19, 2024

So there is a conflict as I can see, this happens with huge files but we already talked about this.

Anyway I did the changes you suggested.

@sonic2kk
Copy link
Owner

this happens with huge files

No, conflicts happen regardless, it is nothing to do with the size of the file (lines, not bytes ofc 😉). This is simply the nature of version control, happens often in other projects I contribute to (namely ProtonUp-Qt), happens in work all the time as well.

I haven't checked yet, but the issue here is almost definitely a result of needing rebased from the version bump in other commits (most likely a7e06e3), this is typically what happens when one feature is merged before another. Normally each PR needs a rebase after a feature merge. I'm not sure why the GitHub Web Editor says the conflicts are "too complex" but this happens often now, and the Web Editor is not great to use to resolve conflicts anyway.

But either way, I'm happy to do the rebasing. If you haven't done a rebase before, it is basically lifting all of the commits and moving them on top of the most recent commit, as if you were applying your changes to begin with at the most recent commit.

For cases like this it is normally just a case of:

  1. Make sure your master branch is synced with this master branch (you can typically do this from GitHub on your fork's master branch page, it'll fast forward the commits - There is a command, but I forget it now)
  2. Pull these changes into your local master, so it is up-to-date also, i.e. git checkout master && git fetch && git pull master && git checkout steam-check
  3. Rebase with git rebase master
  4. Resolve the conflicts, normally this is just the version bump. Some editors have UIs for resolving conflicts, but usually you can just accept your current version. After this you can git add steamtinkerlaunch (or any other files that have conflicts) and force-push the rebased commits (rebasing rewrites history since it "moves" commits and the hash changes).

You can read more about rebasing on the Git docs, since it can do a bit more than what is needed here (for example, squashing certain commits together).

But I'm always happy to help out with rebasing or other related things with PRs :-)

@sonic2kk sonic2kk added the shellcheck Run ShellCheck workflow against PR label Mar 19, 2024
@sonic2kk
Copy link
Owner

Rebase done, it was indeed just the version that needed a conflict resolved. I rebased keeping the version the same, and then pushed a commit that bumped the version to be ready for merging.

Running ShellCheck locally and on CI, and once verified, I will merge (the CI is so freaking cool by the way, I love it 😄)

@sonic2kk sonic2kk removed the shellcheck Run ShellCheck workflow against PR label Mar 19, 2024
@sonic2kk sonic2kk merged commit c9a8ed9 into sonic2kk:master Mar 19, 2024
1 check passed
@sonic2kk
Copy link
Owner

sonic2kk commented Mar 19, 2024

Updated the wiki to note that Steam is required: https://github.com/sonic2kk/steamtinkerlaunch/wiki/Installation/_compare/1355066310e31cc7b00d872d2c3f67ce8e79b54d

Not sure if it's worth updating the note on ProtonUp-Qt. I think I'll open a discussion over there and ask. Nevermind, after DavidoTek/ProtonUp-Qt#356, it shouldn't be possible for it to install any Steam compatibility tool without a valid installation; in other words, Steam will always be available when installing SteamTinkerLaunch from ProtonUp-Qt, since the option to install any Steam compatibility tool won't appear if there is no Steam installation to begin with.

@sonic2kk
Copy link
Owner

I forgot to update the changelog to note this contribution until just now! I'm sorry about that, the changelog is now updated: https://github.com/sonic2kk/steamtinkerlaunch/wiki/Changelog/_compare/95260902f974ece48d77509e0362eab14b207832

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.

Check if steam is installed
2 participants