Improve CI experience for contributors

[lawnjelly]

Godot version:

N/A

OS/device including version:

N/A

Issue description:

Getting large PRs to pass CI checks can be overly involved and time consuming. I'm probably the most vocal on this subject (sorry! :grin: ), I suspect some others are too polite to mention, but being an impatient old guy I thought I'd put in some experience from a user perspective and some suggestions and have a rant. They may not all be actionable / best option, and I may well be flat out wrong in some areas.

The problem is mainly that it is currently difficult to run the same checks (that are done by CI) locally. They need not necessarily be impossible, just difficult is enough to prevent use (if you have to install docker + a dozen tools it isn't going to happen). So you often get caught in a loop of

1) push PR to github 2) wait for significant amount of time (up to 30-40 mins) for CI to find an error 3) fix error 4) return to step 1

With a small PR, this isn't a massive problem. But with large PRs, this process simply is not efficient or practical.

Hooks

To try and make this easier we have hooks. But in my experience these involve installing a bunch of other software, which must be the exact correction version in some cases (e.g. clang-format), resolve conflicts with the OS, and get these to run, only to find that the CI may do extra checks that these hooks don't seem to cover (like formatting within comments).

Aaronfranke suggested that the CI produces a patch file, but I have no idea where this is or how to use it (instructions?). On top of that, given that compilation stops on the first error, I don't think it helps much if you have e.g. 1000 errors to work through. Of course you can turn warnings to full locally, but some compilers that you may not have access to (e.g. cl.exe) may produce different warnings to gcc / clang.

Tests

On top of the formatting issue, we also now have unit tests and testing via Qarmin's project which is needed to pass CI.

If it doesn't pass, the problem is that this testing is done at the end of CI (sometimes after a ~35 minute wait), and these failing these tests can be difficult to resolve with the limited information provided. The solution I ended up with was to figure out how to run the test project locally, otherwise I would have simply never found the bug in question, given up and chosen a different hobby!

Some suggestions for improving the process for contributors

Limit formatting tools

Don't require a long list of formatting tools to be installed for the most common use case, c++ code. Really you should pick a tool and version - e.g. clang-format 10, use the features provided by that tool only. If it doesn't offer such and such feature, to do just the right amount of tabs in such and such a case, simply don't use that feature.

The amount of style checks are now bordering on pedantic. My rule of thumb would be - if you had to explain these checks to a psychiatrist, and he would offer you prozac etc, you are probably going too far.

We as programmers tend to be very OCD (I know I do), but the trick with compulsive behaviour is to notice when it is becoming a problem and that you are being irrational, and reign it in. Style checks should be there to make things easier, not to make things more difficult. At the moment we are in a situation where they create more problems than they solve. So while style checking is good - it is essential to make them practical and easy to use.

Maybe we could cover all cases by a combination of specified version of clang-format, and a custom tool written in c or python or some such so that it was easy for users to install / use. Maybe we can do everything with clang-format, that would be even better.

Testing

We should have detailed instructions so contributors can download and run tests locally. I worked out from the github action that I could build with sanitizers and use qarmin's test project. But this should be in the documentation for contributors, step by step instructions to do this kind of thing. Treat us as morons, and you won't be far wrong. :grinning:

Compilation - warnings as errors

A real issue is that the different compilers in CI given different warnings, and thus errors. So a clean run on your own machine doesn't mean you'll pass CI. One possibility is to limit the CI warnings as errors check to one compiler / version / platform only. This means that, provided everyone on windows and linux could install that compiler, everyone could potentially guarantee to pass CI checks locally.

Warnings could still be important on another platform so this is not ideal. Another option is to compile without warnings as errors (thus creating a long list of issues that could be fixed in one go), instead of terminating the compile on the first error. You could then search string through the compilation output for a warning string, and use that to signify the CI had failed, instead of the current method.

Rule of Thumb

