-
Notifications
You must be signed in to change notification settings - Fork 72
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
Comments
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... |
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.
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 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).
The Makefile is used to install and uninstall SteamTinkerLaunch to your system directories, such as
Perhaps the wording is unclear, "Development Build" is the wrong phrasing, in this section I am referring to testing your local script changes.
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.
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.
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 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 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).
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:
You did this a bit in #1052 to find the 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!
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:
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:
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). |
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. 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. |
SteamTinkerLaunch uses Yad for its GUI. All of the UI is done in Bash scripting by calling Yad.
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.
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 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! |
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. |
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. |
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 🤘 |
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 😓). |
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! |
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.
The text was updated successfully, but these errors were encountered: