GAME_CI_BUILD_SUCCESS and GAME_CI_STEP_SUMMARY #576
Conversation
|
|
| } | ||
|
|
||
| // Check for presence of game-ci message(s) | ||
| const gameciMatch = result.stdout.match(/^GAME_CI_BUILD_SUCCESS$/ms); |
There was a problem hiding this comment.
I really like the idea of having a very simple message to check for. We should also document this to make sure, people using this for non-build stuff are aware.
Additionally, I'm wondering if there could be use cases, where the rather explicit GAME_CI_BUILD_SUCCESS should have a higher priority than Build results (You want to have the job succeed even if the Build results contain an error).
There was a problem hiding this comment.
See comment #576 (comment)
| const errorMatch = buildResults.match(/^Errors:\s*(\d+)$/m); | ||
| if (errorMatch && Number.parseInt(errorMatch[1], 10) !== 0) { | ||
| throw new Error(`There was an error building the project. Please read the logs for details.`); | ||
| } | ||
| } else { | ||
| throw new Error(`There was an error building the project. Please read the logs for details.`); | ||
| await summary.addHeading('Build Results').addQuote(unityMatch[0]).write(); |
There was a problem hiding this comment.
Should this be logged before the exception? Otherwise you wouldn't see how many errors/exceptions there are, if you have any.
|
Having looked at this a bit more (prompted by the documentation part of #574), I wonder: why have this post-run parsing at all? To my understanding what’s happening is this
What I didn’t realise when fixing this behaviour for my custom build method, which pre-dated knowing about GameCI and was written from scratch, was that the default build method is GameCI’s, not Unity boiler plate. Or at least, even if it is Unity boiler plate, the example referenced in the documentation is hosted on GameCI and so under our control. So why not adapt the GameCI C# build method? This has the following advantages
There may of course be a reason why this wasn’t done, please holler if so. What would remain is having some kind of flag in the workflow spec for the default build method that specifies whether errors should fail an otherwise successful build. To do so, I see the neatly factored switch (result)
{
case BuildResult.Succeeded:
Console.WriteLine("Build succeeded!”);
if (BuildSummary.totalErrors > 0 && FailOnError)
{
EditorApplication.Exit(1);
break;
}
EditorApplication.Exit(0);
break; |
|
One clarification – GAME_CI_STEP_SUMMARY is still wanted! So the javascript parsing would still be there, just not futzing with the exit code. |
|
One further thought – though, doing |
I think moving this behavior from the build parameters to the c# script would be a very nice move to make this logic more understandable. As an end user I do look through the script (and also adapted it already), but haven't up to that point cared too much at what happens in the typescript scripts. This change would help for more transparency, while making the api less bloated.
I would prefer to keep it that way, since it is not breaking anything and in the meantime allows the default script to also work with |
|
Thanks. The plan, then, would be this?
I’ll do that with an OK. |
@davidmfinol @AndrewKahr @frostebite @GabLeRoux @cloudymax any thoughts or comments? |
|
I don't have much free time this week, but I will look into this as there are quite a lot of related issues, I saw a few related help threads on discord as well: I've updated description to point to a few issues that this should help closing and will look into details later this week 👍 We definitely need to improve on error reporting. Something I had in mind would be to expose the errors found in the logs as warning instead of failing the build directly. |
|
If you can take care of this in a quality way we'll try to support this and get it in after you've made the changes. We do need this to be tested for the different scenarios. |
|
I'd really like to be able to opt out of the behavior. In my case, Unity outputs an error that I cannot work around, but the build is otherwise successful. A configuration option would be simple enough, I think. Trusting Unity's exit code should be sufficient for many cases. |
|
Just wanted to tag in some context on why this got added. What we noticed was on WIndows, the error codes didn't seem to be propagating out correctly and thus failed builds would still cause the workflows to appear as succeeded. So this parsing was added to check for errors so that it would fail the workflow correctly. The assumption we operated on was if there was an error, the build was probably bad as I had seen builds get made but had an Error and while it opened, there would be stuff wrong with it. But perhaps that was too broad an assumption. So what I see as the options would be to either ensure that the exit code actually works across all 3 platforms or the plan stated above. Though, with regards to the plan stated above, if you set it to not fail on error, how does it know to throw an error if the build actually fails to stop the pipeline? But definitely open to getting a solution to this. We're gearing up for v4 of the actions so this is probably an ideal time to get something big like this into the release. |
|
@AndrewKahr To be clear, where you say “error codes didn’t seem to be propagating out correctly”, do you mean 1. a success process exit code happened when Unity’s build method had returned a report containing errors, or 2. a failure process exit code happened but that somehow wasn’t propagating and the e.g. Javascript action code thought it was a success exit code? If 1., then the error-parsing is not being removed, just the behaviour being straightened-out technically and to not make assumptions about custom build methods. If 2., then that’s new to me and... hmm. There’s still the base need that motivated this in the first place, but the right solution may take a bit more thought. All – I was away, but am back now and will get to this. |
|
To be honest, I'm not sure exactly what was happening. All I know for certain is Unity would fail to build something, which on Linux would produce a nonzero exit code and then the pipeline would fail out, but on windows it would continue as if it succeeded tho I'm not sure if it failed to produce an error code or if it failed to propagate it out. So you might need to do some debugging. If you use Unity-Builder@main it will use the new images and build scripts which might have different behavior as well. |
|
It would be 🔥 if the build errors were surfaced to the GitHub Action via the annotations feature: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions Right now you just get this: Seems like if they're available to the script doing this work either in C# or Shell it would be relatively straightforward to emit them so you don't have to dig through the enormous log file usually. |
That kind of thinking is what’s behind
|
|
Rather than parse, might be worth having it be part of the default build script in Unity so it can tap into the Unity log system and know exactly what is an error/warning. Found this thread that might help: https://discussions.unity.com/t/detect-compilation-errors-in-editor-script/74474/4 I've previously thought about how to parse for errors and there's just so many ways it can throw an error that isn't immediately obvious and creating a parser for all of them would be practically impossible. Certain errors are definitely easier to see as it might say error: or exception, but other times a plugin can throw an error but not explicitly have the word error in it (ie file not found or unable to do xyz). And sometimes it will print messages that sound like errors but actually aren't. So I would highly advise not going down the parsing route due to the infinite possibilities for what is/isn't an error and leverage Unity as much as possible if this is a desired feature. |
|
I think you missed my point about the parsing. The problem I hit is once discovered how to get annotations out of Unity code and into the GitHub Action itself. The problem is that Unity is running inside a docker container, so the method for scripts to communicate annotations won’t work (writing to host file system at a location provided by env var). Hence parsing the log output for Your goal is good, this is a technical detour on how to get there. Disclaimer – I’ve only used my own custom build method, so all I currently know about the Game CI build method is what I found by looking for error code handling. So there’s some discovery I’ll do. |
|
Ah apologies I misunderstood the goal. From what I'm reading, it doesn't look like annotations need the env files do they? For example this is their code for error: If as long as we print out in that format via either debug.log or calling echo from c# code, is that enough to get it to annotate or am I missing something? If we just need to be able to access the GitHub env and output files, from what I understand those are just files and the environment variable is a path to it. If in the docker.ts file where we mount relevant files/folders, what if we just mount the env and output files so the container can access it? And add those env variables to the list passed into the container so we know where to look since the file names are dynamic. Then you in theory can do everything in the container right? |
|
I expected that kind of “transparent container” experience when I first used Game CI, e.g. I was surprised environment variables set in the workflow step were not available within Unity. But on suggesting that, was firmly rebuffed on design grounds. That’s not my argument to make, what I’m drawing on is here. But I have now sympathy to the idea to just saying no, rather than special casing an increasing list of things, every time someone gets confused. There’s a decision here. What I’ve got already implemented for annotations is a small tweak to the existing code here. But that whole log parsing step could be eliminated by configuring the docker container (assuming the paths are mappable, and maintaining it all when GitHub change things). Caveat – assuming we don’t need to parse the logs for errors as per Andrew. i.e. that the Unity build results object and exit codes are sufficient |
|
Procedural note – this is where splitting a PR would make sense, so one piece of functionality isn’t delayed by another ( |
|
I had a look at the thread you mentioned and the key idea there was the environment variables automatically passing through. What I am talking about is adding the I also did an experiment yesterday and was able to create annotations with just |
|
Probably useful to also make this work out of the box for people who use their own custom script. We either need a clear snippet to add to people's script or take the parsing out of the container. In the case of the latter it would be better to run it from JS and not bash, as we're trying to get rid of as much low level scripts as possible. Personally I think we shouldn't add this to the custom build script but in the JS logic, unless parsing proves to much more cumbersome than doing it directly during the build. Passing the information from the build script to outside the container might be a fair option as well. |
|
I think it makes the most sense for the annotations to be done via build script in Unity than via the js action. The build script option is only 20 lines in a script the user needs to drop into their project (or automatically get it if they use the built in default build script). If we do it via js, we'd need to manage an endless amount of parsing formats to catch variations of warnings/errors and will probably be a long term pain as we aren't depending on an API which should remain stable long term. We could technically figure out a way to pass the info out of the container to the js, but I think that is a lot of unnecessary complexity and means we need to create and maintain our own API to pass stuff out rather than just letting people implement against APIs for their own CI systems. |
|
Fair enough, sounds good. |
|
Just wanted to give a heads up, I've merged some fixes for license activation issues in addition to annotations for errors/warnings so make sure to merge main into your branch. That should be a nice baseline to work off of for the annotations your are looking to do. |
|
I was thinking a bit more about the exit code on Windows and did a deep dive into the issue and I think I've solved it so exit codes appear to be flowing out now. Thus this PR probably isn't needed anymore as the logic for build error is back to checking for a 0 error code which aligns with unity-builderv2. We do really appreciate the work that was put into this PR though, it ultimately has led to some really nice QOL improvements so it really is a net positive overall. After we get the changes released and it's clear the issue is in fact solved, we will close this PR but in the meantime, we can keep it open. If there's something not covered by the other PR that this one improves on though, definitely happy to merge those other improvements. |
|
That all sounds great. Relying once more on exit codes being canonical is both how it should be and solves my need. And selfishly, I’m happy you did that Windows deep dive, as it isn’t my speciality; I was concerned about custom build methods. (And hadn’t picked this back up yet as I’ve been on the cusp of getting back to my CI task for work, but emphasis cusp there) Being able to output GitHub annotations without any Game CI special casing is also... how it should be. Though some documentation will be required to say while the docs give these options, only do it this way. Given at least I read their docs and tried a method that doesn’t work. |
|
Closing this now as it looks like the fixes for v4 should've solved the build issues |
Changes
Seemingly a breaking change in v2 → v3 is the action reporting error depending on the parsing of Unity build results in the log. Not all Unity builds invoke a player build and so this output cannot be expected. Examples of people hitting this issue can be readily found in the issues tab. The discussion in #563 resulted in the user faking these results in order to be able to use the v3 action. Better would be an explicit success message Game CI could check for in these circumstances.
Further, there is an issue that the Unity build method cannot output step summaries, as the required file is on the host machine not docker container where Unity is running. As there is already code parsing the log output, adding a Game CI Step Summary token alongside the success token would be a conceptual fit and technically straightforward.
This PR adds the following
GAME_CI_BUILD_SUCCESSpasses, thenGAME_CI_STEP_SUMMARYare set as the step summary and the action completesGAME_CI_BUILD_SUCCESSThis expands relevant functionality (if you’re using a custom build method there’s a good chance you might have reason to want to summarise what happened in a successful build) and gives actionable information that wasn’t present before in the fail case.
Checklist
code of conduct
a PR in the documentation repo)
Related issues