Replace Travis with GitHub action for linting & tests

[repi]

This is a first experiment to use @svartalf 's new Rust GitHub actions for verifying that that are no differences with rustfmt and no clippy warnings instead of Travis CI.

My hope is that it would both be less setup, better performance (Travis is capped to 5 concurrent builds per org) and better user experience with being able to see the errors & warnings of a failing check directly in GitHub.

Have run into some issues though, primarily that occasionally the clippy build simple doesn't want to start, which is a major blocker. Issue with GitHub Actions in general, or configuration error?

Here is an example of such a build: https://github.com/EmbarkStudios/texture-synthesis/runs/238881653. Just sits on "Starting your workflow run..."

[Jake-Shadle]

Hmm, strange, it doesn't even appear that you can cancel the stuck clippy action. 😦

[svartalf]

Hey, @repi and @Jake-Shadle! I think that the stuck "CI / clippy (push)" check is on me.

Can you change this line

uses: actions-rs/clippy-check@v1

to

uses: actions-rs/clippy-check@instant-success

temporary so we could test the fix?

[repi]

@svartalf thanks! trying it out and we'll see if it works better

[repi]

@svartalf that did start the build and passed, but didn't do anything in it

[svartalf]

@repi according to the check page, clippy had found nothing to report about.
Had you expected another result?

[repi]

Yes I expected it to report 1 warning, this one which 1.38 clippy finds:

warning: hidden lifetime parameters in types are deprecated
    --> lib\src\multires_stochastic_texture_synthesis.rs:1193:13
     |
1193 | ) -> Option<GuidesStruct> {
     |             ^^^^^^^^^^^^- help: indicate the anonymous lifetime: `<'_>`

And I thought it was a bit suspicious in the build log that one doesn't see any output from the cargo clippy command execution. So did it actually run here now?

[svartalf]

Okay, I think I know what the problem is :)

args input in the workflow file should be fixed, as with the current value it invokes clippy as in following:

          args: --all-features -D warnings

$ cargo +1.37.0 clippy --all-features -D warnings
error: Found argument '-D' which wasn't expected, or isn't valid in this context

USAGE:
    cargo check --all-features

For more information try --help

cargo clippy output is silent over there, as this Action is using --message-format=json in order to parse clippy warnings, but in your case it is also failed to show the CLI error :)
I'll create an issue about it.

Can you try to remove the -D warnings argument?

[repi]

Ah good catch!

We do want clippy warnings to be interpreted as errors for CI, but I added the setting wrong, needs to be clippy -- -D warnings, not clippy -D warnings. Will fix

[svartalf]

In fact, with a current implementation, non-zero exit code for cargo clippy will not mark the Action execution as failed because it is just ignored.
Can you elaborate a bit on your case? Why are you want to interpret warnings as errors only in CI? Would not it be easier to just add some #![deny(clippy::*)] attributes?

[repi]

Ok interesting.

We chose this workflow in our repos because when working locally it was a bit too much of a hassle to make sure we have no clippy warnings, esp. as one may also try different toolchains such as switching between stable, beta and nightly for testing and experimentation purposes.
But when submitting new PRs we do want to make sure there are no clippy warnings on that branch so there isn't any that come in to master.

Quite similar to rustfmt, while one is working locally it is ok that the code is not formatted, but want to make sure in PRs that everything has been formatted. Actually in that specific case we wouldn't ideally want to fail CI on rustfmt mismatches, but actually have another action that just runs rustfmts on that branch and commits it. But that is another story.

We could potentially change our clippy workflow to deny all clippy warnings, but the current set up have been working quite well for us. At least with clippy it is not the end of the world if the clippy build fails locally, as that is not blocking you from running or developing on it.

[svartalf]

@repi it is very interesting, thank you. Could I ask you to restart the build once again? No additional changes, just press the "Re-run all checks" button

[repi]

@svartalf done

[svartalf]

@repi ta-da: https://github.com/EmbarkStudios/texture-synthesis/pull/47/checks?check_run_id=239021326

