Simplify ci config a bit #2443

Veetaha · 2020-08-19T18:31:17Z

Description

Simplified the config by removing actions-rs/cargo@v1
See the uses cases described in its repo.

We don't use cross, do we?
Github annotations from ci failures are generally not that useful, most of the time we just open the job logs with red crosses near them and that's all.
Instead, we get less dependencies for our CI and less code

PR Checklist

By placing an x in the boxes I certify that I have:

Added unit tests for new code added in this PR.
Acknowledged that by making this pull request I release this code under an MIT/Apache 2.0 dual licensing scheme.
Added a changelog entry if this will impact users, or modified more than 5 lines of Rust that wasn't a doc comment.
Updated the content of the book if this PR would make the book outdated.

If this modified or created any rs files:

Ran cargo +stable fmt --all
Ran cargo clippy --all --features "empty" (may require cargo clean before)
Ran cargo build --features "empty"
Ran cargo test --all --features "empty"

Veetaha · 2020-08-19T18:35:46Z

@CleanCut I'd recommend scaling down the PR template, at least the part about Ran X.
The sentences about clippy, build, test are not that necessary, because CI will catch these nevertheless.
For cargo fmt I would suggest creating a git-pre-commit hook, for example in our repos we generally have a meta binary crate called xtask for dev things like pre-commit hooks that run cargo fmt automatically:
https://github.com/elastio/devx/blob/master/xtask/src/main.rs

CleanCut

Good direction. Fix a couple things at it's good to go.

CleanCut · 2020-08-20T04:30:58Z

.github/workflows/ci.yml

-        with:
-          command: fmt
-          args: --all -- --check
+      - run: cargo fmt


--all is a now-deprecated synonym for --workspace. Without one of these this command will only run on only the amethyst crate and none of the other crates in the workspace.

-- --check makes the command fail if it has to format something.

Suggested change

- run: cargo fmt

- run: cargo fmt --workspace -- --check

Cargo fmt actually never made the switch to --workspace and has to use --all.
There is an old issue talking about it on their repo.

Edit: Found the issue.
--all has a different function/meaning here

Yup, --workspace argument isn't valid with cargo fmt, added --all -- --check.
6fcda3a

Yikes! Sorry for the misinformation.

CleanCut · 2020-08-20T04:32:27Z

.github/workflows/ci.yml

-        with:
-          command: clippy
-          args: --features=${{matrix.FEATURES}}
+      - run: cargo clippy --features=${{matrix.FEATURES}}


Aha! You've discovered another bug. We were only running clippy on the root crate. We can't make it fail yet, though, because we haven't taken care of the warnings.

Suggested change

- run: cargo clippy --features=${{matrix.FEATURES}}

- run: cargo clippy --features=${{matrix.FEATURES}} --workspace

Yup, added --workspace in 6fcda3a

Surprisingly, clippy hasn't detected any new issues for the rest of the codebase. I guess that Ran cargo clippy chebox in PR template did the thing?

I won't complain if the clippy issues are all fixed! If that is the case, let's make clippy fail if it finds anything.

@CleanCut
Well, actually clippy does detect issues, but only at warning level and it doesn't fail ci, see this line in checks log

Would you merge this if I add these to to global ci env vars we have in our project?

RUSTFLAGS: "--deny warnings -Cdebuginfo=0" // Disabling debug info should speed up ci? RUSTDOCFLAGS: "--deny warnings"

CleanCut · 2020-08-20T04:44:58Z

@CleanCut I'd recommend scaling down the PR template, at least the part about Ran X.

@Veetaha I agree. I added it to the list in #2407. In addition, in the future I would like as much of each CI step as possible to be encapsulated in standalone scripts, and then link from the PR template to instructions about how to use them locally to have a faster feedback loop (that could include scripts for installing pre-commit hooks, etc.). So rather than having cargo fmt --workspace -- --check in CI, we'd have something like script/ci-fmt in CI, and you could also run that locally to get the same effect.

Veetaha · 2020-08-20T15:22:41Z

Btw, @CleanCut why not running cargo fmt --check only once on stable toochain and ubuntu-latest os.
I see that we already have 'macos' selected for doing some additional work which is platform-independent (e.g. mdbook checks), but from my experience 'ubuntu-latest' runners are the fastest ones among other Github-hosted runners

CleanCut · 2020-08-20T22:15:15Z

@Veetaha I would be fine running fmt and clippy only on a single stable OS. I'm also fine if we want to experiment with whichever OS's are the fastest to reduce overall time. Since I was experimenting with things, GitHub appears to have switched its macOS runners to some setup that runs significantly slower (it was faster than Linux at the time I chose macOS for the extra stuff). For clarity, let's do that in a separate PR from this one.

Veetaha · 2020-08-20T22:17:48Z

Sure, though clipply is quite caveat-y. I guess if there are some cfg(windows) or other stuff going on it won't check the windows-specific code if we run it on Linux only?

CleanCut · 2020-08-20T22:59:13Z

Sure, though clipply is quite caveat-y. I guess if there are some cfg(windows) or other stuff going on it won't check the windows-specific code if we run it on Linux only?

🤷‍♂️ If clippy gives different results on different platforms, then I withdraw it from consideration. 😄

Blisto91 · 2020-08-21T05:51:28Z

I think @Veetaha is correct here. Clippy has to compile the code it is working on, so platform (or feature) dependent code might not get lints unless run on that platform.

Fmt works on the raw source files and should be able to check everything no matter the OS. As far as I'm aware.

CleanCut · 2020-08-21T16:49:19Z

bors r+

bors · 2020-08-21T17:51:43Z

Build succeeded:

2450: Move cargo fmt checks and some other jobs to stable, ubuntu-latest r=CleanCut a=Veetaha ## Description See #2443 (comment) ## Modifications - Now a bunch of jobs that were previously selected to be run on `macos` switched to `ubuntu-latest`, cargo fmt check is one of them ## PR Checklist By placing an x in the boxes I certify that I have: - [ ] Added unit tests for new code added in this PR. - [x] Acknowledged that by making this pull request I release this code under an MIT/Apache 2.0 dual licensing scheme. - [ ] Added a changelog entry if this will impact users, or modified more than 5 lines of Rust that wasn't a doc comment. - [ ] Updated the content of the book if this PR would make the book outdated. If this modified or created any rs files: - [ ] Ran `cargo +stable fmt --all` - [ ] Ran `cargo clippy --workspace --features "empty"` (may require `cargo clean` before) - [ ] Ran `cargo build --features "empty"` - [ ] Ran `cargo test --workspace --features "empty"` Co-authored-by: Veetaha <veetaha2@gmail.com>

2450: Move cargo fmt checks and some other jobs to stable, ubuntu-latest r=Blisto91 a=Veetaha ## Description See #2443 (comment) ## Modifications - Now a bunch of jobs that were previously selected to be run on `macos` switched to `ubuntu-latest`, cargo fmt check is one of them ## PR Checklist By placing an x in the boxes I certify that I have: - [ ] Added unit tests for new code added in this PR. - [x] Acknowledged that by making this pull request I release this code under an MIT/Apache 2.0 dual licensing scheme. - [ ] Added a changelog entry if this will impact users, or modified more than 5 lines of Rust that wasn't a doc comment. - [ ] Updated the content of the book if this PR would make the book outdated. If this modified or created any rs files: - [ ] Ran `cargo +stable fmt --all` - [ ] Ran `cargo clippy --workspace --features "empty"` (may require `cargo clean` before) - [ ] Ran `cargo build --features "empty"` - [ ] Ran `cargo test --workspace --features "empty"` Co-authored-by: Veetaha <veetaha2@gmail.com>

Simplify ci config a bit

e170991

CleanCut suggested changes Aug 20, 2020

View reviewed changes

Add proper args to cargo fmt and cargo clippy

6fcda3a

CleanCut mentioned this pull request Aug 20, 2020

replace cargo --all with cargo --workspace #2435

Merged

4 tasks

CleanCut approved these changes Aug 21, 2020

View reviewed changes

bors bot merged commit 841f984 into amethyst:master Aug 21, 2020

Veetaha mentioned this pull request Aug 21, 2020

Move cargo fmt checks and some other jobs to stable, ubuntu-latest #2450

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify ci config a bit #2443

Simplify ci config a bit #2443

Veetaha commented Aug 19, 2020 •

edited

Loading

Veetaha commented Aug 19, 2020 •

edited

Loading

CleanCut left a comment

CleanCut Aug 20, 2020

Blisto91 Aug 20, 2020 •

edited

Loading

Veetaha Aug 20, 2020

CleanCut Aug 20, 2020

CleanCut Aug 20, 2020

Veetaha Aug 20, 2020

Veetaha Aug 20, 2020 •

edited

Loading

CleanCut Aug 20, 2020

Veetaha Aug 20, 2020 •

edited

Loading

CleanCut commented Aug 20, 2020 •

edited

Loading

Veetaha commented Aug 20, 2020

CleanCut commented Aug 20, 2020 •

edited

Loading

Veetaha commented Aug 20, 2020

CleanCut commented Aug 20, 2020

Blisto91 commented Aug 21, 2020 •

edited

Loading

CleanCut commented Aug 21, 2020

bors bot commented Aug 21, 2020

	- run: cargo clippy --features=${{matrix.FEATURES}}
	- run: cargo clippy --features=${{matrix.FEATURES}} --workspace

Simplify ci config a bit #2443

Simplify ci config a bit #2443

Conversation

Veetaha commented Aug 19, 2020 • edited Loading

Description

PR Checklist

Veetaha commented Aug 19, 2020 • edited Loading

CleanCut left a comment

Choose a reason for hiding this comment

CleanCut Aug 20, 2020

Choose a reason for hiding this comment

Blisto91 Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

Veetaha Aug 20, 2020

Choose a reason for hiding this comment

CleanCut Aug 20, 2020

Choose a reason for hiding this comment

CleanCut Aug 20, 2020

Choose a reason for hiding this comment

Veetaha Aug 20, 2020

Choose a reason for hiding this comment

Veetaha Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

CleanCut Aug 20, 2020

Choose a reason for hiding this comment

Veetaha Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

CleanCut commented Aug 20, 2020 • edited Loading

Veetaha commented Aug 20, 2020

CleanCut commented Aug 20, 2020 • edited Loading

Veetaha commented Aug 20, 2020

CleanCut commented Aug 20, 2020

Blisto91 commented Aug 21, 2020 • edited Loading

CleanCut commented Aug 21, 2020

bors bot commented Aug 21, 2020

Veetaha commented Aug 19, 2020 •

edited

Loading

Veetaha commented Aug 19, 2020 •

edited

Loading

Blisto91 Aug 20, 2020 •

edited

Loading

Veetaha Aug 20, 2020 •

edited

Loading

Veetaha Aug 20, 2020 •

edited

Loading

CleanCut commented Aug 20, 2020 •

edited

Loading

CleanCut commented Aug 20, 2020 •

edited

Loading

Blisto91 commented Aug 21, 2020 •

edited

Loading