search

sonic2kk / steamtinkerlaunch

Linux wrapper tool for use with the Steam client for custom launch options and 3rd party programs
GNU General Public License v3.0
2.04k stars 70 forks source link

feat(dep): check integrity #1054

Closed Mte90 closed 3 months ago

Mte90 commented 3 months ago

Based on https://github.com/sonic2kk/steamtinkerlaunch/issues/859

This check if innoextract and cabextract are fine.

Check the size is not something that can confirm as you suggested in the ticket, the only one is to run the command and see if crashes.

sonic2kk commented 3 months ago

This looks pretty great, thanks! I like this approach a lot, I think it makes sense. I had some comments but ShellCheck summarises them better, I'll leave them as comments on the specific lines themselves.

I realise you had issues with the Bash LSP so it's understandable. As a workaround you can instead download ShellCheck 0.8.0 manually and add it to your path as an alias, that's what I do for ShellCheck 0.8.0. Then from the project folder, you can run shellcheck steamtinkerlaunch to check the script. This will hopefully be unnecessary once a newer ShellCheck version releases, fixing the issue for various scripts.

Apart from that, I was wondering if we should make it a dedicated function, in case we can use this elsewhere, but I don't think we really use many native binaries this way. If needs be in future, we can split it out.

Thanks for your contribution! Once the ShellCheck comments are addressed, I'll merge this :-)

Mte90 commented 3 months ago

Yeah I am using Scite that doesn't use LSP and doesn't lag with huge files (I use that for mysql dump that often are bigger than this).

sonic2kk commented 3 months ago

ShellCheck v0.10.0 is out, and once I add the directive to not use extended analysis, as long as you're using an up-to-date LSP things should be fine.

sonic2kk commented 3 months ago

Sorry, I just realised I forgot to mention (and document) that the version should be bumped. There's no exact rule on this, basically just the date for you + 1, and then bump the revision number. So for example you might set v14.0.20240316-1.

This isn't enforced, this is just my own workflow that you might find useful when working on PRs: typically what I do is set the version during development, and in brackets, add the branch name. This means if another PR uses the same date, the version string is still unique. So in this case it might be v14.0.20240316-1 (valid-dependency). Then, once the PR is ready to be merged, I change the version. If you submit a large PR it might be good to do this, and once all feedback is addressed and ready to merge, I can leave a comment to bump the version and then it'll be ready to merge. :slightly_smiling_face:

This is used to know when and how to update config files, and they are assigned a version. It's not massively important for afaik any of your PRs but still just good practice (I forget to do it sometimes myself!)

Once this is done, the rest of the changes look good to me, and this will be ready to merge. ShellCheck v0.10.0 is very angry about a lot of this script, but v.0.8.0 gives no warnings after the changes. At a glance though I think v0.10.0 isn't complaining about anything specific to this PR.

Thanks a whole lot! :smile:

Mte90 commented 3 months ago

As you asked version bumped.

sonic2kk commented 3 months ago

Thanks! Although the string isn't formatted quite right.

Your version string is v14.0.20240316-(valid-dependency).

This PR has conflicts because of my mergings, but I'll see if I can rebase this PR myself and merge it if that's ok. :smile: You can also use the diff here as reference for future PRs.

Thanks!

Mte90 commented 3 months ago

Fixed the version and for conflicts, yeah it is better that you do so :-)

sonic2kk commented 3 months ago

Heh, I took a quick look at how to contribute to someone else's PR using this gist. I will try my best not to mess anything up on you, but if I do, I can probably just pick your PR into another branch and then squash-merge it (you might end up as a co-author), but you'll be credited correctly in the changelog :smile:)

Mte90 commented 3 months ago

To me is perfectly fine :-)

sonic2kk commented 3 months ago

I got it in the end I think. Just doing one last ShellCheck run and then I will merge this.

Thank you for your patience, continued discussions and contributions! It is much appreciated.

sonic2kk commented 3 months ago

Changelog has also been updated to include your contribution (might be easiest to see with Ctrl+F for @Mte90): https://github.com/sonic2kk/steamtinkerlaunch/wiki/Changelog - This changelog gets copied and pasted into the release notes, so you will be credited in the same way there too.

Also, it looks like despite the commits in the PR being co-authored (because of the rebase), the squash-merge properly only included you, which I am happy about!

I am very keen on making sure all contributors get properly recognised, which is also why I bring it up here too. Thanks again for all your work!