Your idea is reasonable, so I added two extra things:

1. clippy output is in the logs now (it is in JSON format, but it is what it is)
2. Action itself will fail too if clippy exit with a non-zero code now

[repi]

@svartalf sweet, big thanks for the quick support and fixes!

[svartalf]

And thank you for your thoughts and test environment :) I published new version already, do not forget to switch back from the @instant-success to @v1 version

[repi]

@svartalf right will change it back.

Oh and another small thing if you don't mind. or actually two :)

1. the rustfmt action here runs actions-rs/toolchain@v1, which takes ~45 s. But the clippy action here doesn't run it and directly adds the clippy component which takes just 1 sec. Possible to get the rustfmt action as fast also? nice to get feedback super fast on formatting mistakes.

2. In the GitHub checks UI one gets clippy showing up twice, assume because one of the actions launches the other one. this is a bit cryptic though. possible to set up in a different way to avoid it?

[svartalf]

1. Well, as for clippy, you are using system Rust (1.37.0 as for ubuntu-latest image). For rustfmt you are specifically installing latest stable and since yesterday it is a 1.38.0 version, that's why it takes so long.

Either you can remove the actions-rs/toolchain@v1 line and use system Rust for clippy too, or, if you want to use latest stable all the time, you may try to combine rustfmt and clippy jobs into a one big job, which is sharing the same Rust toolchain (I would suggest to use cargo clean between their invocations, though).

2. Yep, it irritates me too. First one is the Action itself and the second one is the "Check run". I'm not sure yet if is there any way to join them, probably I should ask GH support about it? No idea at the moment, sorry :(

[repi]

Thanks, makes sense. and as one can't select other images one will always have that Rust install step. Hopefully later rustup gets the option to install without docs that could speed it up a bit though.

Good suggestion to combine the steps, that actually solved point 2 for me also because it is just a single step and we run both rustfmt and clippy on the same latest stable version, not different versions. And configuration is simpler:

on: push
name: CI
jobs:
  fmt:
    name: Lint
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v1
      - uses: actions-rs/toolchain@v1
        with:
          toolchain: stable
          override: true
      - run: rustup component add rustfmt
      - uses: actions-rs/cargo@v1
        with:
          command: fmt
          args: --all -- --check --color always
      - run: rustup component add clippy
      - uses: actions-rs/cargo@v1
        with:
          command: clippy
          args: --all-features -- -D warnings

[svartalf]

In fact, rustup will support "profiles" really soon, so the toolchain installation step going to be faster then.

[repi]

@svartalf Yeah that will be nice, running the toolchain install action on Windows takes close to 3 min. Though we have the same problem on Travis. Looking forward to that improvement

[repi]

@Jake-Shadle I tested replacing also the cargo test triple platform builds here with the GitHub action, and seems to work quite well. Was around the same speed but a bit cleaner configuration.

Build times (Travis vs Github Actions):

• Linux: 7:10 -> 6:54
• Mac: 7:09 -> 4:32
• Windows: 10:44 -> 10:04

And this is on their standard 2 core machines, but should overall be a lot better than our Travis usage because GitHub allows running way more parallel builds:

• You can execute up to 20 workflows concurrently per repository.
• You can execute up to 1000 API requests in an hour across all actions within a repository.
• Each job in a workflow can run for up to 6 hours of execution time.
• You can run up to 20 jobs concurrently per repository across all workflows.

And I hope later when they are out of beta that one could pay for getting better machines (8 cores at least would be nice).

[repi]

Note: still currently use Travis for publishing releases here

[repi]

If/when we merge this in, I'll have to reconfigure the list of required checks on the protected master branch here in Settings. unfortunately that is not just a file that one can change in a branch, not ideal. Really would have wanted the default to be that all checks are required of one requires checks.

[Jake-Shadle]

Awesome, I can transition the deployment next week since I want to do it for cargo-deny as well, good that everything is better and cleaner. Thanks a lot for the actions and help @svartalf!