CI Enhancements

[qarmin]

With good CI, a lot of bugs and regression can be caught and also things like bad formatting

• Check formatting in code by cargo fmt
• Unit tests(are in CI, but seems that are missing in code)
• Integration tests
• Build Bevy also on Windows and MacOS (add build windows block to github actions ci #129)
• Cache for faster building (https://github.com/godotengine/godot/blob/f568cede8d8a4a3f7051afd2ae8502deb09bc157/.github/workflows/linux_builds.yml#L30-L40) (Separate CI jobs, add job caching #357)
• Benchmarks and performance regression testing
• Testing project for regression(panics etc.) - project should use as much as possible elements(This is not available now, since there isn't exists for now any tool that provide software rendering of Vulkan apps)
• Sanitizers support(ASAN, LSAN, TSAN, MSAN) - Bevy use some amount of unsafe code which may be invalid, but also some valid code may cause some leaks or bugs - LeakSanitizer, ThreadSanitizer, AddressSanitizer and MemorySanitizer support rust-lang/rust#38699
  I think, that same project from above should be running but this time with sanitizers support.

[cart]

These are all great ideas!

• unit tests: we do have tests. not as many as i would like, but make sure you're running cargo test --workspace to run everything.
• cache: i had the cache wired up in the past, but due to the large size of build artifacts, the save/restore steps ended up being more expensive than just rebuilding.

Testing project for regression(panics etc.) - project should use as much as possible elements

could you elaborate on this?

[multun]

Can you add some merge queue management to the list, like bors?

[cart]

yup that is actually priority 1 as far as CI is concerned. right now we dont have strong safety guarantees that a build is clean. we really need to fix that asap.

[multun]

I'd also remove caching and regression testing from the list, it doesn't seem yet worth it, the build isn't big nor long enough, and regressions not a big deal until things are a bit more settled

[erlend-sh]

Looking at Amethyst’s recent CI migration to GitHub actions would probably be informative here.

I dunno what’s the best place to look to get the full picture of how it works, but @CleanCut might.

[AngelOnFira]

Hey @qarmin, I come from the Veloren project, and I've done a lot of work on CI in Rust. After taking a look through the current CI, there are a few things I might suggest changing/adding. None of them need to be used, but hopefully it might include something of value 😄

Change:

• Currently, there is only one build job. It might make sense to split into multiple jobs, so that each can run in parallel. The separate jobs might be:
  • cargo check
  • cargo fmt
  • cargo clippy

Add:

• Some other tasks that we do that might be of use
  • Build docs from cargo doc (this might already be done if bevy is on crates.io)
  • cargo audit for security checks

[duncanrhamill]

Hey not sure if this is the right place but would also be nice to have a build status tag in the readme, since the book actually suggests using the git dependency and it would be nice to see if that's immediately going to break a build. Maybe a "last commit that passed" tag would be helpful too?

[AngelOnFira]

@duncanrhamill Good idea, I added that in #325

[AngelOnFira]

@cart in the CI workflow, I see this:

  # NOTE: temporarily disabled because on-push currently uses up too many resources
  # push:
  #   branches: [master]

Since the repo is now public, you get unlimited Action minutes, and up to 20 concurrent jobs. I believe this is enough to allow running on every push?

[CleanCut]

Since the repo is now public, you get unlimited Action minutes, and up to 20 concurrent jobs. I believe this is enough to allow running on every push?

That's correct, public repositories get unlimited minutes. I wrote a comment to this effect several days ago, but must have forgotten to hit the submit button. 😊

[cart]

Well thats fantastic news. Thanks for the heads up. Time to heat up some datacenters 😈

[cart]

Hmm I'm still seeing a 2000 minute limit for the Bevy Organization.

[AngelOnFira]

I think that just includes any minutes that are from repos that aren't public. To test it, it might be worth seeing if it increases past 86 once there is another push to master. But there have also been so many CI jobs running, that I don't imagine it could only be 86 minutes. It's not eating into my personal minutes when I make a PR, so that should mean that forks only use CI minutes at the expense of the org (and not at all in the case of public repos) 💯

[CleanCut]

Yes, you still have a limit for private repos. bevyengine/bevy is a public repo, so it is not subject to those restrictions.

[cart]

Bawhaha thanks for pointing out what (should be) obvious. I totally missed that 😅

[AngelOnFira]

😆 I didn't see that either 😛 I'll make a PR to get CI running hot 🔥

[lberrymage]

Something like GitHub's dependabot (not dependabot-preview anymore!) would be nice to use as well so we don't have to manually make periodic "Update xxx dependency to x.y.z" commits. Not sure how well that fits under the CI umbrella, but it would be useful to automate (most) dependency upgrades nonetheless.

[memoryruins]

If dependabot is used, I suggest setting the interval to monthly or weekly to minimize PR opening/closing noise.

[cart]

I think we dont always want to upgrade asap because we want to stay synced up our dependencies to avoid mismatches / reduce dependency counts.

[lberrymage]

That makes sense. dependabot could be nice for the sake of having a tracking issue of sorts though; that way certain dependency upgrades have a mention in the issue tracker and (theoretically) don't get forgotten for long periods of time. I agree that automatic merges aren't a good idea if that's what you're talking about specifically.

[CleanCut]

Dependabot's killer feature for me is the quick reporting of security vulnerabilities in your dependencies. Non-security updates can be configured down to only alert monthly (and specific entries can be set to be ignored), but can't be turned off entirely as far as I can tell.

[lberrymage]

I'm pretty sure you can disable non-security updates by just not checking in a dependabot.yml. Security updates are enabled through repository settings. It's explained in detail in the first paragraph here (plus the page that links for configuring security updates).

[cart]

Ok i think im sold on dependabot (with a decently long check timer to avoid noise and manual-only merges). And we should always verify that our "redundant dep count" doesnt go up before merging. We've already regressed a bit in this area, but that was largely unavoidable. Can't wait for winit 0.22.3 :)

[lberrymage]

How long of a check timer are you thinking? The options we have are one week or one month, but consider that there's also open-pull-requests-limit (which is five by default) to prevent noise. Sadly cron-like schedule syntax isn't implemented yet.

I'm happy to say that the last major blocker for winit 0.23.0 is under active review!

[cart]

hmm i guess lets try one week with the default pr limit. we can always bump it up to a month if theres too much noise.

[lberrymage]

Sounds good. I'll make a PR.

[lberrymage]

One more question: do we want dependabot to try to update GitHub Actions in the CI configuration?

[cart]

Sure! We can always disable it if it becomes a problem.

[alice-i-cecile]

These are largely complete, although we can't run benchmarks in CI due to reliability, or sanitizers due to time. Closing this out, feel free to make more targeted issues to resolve anything that I missed.