Skip to content
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

Add GitHub actions CI, with format check #16

Merged

Conversation

64kramsystem
Copy link
Member

@64kramsystem 64kramsystem commented Jun 28, 2022

This blocks code that is not formatted (note: Actions may need to be activated on this repo).

Note that the other action that the README currently suggests:

cargo clippy -- -W clippy::correctness -D warnings

is not currently possible (without tweaks) because of some Bevy issues/characteristics, e.g. the following (known) problem:

error: call to `std::mem::forget` with a value that does not implement `Drop`. Forgetting such a type is the same as dropping it
  --> src/main.rs:102:10
    |
102 | #[derive(Bundle)]
    |          ^^^^^^
    |
note: argument has type `Enemy`
  --> src/main.rs:102:10
    |
102 | #[derive(Bundle)]
    |          ^^^^^^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#forget_non_drop
    = note: this error originates in the derive macro `Bundle` (in Nightly builds, run with -Z macro-backtrace for more info)

Some of the suggestions are worth applying though, e.g. unnecessary clone() calls.

@odecay
Copy link
Collaborator

odecay commented Jun 28, 2022

It seems worthwhile to have CI setup.
I know it possible to work around the bundle issue in bevy bevyengine/bevy#4601 (comment).
We could either delay setting up warnings till that is fixed or use the workaround.

@64kramsystem
Copy link
Member Author

It seems worthwhile to have CI setup. I know it possible to work around the bundle issue in bevy bevyengine/bevy#4601 (comment). We could either delay setting up warnings till that is fixed or use the workaround.

Definitely. Since that task has some complexity involved, I think it's more efficient to split the CI tasks in separate PRs:

  • format (this)
  • clippy
  • build (some level of smoke testing, e.g. on multiple platforms)

@odecay
Copy link
Collaborator

odecay commented Jun 28, 2022

PR away, I was thinking for build we would base off https://github.com/bevyengine/bevy_github_ci_template/blob/main/.github/workflows/release.yaml

Would be nice to get that set up for next release. On top of that maybe we could set up as a github pages hosted wasm builds on tags/release.

@odecay odecay merged commit bb6985a into fishfolk:master Jun 28, 2022
@64kramsystem 64kramsystem deleted the add_github_actions_ci_with_format_check branch June 28, 2022 15:21
@64kramsystem
Copy link
Member Author

64kramsystem commented Jun 28, 2022

PR away, I was thinking for build we would base off https://github.com/bevyengine/bevy_github_ci_template/blob/main/.github/workflows/release.yaml

Would be nice to get that set up for next release. On top of that maybe we could set up as a github pages hosted wasm builds on tags/release.

Rust builds need to be carefully planned. AFAIK open source projects have 2000 minutes per month, which is not little, but build time in the Rust world is... something 😉 especially, when one builds for multiple platforms. Oh, and Bevy is B-I-G 😄.

Like that wasn't enough, sometime seemingly little known in the Github Actions world is that caching is not shared across PRs. This means that if each PR will rebuild the project from scratch (on the first build). As a workaround, one needs to build in the main branch (whose cache is shared with branches/PRs).

This is unfortunately not a theoretical matter, since with the rusty game jam project, we (our team) have exhausted our credit 🤯.

Anyway, nothing impossible, in particular, if a solid solution is found, it's something worth upstreaming. I'll follow up on Discord and/or here. 👋

@odecay odecay mentioned this pull request Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants