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

How to code contribute #1053

Closed
Mte90 opened this issue Mar 12, 2024 · 9 comments
Closed

How to code contribute #1053

Mte90 opened this issue Mar 12, 2024 · 9 comments
Labels
enhancement New feature or request

Comments

@Mte90
Copy link
Contributor

Mte90 commented Mar 12, 2024

So after investigating with the project files I see that is missing a guide on how to compile it as example, there isn't any CI that generate the packages (can be helpful to have the nightly packages as example).
There is a makefile but what are the dependences? edit: found on https://github.com/sonic2kk/steamtinkerlaunch/wiki/Installation#dependencies-1
It would be cool a script like configure that checks if you have everything locally to develop STL.

On https://github.com/sonic2kk/steamtinkerlaunch/blob/master/CONTRIBUTING.md#testing-development-builds there is a mention on testing build, maybe can be helpful a way to get the latest commit automatically so is not needed to change a variable in the code, with the risk to commit it.
Usually with CI the version is set by code or the tag and the CI generate the package using that tag name as version so isn't needed to change something.

Also for code quality having a huge bash file with 18k~ lines is not very welcoming and easy to navigate (https://github.com/sonic2kk/steamtinkerlaunch/blob/master/steamtinkerlaunch).

I would like to help on those tasks but some guidance or a documentation for that can help others to join the project.

@Mte90 Mte90 added the enhancement New feature or request label Mar 12, 2024
@Mte90
Copy link
Contributor Author

Mte90 commented Mar 12, 2024

Reading the STL bash code it shouldn't be difficult to split in various files, so as example if I want to do a config generator with a bash script or a command shouldn't be impossible.

Also on my debian sid machine with kate and bash-lsp configured crash the computer with 16gb of ram because the file is too huge...

@sonic2kk
Copy link
Owner

sonic2kk commented Mar 12, 2024

I'll try to reply to this piece by piece, because I think there is a misunderstanding :-)

There is absolutely room to clean up CONTRIBUTING.md though, some of the information is a little outdated or could be amended. I may also look into clearing up some of the points mentioned here specifically.

So after investigating with the project files I see that is missing a guide on how to compile it as example, there isn't any CI that generate the packages (can be helpful to have the nightly packages as example).

There is no compilation for SteamTinkerLaunch, it is a Bash script. There are no nightlies because there is nothing to compile. To run SteamTinkerLaunch, just clone from master and ./steamtinkerlaunch, and this is your nightly :-) This will just show the help screen because no command was given. To use it with Steam, there is some information in CONTRIBUTING.md but it is basically a case of pointing the symlink in Steam to your local development script instead of one installed elsewhere. To use SteamTinkerLaunch with native games, you'll need to use the Makefile.

I would like some CI for ShellCheck eventually, but it's not a high priority, and I will wait until a version of ShellCheck is available as it should fix a bug that cause some scripts (including SteamTinkerLaunch) to eat up RAM and crash the system (koalaman/shellcheck@d80fdfa is supposed to fix this, see koalaman/shellcheck#2652).

There is a makefile but what are the dependences?

The Makefile is used to install and uninstall SteamTinkerLaunch to your system directories, such as /usr/bin/steamtinkerlaunch. This is for commandline usage and for Native Linux games, where SteamTinkerLaunch is used as a launch option.

there is a mention on testing build, maybe can be helpful a way to get the latest commit automatically

Perhaps the wording is unclear, "Development Build" is the wrong phrasing, in this section I am referring to testing your local script changes.

Usually with CI the version is set by code or the tag and the CI generate the package using that tag name as version so isn't needed to change something.

Again, there is no package to generate. Even the Releases section for SteamTinkerLaunch is just the repository zipped/tar'd at that given point with a given tag. There's nothing to compile because it's a Bash script.

Also for code quality having a huge bash file with 18k~ lines is not very welcoming and easy to navigate

I would like to direct you to the Winetricks project :-) This is simply how Bash scripts are because of the nature of the language. Also, there are other reasons why SteamTinkerLaunch is a single script.

I don't think the file size makes it less welcoming per-se. It might be daunting at first, but any project with lots of code would be like this. Even projects with multiple files can have files tens of thousands of lines long. Valve's Gamescope has some pretty huge files, for example. SteamTinkerLaunch was smaller when I started contributing but when you're into tens of thousands of lines, I don't think a few thousand more or less makes it any more or less welcoming at that scale 🙂

