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

Veetaha · 2020-08-21T22:18:42Z

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.
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"

CleanCut

Caught a couple things you missed on the mdbook-linkcheck section. I know it's commented out, but let's keep it in good shape for when we uncomment it (hopefully really soon!). 😄

I find the runtimes of this CI run fascinating.

macos stable - 17m
macos beta - 14m
macos nightly - 51m

The speed of first two means we got assigned to the fast macOS runners (the newer, slower ones take 20+ minutes).

The slowness of the last one is the sporadic macOS CI bug that hasn't been eliminated from the shared workers. AFAICT the bug affects both the fast and slow runners.

ubuntu stable - 22m
ubuntu beta - 18m
ubuntu nightly - 18m

In my initial testing, there were only the faster mac runners, so I was comparing the ~18m Linux runtime against ~14m macOS runs. Given the current situation, Linux is a better choice.

CleanCut · 2020-08-23T04:29:25Z

.github/workflows/ci.yml


 # Should be working, but postponing until after we go live with GitHub Actions
-#      - name: install mdbook-linkcheck on stable macos
+#      - name: install mdbook-linkcheck on stable, ubuntu-latest
 #        run: |
 #          curl -L https://github.com/Michael-F-Bryan/mdbook-linkcheck/releases/download/v0.7.0/mdbook-linkcheck-v0.7.0-x86_64-apple-darwin.tar.gz | tar -xz && mv mdbook-linkcheck /Users/runner/.cargo/bin/mdbook-linkcheck


The URL for the mdbook binary needs to be changed to the Linux one.

The location for the untarred binary needs to be changed

Done 213d343

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

.github/workflows/ci.yml


 # Should be working, but postponing until after we go live with GitHub Actions
-#      - name: install mdbook-linkcheck on stable macos
+#      - name: install mdbook-linkcheck on stable, ubuntu-latest
 #        run: |
 #          curl -L https://github.com/Michael-F-Bryan/mdbook-linkcheck/releases/download/v0.7.0/mdbook-linkcheck-v0.7.0-x86_64-apple-darwin.tar.gz | tar -xz && mv mdbook-linkcheck /Users/runner/.cargo/bin/mdbook-linkcheck
 #          chmod a+x /Users/runner/.cargo/bin/mdbook-linkcheck


Needs to be updated to the new location of the binary

Done 213d343

CleanCut · 2020-08-23T04:33:37Z

.github/workflows/ci.yml

-      - name: run tests
-        run: |
-          cargo test --workspace --features=${{matrix.FEATURES}}
+      - run: cargo test --workspace --features=${{matrix.FEATURES}}


I prefer the concise, descriptive names, but I am willing to approve the PR without them.

As for me, a simple cargo foo invocation looks descriptive enough by itself...

Veetaha · 2020-08-25T19:58:30Z

I've verified that linkcheck works in this commit 213d343 in this ci run: https://github.com/amethyst/amethyst/pull/2450/checks?check_run_id=1028251375#step:13:11

Blisto91 · 2020-08-27T18:37:32Z

I'm currently working on fixing all the broken links and linkcheck warnings.
Will also prepare the options in book.toml 👍

CleanCut · 2020-08-27T20:09:05Z

bors r+

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>

bors · 2020-08-27T20:14:09Z

Build failed:

Tests (ubuntu-latest, stable)

Veetaha · 2020-08-27T20:15:51Z

Whoops, what could go wrong...

Veetaha · 2020-08-28T00:31:57Z

Hmm, I can't reproduce the clippy warning locally...

Blisto91 · 2020-08-28T06:01:42Z

Looks like it started with the newly released rust version.
But I didn't see it mentioned in the clippy changelog

Veetaha · 2020-08-28T08:53:58Z

This could be some bug fix in 1.46
I guess it might make sense to pin the stable version, or run clippy at least for the beta toolchain or don't deny warnings on ci

Blisto91 · 2020-08-28T10:49:43Z

I'm not a fan of preventing the CI from updating to the latest stable as soon as it's released. If thats what you mean by pin the stable version.
A minor semver version of rust is only released approx. every 6 weeks so i don't think it's too much of a bother if something breaks.
If we also enable clippy lints on beta we might risk that things break more often when bugs are introduced or false positivs comes up for a short time.

Veetaha · 2020-08-28T19:20:26Z

I'm not a fan of preventing the CI from updating to the latest stable as soon as it's released. If thats what you mean by pin the stable version.
A minor semver version of rust is only released approx. every 6 weeks so I don't think it's too much of a bother if something breaks.
If we also enable clippy lints on beta we might risk that things break more often when bugs are introduced or false positivs comes up for a short time.

Since clippy may introduce new warnings to code that was ok on the previous minor toolchain version and
taking into account @Blisto91 feedback I see these options:

We accept that each 6 weeks our ci may be failing due to new clippy lints and fix them ASAP
This means unrelated PRs will be blocked until the fix.
We make clippy a separate job and make it optional to pass via continue-on-error. This will protect us from sudden breakages, though also allow merging the branch with clippy warnings, but the job should be displayed with a red cross so it's easy to notice that clippy is failing, anyway
We do remove --deny warnings. This means the warnings will most likely be ignored if some person doesn't regularly keep an eye on them.

Regarding the beta/nightly runs
I would also make them optional via continue-on-error. In my experience we had a number of times the bugs were introduced to nightly toolchain so that our nightly job was failing. The purpose of running such jobs is to verify our code forwards compatibility with the toolchain updates and to give rust core team some feedback in case of regressions too.

Blisto91 · 2020-08-29T10:30:57Z

The resistance we had to continue-on-error originally was because we didn't like how it was presented in the ui with the red crosses.
We were looking for alternatives but there aren't any that are properly supported by github.
But I can see that there are scenarios where it can make sense to use it.

I think the short to mid term aim should be to make life easier for the project with minimal manual maintenance (without compromising too much on quality). Currently the activity and reaction pace can be at bit slow so I would like to minimize scenarios where something has to be fixed quickly to rid errors popping up everywhere.
In the clippy case I'm personally leaning towards removing --deny warnings

2468: Fix clippy, don't deny warnings r=Blisto91 a=Veetaha ## Description Fixed the new warnings introduced in 1.46.0 update. Also had to remove `--deny warnings` as discussed: #2450 (comment) ## 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>

Veetaha · 2020-08-29T14:22:13Z

@CleanCut , @Blisto91 I think this is ready for bors r+ to retry

Blisto91 · 2020-08-29T14:35:55Z

bors r+

bors · 2020-08-29T14:54:16Z

Build succeeded:

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

a3e1d52

CleanCut suggested changes Aug 23, 2020

View reviewed changes

Verify linkcheck works

213d343

Veetaha requested a review from CleanCut August 25, 2020 19:58

Veetaha force-pushed the feat/run-jobs-on-ubuntu-latest branch from 231e117 to 08106e7 Compare August 25, 2020 20:15

Comment out linkcheck step as it was before

5b3410e

Veetaha force-pushed the feat/run-jobs-on-ubuntu-latest branch from 08106e7 to 5b3410e Compare August 25, 2020 20:16

CleanCut approved these changes Aug 27, 2020

View reviewed changes

retirgger ci

104f47f

Veetaha mentioned this pull request Aug 29, 2020

Fix clippy, don't deny warnings #2468

Merged

8 tasks

bors bot merged commit c8c01b0 into amethyst:master Aug 29, 2020

ezpuzz added the type: maintenance A task that has no noticeable user-facing effect. Code cleanup, small refactors, etc. label Jan 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Veetaha commented Aug 21, 2020

CleanCut left a comment

CleanCut Aug 23, 2020

Veetaha Aug 25, 2020

CleanCut Aug 23, 2020

Veetaha Aug 25, 2020

CleanCut Aug 23, 2020

Veetaha Aug 25, 2020 •

edited

Loading

Veetaha commented Aug 25, 2020

Blisto91 commented Aug 27, 2020

CleanCut commented Aug 27, 2020

bors bot commented Aug 27, 2020

Veetaha commented Aug 27, 2020

Veetaha commented Aug 28, 2020

Blisto91 commented Aug 28, 2020

Veetaha commented Aug 28, 2020 •

edited

Loading

Blisto91 commented Aug 28, 2020

Veetaha commented Aug 28, 2020 •

edited

Loading

Blisto91 commented Aug 29, 2020 •

edited

Loading

Veetaha commented Aug 29, 2020

Blisto91 commented Aug 29, 2020

bors bot commented Aug 29, 2020

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

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

Conversation

Veetaha commented Aug 21, 2020

Description

Modifications

PR Checklist

CleanCut left a comment

Choose a reason for hiding this comment

CleanCut Aug 23, 2020

Choose a reason for hiding this comment

Veetaha Aug 25, 2020

Choose a reason for hiding this comment

CleanCut Aug 23, 2020

Choose a reason for hiding this comment

Veetaha Aug 25, 2020

Choose a reason for hiding this comment

CleanCut Aug 23, 2020

Choose a reason for hiding this comment

Veetaha Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

Veetaha commented Aug 25, 2020

Blisto91 commented Aug 27, 2020

CleanCut commented Aug 27, 2020

bors bot commented Aug 27, 2020

Veetaha commented Aug 27, 2020

Veetaha commented Aug 28, 2020

Blisto91 commented Aug 28, 2020

Veetaha commented Aug 28, 2020 • edited Loading

Blisto91 commented Aug 28, 2020

Veetaha commented Aug 28, 2020 • edited Loading

Blisto91 commented Aug 29, 2020 • edited Loading

Veetaha commented Aug 29, 2020

Blisto91 commented Aug 29, 2020

bors bot commented Aug 29, 2020

Veetaha Aug 25, 2020 •

edited

Loading

Veetaha commented Aug 28, 2020 •

edited

Loading

Veetaha commented Aug 28, 2020 •

edited

Loading

Blisto91 commented Aug 29, 2020 •

edited

Loading