Overall, if I was deciding on such a system, the rule would be, if the check can't be reproduced simply and easily on contributors machines, it shouldn't be required to pass CI. This may not be completely practical in all cases, but the gist is good.

Anyway these are just some suggestions and viewpoints, hopefully to get some discussion / positive changes made.

[YuriSizov]

Overall, if I was deciding on such a system, the rule would be, if the check can't be reproduced simply and easily on contributors machines, it shouldn't be required to pass CI. This may not be completely practical in all cases, but the gist is good.

Sorry for picking a small note at the end to reply to, but I think this is kind of backwards. I'd much rather prefer a strict CI that would do as much as possible to prevent us from merging invalid code, even if, and maybe because, we can't simply and easily do the same checks locally. Of course it is desirable to automate the most of it before any commit is pushed, and in my experience pre-commit hooks are very easy to install (on Windows!) and catch almost everything, but I've never done a huge PR with tons of refactoring, nor do I use more advanced C++ technics, my contributions are pretty trivial.

Still. Making CI less strict and demanding just because we cannot overcomplicate local setups for contributors is a weird argument to me.

[Calinou]

One reason to have a strict CI is that it results in cleaner VCS diffs. It makes it harder for people to accidentally change formatting in unintended ways.

Don't require a long list of formatting tools to be installed for the most common use case, c++ code. Really you should pick a tool and version - e.g. clang-format 10, use the features provided by that tool only. If it doesn't offer such and such feature, to do just the right amount of tabs in such and such a case, simply don't use that feature.

This could be improved by migrating to pre-commit, which only requires a Python installation to work on all operating systems. It only checks modified files by default and handles partial commits nicely. In CI, you can run pre-commit run --all-files to make the checks run on all files. Here's the .pre-commit-config.yaml I use in my repositories:

repos:
  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v3.4.0
    hooks:
      - id: fix-byte-order-marker
      - id: end-of-file-fixer
      - id: trailing-whitespace

      - id: mixed-line-ending
        args: [--fix=lf]

We'll need to check whether there's a good clang-format hook for it though (or create our own).

[lawnjelly]

Excuse my noobiness, but is there a simple way we can have the local github actions in our local repository versions of godot (i.e. on github but in our own user folders) run slightly differently, like run without warnings as errors? This could be a potential solution to some of the problems? (i.e. you can then let it find 1000 warnings for you in one go, and you fix them all at once, and then a patch file could become useful)

I know I can turn on and off various builds for the github actions locally. If I modify the yml workflow files locally, presumably they will get picked up as part of the PR, which we wouldn't want (haven't tested this).

[Calinou]

Excuse my noobiness, but is there a simple way we can have the local github actions in our local repository versions of godot (i.e. on github but in our own user folders) run slightly differently, like run without warnings as errors? This could be a potential solution to some of the problems? (i.e. you can then let it find 1000 warnings for you in one go, and you fix them all at once, and then a patch file could become useful)

This is possible thanks to Docker, but only for Linux containers: https://github.com/nektos/act

You'll also need to specify an alternative runner image that weighs 18 GB to run most actions as expected: https://github.com/nektos/act#default-runners-are-intentionally-incomplete

[Zireael07]

@lawnjelly has a point. Most repos that have CI checks have them set up so they're easy to replicate locally (e.g. Cataclysm DDA uses clang-format and some tests, but all a prospective contributor needs to do is run a pre-commit hook and he's done, and there's an online linter to help, too) Godot does not, and if I were to contribute now, I'd be stuck in the loop that he describes.

[YuriSizov]

if I were to contribute now, I'd be stuck in the loop that he describes.

I would rather we not go into hypotheticals, because from my experience contributing to Godot it's exactly how you describe it should be: use pre-commit hooks and never fail with CI. So clearly, at the very least, your mileage may vary.

[lawnjelly]

I would rather we not go into hypotheticals, because from my experience contributing to Godot it's exactly how you describe it should be: use pre-commit hooks and never fail with CI.

but I've never done a huge PR with tons of refactoring, nor do I use more advanced C++ technics, my contributions are pretty trivial.

As mentioned in the OP, and perhaps I should highlight this, the problems mainly occur for people making larger PRs (e.g. GLES2 was 36K lines of code, portals is 4.5K lines etc). If you are only making small PRs, you are unlikely to suffer major difficulties.

[YuriSizov]

Exactly, which is why I've underlined my experience being different and why I say that the argument "I don't contribute, but if I did" by Zireael07 is not helpful.

This is clearly a very specific problem that very few contributors would face, and relaxing the CI is not the solution.

[Zireael07]

I did contribute before CI checks were a thing, and since I am unable to set up the CI locally, I would be stuck in the loop described because I can bet even a one liner fix by me would not have the correct formatting at first.

[YuriSizov]

since I am unable to set up the CI locally

We have pre-commit hooks which check for formatting. You can set them up with little effort, and all contributors are encouraged to do so. Am I missing something about your point?

[Zireael07]

Those same pre-commit hooks were reworked recently because something something was a PITA for Windows users...

[YuriSizov]

Yes, but as a Windows user I'm not aware of what exactly was the issue. And even then, that particular script can be fixed to work cross-platform, as evident from the discussion on the PR.

[Xrayez]

The discrepancy between treating warnings as errors on Linux/Windows is so big that I stumble upon CI errors in like 30% cases even for small PRs (I resolve most of them in my fork before submitting a PR now), it also seems like werror=yes works differently on my machine and GitHub Actions even for Windows builds alone, and I can't figure out what could be the cause for this.

So yeah, I greatly emphasize with the problem of having to set up the exact same configuration to fix a non-essential warning.

See also #40355 and further revert #40367.

And I'm not even talking about hook scripts which I have to update manually every time they are modified (there should be an easy way to install/update hooks).

[aaronfranke]

wait for significant amount of time (up to 30-40 mins) for CI to find an error

The static checks finish in like 2-3 minutes, less if there's a formatting problem. You don't have to wait for all of the checks to finish to examine the failure of others.

Aaronfranke suggested that the CI produces a patch file, but I have no idea where this is or how to use it (instructions?).

I would have assumed it was self-explanatory, but here are some screenshots (using 45263 as an example):

On top of that, given that compilation stops on the first error, I don't think it helps much if you have e.g. 1000 errors to work through.

Well, kinda, but in practice not really. It's not first error as in first formatting mistake, each script will run to completion then stop the job if there's an error and won't run the future scripts. However, there are already pre-commit hooks for clang-format and black-format, so if you have the hooks then you would only ever need to apply at most one patch.

Anyway, I'm just trying to say that it's not so bad right now... of course it can be better / easier :)

[tavurth]

I think this issue will affect many first time contributors, and sure once you know the ins-and-outs of any build system it's easy, but at the moment I feel like the documentation for the build system is not as complete as the documentation for Godot in general.

A better CI experience would keep things inline with how I feel when I make an issue for Godot. Issues are treated with enthusiasm, whereas the CI reacts with distain. There are many blockages in the processing pipeline, and it took me a while to figure them out. Who knows if more junior devs would even be able to try?

I think that if the CI is going to be very picky a good solution would be to state that in the docs somewhere, and have a FAQ for common build errors & team development flow linked to at the end of each Compiling for... section in the docs.

Even with a small change like this contributors could cut down their wait time by hours even for small PRs.

[Calinou]

and have a FAQ for common build errors & team development flow linked to at the end of each Compiling for... section in the docs.

Most users compiling the engine don't perform any changes to the source code, so this should be on a separate page. Nonetheless, the Code style guidelines page already documents how to install the Git pre-commit hooks and lists IDE plugins you may want to install as well.

[fire]

@aaronfranke Is it trivial to store a patch file that you can apply to the Godot Engine tree?

The use cases is if the CICD knows how to fix it, it can tell the developer directly and save it as a downloadable .patch file and we can explain how to use it.

The other cases are genuine errors.

[fire]

Secondly we should promote a workflow that uses https://github.com/nektos/act as a github ci simulator that runs on the local desktops.

# scoop install act
# Install docker
# Enter godot directory
act --job static-checks --bind -P ubuntu-20.04=ghcr.io/catthehacker/ubuntu:full-20.04 --verbose

[aaronfranke]

It already generates a patch file and prints it as explained above. Providing a downloadable file would just be a matter of storing this file and making it downloadable as an artifact. I haven't done this before so I don't know off hand how to do it.

[ghost]

Here is my experience having maintained a custom personal build of Godot for the past 2 months or so:

Static checks are way too strict to the point it is counter productive. I was spending more time pleasing the static checker than actually developing. For example my build stopped because I didn't have 2 new lines between Python functions. I don't think that's meaningful check. The problem is how is done it uses some kind of diff comparison which is related to the point below in that the static checks will always pass for those who made it and code that way ie build env dependency.

Zero warning policy although good in theory is really bad in practice as it creates compiler and build environment dependency. In other words project only builds for you. Ran into this enough times that I disabled werror=yes for gcc builds and got rid of static checker and a few other checkers.

Clang seems to handle errors much better and produces more meaningful errors and smaller and faster test builds and I moved to llvm wherever possible. Added use_llvm=yes and use_thinlto=yes and seems to produce a better CI log than gcc and small downloadable artifacts.

Also worth mentioning (although offtopic) that documentation of source code and build environment is sorely lacking. All you have is a one page starter guide then good luck! I had to read the source and look though PRs and meta discussions (such as this post) to learn.

[Calinou]

Here is my experience having maintained a custom personal build of Godot for the past 2 months or so:

For a personal custom build, it makes sense to disable CI static checks if you don't intend on contributing changes back. Removing https://github.com/godotengine/godot/blob/master/.github/workflows/static_checks.yml in your fork should do the trick.

Either way, personal builds are an entirely separate use case from contributing changes back to upstream. The former doesn't require to be strict about code style, but the latter does (and for good reason).

Also worth mentioning (although offtopic) that documentation of source code and build environment is sorely lacking. All you have is a one page starter guide then good luck! I had to read the source and look though PRs and meta discussions (such as this post) to learn.

Please open issues about this on the godot-docs repository (one issue per item to document). Make sure every item is actually actionable and not too vague. Otherwise, it's impossible to know what should be documented :slightly_smiling_face:

[Chaosus]

By my observation, CI build with Linux Builds / Editor with doubles and sanitizers (target=debug, tools=yes, float=64, tests=yes, use_asan=yes, use_ubsan=yes) takes 90% build time, I wonder if this one could be excluded...

[akien-mga]

By my observation, CI build with Linux Builds / Editor with doubles and sanitizers (target=debug, tools=yes, float=64, tests=yes, use_asan=yes, use_ubsan=yes) takes 90% build time, I wonder if this one could be excluded...

It's the most important build, as it does many things which are not tested by any other build:

• Editor build with sanitizers and double precision support
• Run unit tests
• Edit regression test project
• Run regression test project
• Compile godot-cpp library

[tavurth]

Perhaps a staged build would reduce the CI minutes spent per month?

For example, running the fastest test first would give users an idea if they missed something, without also pipelining the other builds which keep running even when that first one could have failed.

A staged CI could break once and then not run the expensive linux build.

Another suggestion would be that the builds don't seem to use any caching? When I rebuild on my low power macbook it takes a very short time, but on any github build it can easily take an hour, even if the last build was successful.

Adding caching to the compiled object files would normally speed this up significantly, is there a reason why it's not doing that? It already is but it's just a slow build machine?

[Xrayez]

I think fuzzing should be done in a separate repository, like I did in Goost: https://github.com/goostengine/goost-fuzzer. The main repository would run tests without heavy sanitizers, but that's ok since running unit tests could also be part of the fuzzer repository.