I should also mention, SteamTinkerLaunch is larger than 18k lines, are you maybe looking at an old version? It is currently 26k~. Make sure you're pointing at master 🙂 I am working to reduce it through removing dead/unmaintained code (#1029) and general code improvements, rather than splitting anything out into separate files, which could end up being problematic.

Reading the STL bash code it shouldn't be difficult to split in various files

This is something I considered, and as far as I recall so did the previous maintainer, but it is risky.

I think it may be more tricky than you might think, for various reasons (it certainly had more to it than I thought at first). The main one is the fact that this script is ran by Steam, and there are limitations with what compatibility tools can do across files. This is why, for example, Valve's Python proton script (which is what Steam runs when you run games with Proton) is in a single, large file. Of course another reason is that SteamTinkerLaunch originally started as a much smaller project and eventually grew in complexity.

Instead of splitting SteamTinkerLaunch into files, I'd like to reorganize the existing code; removing some duplicated functions, shifting to a more "exit early" paradigm (a lot of code that is 3+ years old has a lot of things nested in if conditions, which I am working to change).

In short, this is just generally how Bash projects are as it is easier to share data and allow commandline usage this way, and also to avoid limitations from Steam (a lot of data we use also comes from the Steam environment and sharing this between files could be tricky).

I would like to help on those tasks but some guidance or a documentation for that can help others to join the project.

If there is anything that can be improved in CONTRIBUTING.md let me know, but I think the misunderstandings here may just stem from being new to working with a Bash project. This section I think should help getting started:

SteamTinkerLaunch is basically a huge Bash script. If you know any Bash, feel free to jump in and look around the code. It might seem a little daunting at first but just play around and you'll get the hang of it. I also recommend reading through the steamtinkerlaunch.log file. The log for the most recent launch can be found in /dev/shm/steamtinkerlaunch. This can give you a sense for the flow of the script and how all the functions connect together.

You did this a bit in #1052 to find the launchCustomProg function, so you can expand this idea to get to grips with how the functions are strung together.

This is how I got started with contributing, way back in #404 (before I even really knew much about Bash!) - I didn't start this project, I began as a contributor, so I remember what it's like to come at this fresh. If you have experience with writing Bash then you're a step ahead of where I was already!

Also on my debian sid machine with kate and bash-lsp configured crash the computer with 16gb of ram because the file is too huge...

This doesn't happen for me with Kate, if the Bash language server is using ShellCheck (vscode plugins often do) it could be that you are affected by koalaman/shellcheck#2652 - An issue not exclusive to SteamTinkerLaunch and not even necessarily specific to file size. If ShellCheck 0.9.0 is being used, see if you can manually download 0.8.0 and point your language server to use that binary instead. If it's taking it from your system or installed as a dependency, that is tricker to get around if there are no configuration options available.

If this bash-lsp/bash-language-server project is what you're using, then it looks like it is indeed using ShellCheck, and likely the latest version, which does not work with various scripts including SteamTinkerLaunch, as per the linked ShellCheck issue and its associated discussion.

I expanded some of the initial Steam Deck improvements on a Steam Deck way back (#629), so I don't think the large file size is the problem at all.

Fwiw this issue and a workaround is also already noted in CONTRIBUTING.md.


So I think the main things for this issue are:

  1. How to contribute? - Jump in and get your hands dirty :-)
  2. Where are the SteamTinkerLaunch builds? - There are none! It is a Bash script and you simply run the script to run SteamTinkerLaunch, similar to other projects such as Winetricks.
  3. Where is the CI? - There is no CI yet, although eventually I'd like to integrate ShellCheck once we can run a modern version.
  4. Could SteamTinkerLaunch be split into other files?- Possibly, but it would be tricky, and we may run into problems with Steam integration if we go that road. Improving some of the code architecture to be a bit cleaner would be better (Winetricks, for example, is a gold standard imo for architecting a large shell script).
  5. Kate is crashing - This may be because of ShellCheck, as on my machine, it loads fine.

If there is anything I've explained here that you think could be explained better in CONTRIBUTING.md, please let me know, I'm happy to improve it and make it clearer for how t o contribute. As mentioned, I started as a contributor, and tried to explain things in the document from my perspective going from contributor to maintainer.


