Replace Travis CI with GitHub Actions

[dgunay]

Addressing this issue: #974

One note: this only runs the tests on the latest Ubuntu. I saw there is a different process for running the tests on Windows - that would probably require more work to configure correctly. Let me know if anything needs to be changed.

[coriolinus]

One thing I don't see in this PR is a replication of the stable/beta/nightly and environment matrix. For example, on Travis, both stable and beta channels run both with and without DENYWARNINGS=1, but allows failures for the beta channel with warnings. The nightly channel runs the check-exercises.sh script with BENCHMARKS=1 enabled. I don't see anything in this current configuration selecting a Rust channel at all. Did I miss it?

[dgunay]

@coriolinus You're right, I did not replicate that. I'll try and implement that.

[coriolinus]

Another thing I just noticed: in several places in _test, scripts look for a travis environment variable. A representative example:

rust/_test/check-exercise-crate.sh

Line 7 in a268661

if [ "$TRAVIS_PULL_REQUEST" != "false" ]; then

That will have to be dealt with one way or another. At the most basic level, we can re-write the scripts to use a different mechanism to detect whether they're running in a pull request, or we can inject that environment variable if the action was triggered by a pull request, as opposed to just checking the most recent commit of the master branch. I have a mild preference for rewriting the scripts to stop referencing Travis, but I suspect that we'd need to inject our own environment variable anyway, so I don't mind leaving the scripts as they are.

[dgunay]

Fiddling with the job configuration in a separate branch to avoid spamming this PR - one worrying thing I've noticed is that the scripts (I can see the ones with the old TRAVIS_ env variables at least) fail silently while reporting success in the GitHub Actions UI:

[coriolinus]

Interesting. I'm having a hard time replicating that error--I don't know what git version they're using, but it appears to be different than mine--but I think we can at least improve error propagation by injecting

set -e -o pipefail

into the affected scripts near the top.

[coriolinus]

Also, the error seems to imply that master is not in the working tree, which suggests that something in the checkout has failed; that branch should always be available.

[dgunay]

I believe this might be what's happening: https://github.bokerqi.topmunity/t/git-ambiguous-argument-master/17832/2

I think changing the script to refer to origin/master instead of just master might fix it.

[SaschaMann]

I don't know what git version they're using

git 2.28.0

[dgunay]

Interesting. I'm having a hard time replicating that error--I don't know what git version they're using, but it appears to be different than mine--but I think we can at least improve error propagation by injecting

set -e -o pipefail

into the affected scripts near the top.

This makes the jobs fail at least, but changing to origin/master did not help. I'll try seeing what's wrong.

[dgunay]

Added another step to each job to checkout the master branch locally - turns out that wasn't happening when I had a look with git branch -a. Will have something new to look at soon.

[dgunay]

As far as I am aware, the remaining things left to fix:

1. I'm not sure how to conditionally set TRAVIS_PULL_REQUEST based on whether or not the action is being triggered by a pull request.

2. Still not quite sure how to do continue-on-error when on the beta toolchain with DENYWARNINGS=1

[coriolinus]

I haven't experimented with this, but in the docs, I see the following snippet:

env:
  my_env_var: ${{ <expression> }}

This is tempting, but apparently wrong, because github actions syntax does not support inline conditional expressions. Instead, we should keep looking. For example:

github.event	object	The full event webhook payload. For more information, see "Events that trigger workflows."

Follow through with the pull request object docs, the commands docs, and a bit of bash variable expansion magic and it looks like what we need is something like this:

steps:
  - name: set TRAVIS_PULL_REQUEST
    env:
      PR_NUMBER: ${{ github.event.pull_request.number }}
    run: echo "TRAVIS_PULL_REQUEST=${PR_NUMBER:-false}" >> $GITHUB_ENV

---

Did the suggestion here fail? If yes, how?

[dgunay]

Okay, I think I've got something that works as intended. Things addressed:

• I made sure to set the selected toolchain as the default during rustup
• @coriolinus your suggestion for setting TRAVIS_PULL_REQUEST worked nicely
• I got continue-on-error to work by copying and pasting it for each step that needed it.

I tested it by altering the check-exercises.sh script to fail on beta with DENYWARNINGS=1.

I'm not super happy about the copying and pasting but perhaps someone with more knowledge of GitHub Actions could make it more DRY.

By the way, check-exercises.sh seems to fail right now on stable with DENYWARNINGS=1. I can reproduce this locally. That's only because when I push, it runs all of the tests so not sure it's a concern right now.

[dgunay]

Oh, another thing - is nightly supposed to be run both with and without DENYWARNINGS?

[coriolinus]

Great! At this point, it seems that the only things remaining will be to clean up some of the comment-only edits to the test scripts, and actually test the whole thing.

It is definitely possible to reduce duplication within the source file by using some YAML Anchor and Alias trickery, but I have mixed feelings about that; I've seen the technique used to produce very dense configurations which were almost impossible to understand. I don't oppose trying it out, but I'd reserve the right to ask to revert those changes. I'm sorry to be so vague about the acceptance criteria for this, but I'm finding them hard to verbalize. All I can say is that if the anchor/aliased config is readable and well-documented, I'll probably accept it; if I find it confusing, I'd ask it to be reverted.

Thanks for spotting the DENYWARNINGS failure on stable! I'm working on a fix for that now. #977

nightly does not need DENYWARNINGS; we only run nightly at all in order to run the benchmarks.

[dgunay]

Sorry for the lack of activity - school kind of took over my life for a bit. Do you have any recommendations for how to test it? Otherwise I guess I was just going to run it manually in these scenarios (third dimension not pictured: with and without failing code):

	stable	beta	nightly
direct push
pull request

I do hope there's a good automated way of testing GitHub actions because it seems like a lot of work...

[coriolinus]

No worries about lack of activity; this is volunteer work for all of us, and we've all got other things going on in our lives.

I don't think you need to worry about pushing some code which behaves differently according to the stable/beta/nightly dimension; manual verification that those stages have been run is sufficient. Testing direct push to master is probably not necessary either: we know that it'll be running the same actions as a pull request, and it'll be straightforward to verify whether or not those steps are complete after merging this PR.

To my mind, what we really want to demonstrate are these cases:

• most recent commit to a PR for which all is good, succeeds
• at least one example for each action which causes that action alone to fail independently. Probably the simplest examples:
  • compilation failure in an exercise's example
  • wrong number of #[ignore] lines in a test file
  • README for an exercise has an edit
  • benchmarking fails for an exercise with a bench

As for how to demonstrate this, I think creating a PR based on this PR targeting your own fork of this repo will cause the actions to run. Create some innocuous commit in that PR to demonstrate the success case, then add / revert commits to demonstrate each of the failure cases. Once that is all done, just link to that separate PR here, and we can inspect to verify that things worked as expected.

[coriolinus]

I did #983 to test that these actions work as expected; they succeeded very nicely. I'm going to do a little bit of cleanup now, then approve/merge.

[coriolinus]

Thanks for doing all this work @dgunay!

[dgunay]

@coriolinus Thank you and @SaschaMann for the feedback! Glad I could help and learn a bit about GitHub Actions in the process.