[PORT] Swipes Most TG CI Changes From The Last Year [MDB IGNORE] #453
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## About The Pull Request Hey there, It's on the tin, we just cache the whole Rust installation to use over multiple workflows. Without this, it would take up to _five minutes per linters workflow run_ to install the whole fucking rust toolchain/crate dependencies (ref: https://github.com/san7890/bruhstation/actions/runs/3536980337/jobs/5936542571 ). Now with the cache, it's now down to approximately one minute, thirty seconds after it's able to pull a cached rust installation (ref: https://github.com/san7890/bruhstation/actions/runs/3537002138/jobs/5936585526 ). More than half is very good, and frees up valuable Github Runners slots sooner! I've also always seen linters as that rapid check to immediately call you out whenever you screw up in a PR, and it taking four minutes sorta lessens that immediate: "HEY, LISTEN!" aspect
…ache has never worked (#71529) ci_suite.yml runs on your fork. This means you do not have access to secrets. Every user has had the purge key of blank. WE have it set to something. Which means the master cache that every PR pulls from has been unable to match. This means our cache has been at the max limit all this time, constantly clearing out old caches, and never reusing any.
…structure, and fixes the massive amount of failures that were never caught (#72372) [Documentation here](https://github.com/Mothblocks/tgstation/blob/maplint/tools/maplint/README.md) We should not be using greps to the capacity that we currently are. If you are not smarter than a parser, then you should not try to beat one. DMM files should NOT be treated as text files that can be parsed with any old Unix tool. They are a structured language. Because of our abuse of greps, check_greps is full of hard to read, hard to maintain checks that do not consistently work, because they all make very specific assumptions about how TGM works, which are provably untrue. This format is mostly straightforward for the lints people write, and easily extensible to the ones people will want to write. :cl: fix: Fixes a bunch of cases of windows not being where they were supposed to be, tables/chairs stacking on each other, and other very small stuff you've never noticed before. /:cl: # Conflicts: # .github/CODEOWNERS # _maps/RandomRuins/SpaceRuins/spacehotel.dmm # _maps/RandomZLevels/SnowCabin.dmm # _maps/map_files/CTF/cruiser.dmm # _maps/map_files/IceBoxStation/IceBoxStation.dmm # dependencies.sh # tools/ci/check_grep.sh
## About The Pull Request I was planning on doing this a lot earlier but ran into some weird roadblocks, but this time I finally did the research and it's done properly. We do a lot of useless work on every single CI run, and in the interest of saving the world's energy (by a matter of at least 10 seconds per my testing), lets stop doing so much extra work by caching both the work we do on the python bootstrap installation (we literally call it `cache` for a reason) and the Node installation by sharing it between github actions runners. Here's a random CI run on master where you can see that the `Install Tools` portion takes about 19 seconds - https://github.com/tgstation/tgstation/actions/runs/6167104927/job/16737570519 Here's the CI run I was testing with where you can see we slim it down to about 6 seconds for the `Install Tools` step, but with a second-or-so taken to restore the cache since the runner needs to download+unzip the cache. it tends to be shorter or longer by a second at times but i'm certain this is just noise - https://github.com/san7890/bruhstation/actions/runs/6167245722/job/16737919874 On average, we save about **10 seconds** a run on Run Linters, which is meant to be the fastest CI step. Future improvements would lie in making either maplint or the tgui test suite faster, but that would be a much more complicated and code-intensive task. cache go weeeee ## Changelog Nothing that concerns players.
## About The Pull Request Ensures we don't get a repeat of #76345 (unit test that wasn't ticked in the `_unit_tests.dm` file, fixed in 596ca8b6d4cc49cd69fc104b53b7f4973497a2e5). Basically, we leverage the code that was already being used in the DME Validator but then expand it a bunch via using JSON Schemas that correspond to the type of scan we want to run. Even though sorting unit tests alphabetically is a bit different than sorting the tgstation DME, it's good to leverage the already existing framework rather than create a copy-pasta "lesser" code runner. This went through strenous testing on my end, so let me know if anything seems off. While in the area, I added some other niceties that I've found work really well in GitHub Runners environments, as well as local testing in case you really like doing that before you make a PR for some reason. ## Why It's Good For The Game  This is what it looks like pre-596ca8b6d4cc49cd69fc104b53b7f4973497a2e5 (this is now merged, so it will pass CI) De-hardcodes some stuff and allows for some neater flexibility, less cringe unit tests being coded and not being ticked in the file, etc. etc. ## Changelog Nothing for players to care about. Let me know if you have a better idea than the schemas, I couldn't think of one that could be really extensible and flexible in the same way this is. # Conflicts: # .github/workflows/ci_suite.yml # code/modules/unit_tests/_unit_tests.dm # tools/validate_dme.py
## About The Pull Request The tool added in #72372 is pretty awesome. The output is uhh cryptic though. I had to read the source code to realize the (line 382) or whatever part of the message was the dmm line number and there's stack traces everywhere. I've made it support github action error messages so now you get this beauty if you mess up:  Or, in the run summary:  Errors parsing the lint yml's will also output github action errors, although the line number will always be 1 since the yaml parser discards line numbers to my knowledge. In the midst of doing this, I made the error type contain the file and line info, and added a bunch of type hints in the midst of trying to understand Mothblock's code. Note that for power users, the default behavior is still colored terminal text; `--github` is added by the CI suite to enable this behavior. ## Why It's Good For The Game Much easier to see where the errors are and what they are (who even knows what a 'pop' is? The tg game code calls them grid models.) ## Changelog Nothing player-facing. # Conflicts: # .github/workflows/ci_suite.yml
Probably lets us use merge queue. I would be doing this on a fork but I don't have access to the beta 🤷♀️ Pull request for the sake of downstreams but I intend on self merging this (and my followup test PRs, though to another branch). Will start with commit queue on my project branch. Should be noted that I predict flaky tests might hit us harder, since it might stop the entire queue? I'll see
Checking if this fixes merge queue. This ideally doesn't pass if CI is cancelled, which is a requirement right now for merge queue to work because reasons Also: 
More merge queue bullshit. Cancels are counted as failures, and even though on my test branch it worked completely fine, when trying on two real PRs, it did not. This makes it so merges into master might mess with CI clogging again, but also merge queue is going to do that on its own, and the gain is worth it. 
…we want that (#74364) todo: (i'm going to bed or i'd do them before opening the pr) fork the action over to the org ~~make this more configurable as mothblocks requested~~ ~~add this to the windows build and integration test steps (if it generates a dmb, we gonna validate its got the expected value for client compatibility)~~ ~~test 515 client required fails.~~ https://github.com/MrStonedOne/byond-client-compatibility-check --------- Co-authored-by: Mothblocks <[email protected]>
## About The Pull Request In #74364 (e0ac16102974fadcb5be625e01bb39101639cda0), we wanted to use the fork for this action rather than MSO's version. However, we only updated the changeover to the `tgstation` version in one location, not all occurrences in our CI suite. This PR cleans that up real fast.
## About The Pull Request This PR removes the "Annotate Lints" job step and merges it with the "Run Linters" step above. To achieve this, I wrote a python script that should be identical to the action. I even added the ability to read the output from a file to the script if we ever needed that, but I decided to have the job step pipe the output into the script instead. ## Why It's Good For The Game It always bugged me a bit that we had to check the results for a separate step if we wanted to see the linter results for dm code. I also noticed a few people getting confused as to why their CI failed on linters. Turns out that the action is just a few lines that match the dreamchecker output and reformat it to a format that GitHub can annotate code with. It's so brain dead simple that it shouldn't need to be a whole new step, and for the previous two reasons. ## Changelog not playerfacing
## About The Pull Request Splits the big "Run Linters" step into multiple steps. Also since all of these steps are independent of eachother, I've made them all run regardless of if the job is currently failing. <details> <summary>Proof of testing:</summary> Fail in install tools, all linting steps are skipped: https://github.com/distributivgesetz/tgstation/actions/runs/6151628214/job/16692089726 Fail in Run DreamChecker, other steps continue to run: https://github.com/distributivgesetz/tgstation/actions/runs/6151664705/job/16692203569?pr=2 </details> <details> <summary>Pictured: me breaking CI for a day</summary> https://github.com/tgstation/tgstation/assets/47710522/ea12ad30-2b69-4aa3-9642-7d0818eab2d1 </details> ## Why It's Good For The Game Going through the Run Linters step has always been a slog. Finding an error is like finding a needle in a haystack. Seeing what command exactly went wrong is going to go a long way in helping people find out which linters have failed. ## Changelog nothing playerfacing # Conflicts: # .github/workflows/ci_suite.yml
> `bash`/`sh`: By default, fail-fast behavior is enforced using `set -e` for both `sh` and `bash`. When `shell: bash` is specified, `-o pipefail` is also applied to enforce early exit from pipelines that generate a non-zero exit status. it helps when I actually read the docs
## About The Pull Request few errors which were odd and annoying ## Why It's Good For The Game stealing PRs from san7890, they wanted to do this ## Changelog nothing playerfacing # Conflicts: # .github/workflows/show_screenshot_test_results.yml # .github/workflows/tgs_test.yml
github-actions
bot
added
Continuous Integration
RimiNosha
changed the title
…rep file (#71324) ## About The Pull Request I fucked up the check_grep entry for docking ports. default grep doesn't support newlines and so I need to use `pcregrep -M` ## Why It's Good For The Game linters working good ## Changelog Co-authored-by: Mothblocks <[email protected]> Co-authored-by: san7890 <[email protected]> # Conflicts: # _maps/shuttles/whiteship_obelisk.dmm # _maps/shuttles/whiteship_personalshuttle.dmm # _maps/shuttles/whiteship_pubby.dmm # code/modules/projectiles/guns/special/syringe_gun.dm # code/modules/reagents/reagent_dispenser.dm # tools/ci/check_grep.sh
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
About The Pull Request
Tin. I was starting to port iconslicer, and decided I wanted these too.
Tons of CI changes, so I might be spending a bit on fixing this stuff.
Should speed up CI time by like 3 minutes, if all goes well.
https://github.com/tgstation/tgstation/commits/71b45e54adfaa4c681babc545db97fa7103289de/.github/workflows/ci_suite.yml