And, if you're looking for somewhere to contribute that would make a large impact, the main areas I would suggest are:

  1. Anything to do with SteamOS - Seriously, anything, things on Steam Deck Experience Improvements [Community Contributions Wanted] #859 or otherwise related to SteamOS support. Something as small as a typo to as large as fixing longstanding issues such as the constant (and mostly not working) updating, or automatic dependency updating. I would only ask that when submitting PRs you attach a video briefly demonstrating your changes, since I cannot test on SteamOS. I will happily give code review though, of course :-)
  2. Any bug reports that I was unable to reproduce, such as Compatibility issues with bypassing launchers via custom command #968 or Problems with STL, Proton-TkG, and Winesync (feat. God Eater: Resurrection) #913.
  3. A big win adjacently related to SteamTinkerLaunch would be to help me figure out how to portably package (i.e. as an AppImage so that it can be easily downloaded and run on SteamOS) my DumpSteamCollections project (written in C++), which is needed to get Steam Collection information and fix Non-Steam Game Categories Not Working #949 (more background is in that issue and in the corresponding roadmap for v14.0).
  4. Any documentation improvements. Sadly I don't believe GitHub allows wiki PRs, so you can open an issue detailing the problem and what should be changed and how, and it can be implemented, or at the very least discussed. We're partially doing that here, with discussing information in CONTRIBUTING.md, although you can actually open PRs against those kinds of documents.
    a. Something a few people have missed is the sidebar on the wiki, which has quick links to various wiki pages.

There isn't very much I would say to "not" work on, but the main things I'm planning to take a look at are #949 and #960 (although helping to package DumpSteamCollections would be a massive help related to #949!). Everything else including things on the v14.0 roadmap would be good places to look at for big helps!

This is mostly underlining what is already in #992, which was created and pinned to give insight into what is upcoming, and to ask for help. For SteamOS, #859 was made solely to encourage contributions and give ideas for what to work on, because all SteamOS support going forward will have to be contributed by the community. I don't have the time or desire to maintain any other community channels, so making and pinning GitHub issues is the best I can do (I stay off social media for the good of my health these days).

@Mte90
Copy link
Contributor Author

Mte90 commented Mar 13, 2024

Thanks for the various explanation, I was thinking of a compile step because there is a UI.

About my idea that is a huge files (I am able to see 18000 and kate will crashes after that...), anyway I understand the bugfix on shellcheck (I use it too).

The problem to that the project as it is now it is very difficult to contribute to because of the huge file. Proton it is a single file but it is 1500 lines, we are talking about 26k lines.
I work too on huge bash projects but to simplify contributions we split the various stuff in different files (and focused) so it is more simple also to do refactoring (and avoid this kind of issues with external tools, it is common that LSP servers crash with huge files).
Consider that the case of VVV https://github.com/Varying-Vagrant-Vagrants/VVV/, we have various bash script focused, and external repos with specific scripts for any tool we integrate, so if I need to do a bugfix or a feature for php 8.2 I know that there is a file there of that purpose with everything on it and I don't need to check a huge file.

Consider that usually I do patches everywhere and a PR but if the project is difficult to study and understand is not welcoming, and people doesn't help you.

I am just inviting you to evaluate to organize the code differently so it is more easy to test/develop it locally also for people that doesn't know the project like you.
It is usually the golden rule for refactoring split and organize differently, bash script were built in that way because it is an old way of to do things but it is very simple to create dedicated script.

@sonic2kk
Copy link
Owner

sonic2kk commented Mar 13, 2024

I was thinking of a compile step because there is a UI.

SteamTinkerLaunch uses Yad for its GUI. All of the UI is done in Bash scripting by calling Yad.

The problem to that the project as it is now it is very difficult to contribute to because of the huge file.

I don't think this is really the problem. I mean, myself and others picked it up fine, and there are contributors do other similar Bash projects of a similar structure.

Consider that the case of VVV

This seems like a project of a different nature. SteamTinkerLaunch is entirely Bash, it's not a set of scripts that are orchestrated at a higher level. It is a single script, and Steam compatibility tools are much easier to develop with a single point of entry. If you have binaries, such as Wine, it's a different story, but for scripts it's definitely geared towards a single point of entry. To this end, this VVV project is not constrained to having to run as a Steam Play tool.

I am just inviting you to evaluate to organize the code differently so it is more easy to test/develop it locally also for people that doesn't know the project like you.

I think this is a good goal, and as I stated I am looking to do that without necessarily splitting the project into separate files. I don't believe this is really a blocker for contributions.

Others can and do contribute as well. Does this mean they wouldn't like to see things split out? Not at all, but because of the constraints we're working with, it's the path of least resistance. Things are much less likely to break on the Steam side. Others can and have contributed.

Also, I've essentially said this but I'll make it clear: I didn't make SteamTinkerLaunch. I think if I was able to come in and start contributing to the project with next to no Bash experience as a university student, others can too (and they already have as well).


Look, I'm not saying having everything in one file is ideal, but I don't think this is a blocker for contribution. I feel like discussing this is as productive as the discussions of porting SteamTinkerLaunch to another language; I'd rather this time be spent on contributions instead of dancing around the problem by discussing architecture. This is not directly related to how to contribute, and in my opinion, is looking for a problem to solve when there are explanations of what to implement first.

Maybe one day there'll be a fine way to split STL out into files, but I am not interested in this right now, and any work on this would at least have to wait until after v14.0.

If you have any more questions on how to contribute, feel free to ask and I will answer!

@Mte90
Copy link
Contributor Author

Mte90 commented Mar 14, 2024

I am trying with a tiny patch based on the steam deck ticket.

The experience for me in a single file is not very good but as you mentioned there are other priorities.

@sonic2kk
Copy link
Owner

sonic2kk commented Mar 14, 2024

I totally understand, and as I said, it isn't ideal but there are constraints and other priorities. Maybe after v14.0 goes out I'll make a higher priority of refactoring, although having multiple files might still be a bit of a pipedream.

We're constrainted by Steam but also by using the older compatibility tool manifest, as a program like SteamTinkerLaunch isn't compatible with manifest v2 last time I checked as it enforces the script to run in the Steam Linux Runtime container and thus limiting access to a whole lot of system binaries such as GameScope or scripts like Winetricks. By design the container does this ofc, but it means enabling systemwide GameScope wouldn't work for example -- and symlinks don't work either, tested that for Winetricks a while back to see how limited we'd end up being.

I appreciate all contributions and I'm not trying to be hostile to feedback, I would still maintain though that even if file size isn't ideal I don't think it's quite the blocker. That's my opinion though, and you're totally allowed to feel that it's not a great experience, I hope I've helped justify a bit more of the "why".

I would like to get the script size down eventually, fix up some legacy code, and remove some unmaintained code. After that, experimentation with splitting some things out could be done, but I think we'd hit rocky waters down the line with Steam. Keeping in mind, the script is what Steam itself runs instead of a game launch command, so we would have to be very careful. But if we assume this kind of refactor is feasible, I still think improving the codebase and then breaking it apart is the way to go, so we have better code to split out.

I would also like to underline that, in general, I agree with you that it's better to structure a project with multiple files. I work full time as a software engineer, and contribute to some other projects (mainly ProtonUp-Qt), it's better for maintenance in the vast, vast majority of cases. SteamTinkerLaunch, and Steam compatibility tools in general that aren't just binaries, are under some different constraints though.

@Mte90
Copy link
Contributor Author

Mte90 commented Mar 15, 2024

Another feedback to me is instead to have a ticket with various big task, like #859, it is better to have distinct issue so we can discuss on topic and not mix them.

About the rest 🤘

@sonic2kk
Copy link
Owner

sonic2kk commented Mar 15, 2024

Another feedback to me is instead to have a ticket with various big task, like #859, it is better to have distinct issue so we can discuss on topic and not mix them.

That is fair. I guess I tried out the bigger initative-style issues and they didn't work, at least for things that I expect the community to give back on. In future I'll split them out into smaller issues where appropriate :-)

I guess this issue is the best place to bring it up: Thanks for the discussion on contributing, and also for taking the time to dive in! I appreciate all your PRs. It's very motivating to see someone willing to help out, it can be easy to get burned out working on a large project like this while dealing with a lot of very unhelpful users asking the same duplicate question 10+ times (yeah, actually happens, I get the same MO2 complaints over and over since v2.5.0 came out 😓).

@sonic2kk
Copy link
Owner

The language server related issues may be resolved now that ShellCheck v0.10.0 is out and it allows you to disable the extended data flow engine, and SteamTinkerLaunch now does this (#1061).

I think we've had good discussion. If you need any more help with contributing please feel free to open an issue. As some things like #859 are all in one issue, feel free to open dedicated issues if you need explicit help with anything. I will leave #859 open as more of a "tracker."

This issue has had some good outcomes, and I think the question of contributing in the broad sense that was raised here should be answered, and further questions can be asked more specifically in their issues/PRs (it's totally ok to open a PR and ask for further help btw, see #649).

I will now close this issue, but feel free to re-open if you think there's anything more to add here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants