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

Run CI on Github Actions #1815

Merged
merged 7 commits into from May 5, 2020
Merged

Run CI on Github Actions #1815

merged 7 commits into from May 5, 2020

Conversation

hudson-ayers
Copy link
Contributor

Pull Request Overview

This pull request adds Github Actions based CI to Tock. For now, Github Actions would run in parallel with Travis until we are comfortable that actions is not missing anything that Travis catches.
It also replicates the size reporting script by adding a github check that contains the size diffs. It also saves artifacts on every build that store successfully built .bin files for each board, so that the exact kernel executable produced for each build can be easily obtained and flashed without requiring one to fetch the branch and recompile.

Notably, we split the CI into several logically separate jobs so that they can run in parallel. This does mean that more checks show up at the bottom of each PR.

The total time to run the github actions based CI is about 4 minutes!

Testing Strategy

This pull request was tested by forking into another organization and running CI on various branches.

We tested integration with bors on this fork as well, but for now this PR does not configure Bors to block on Github actions checks.

TODO or Help Wanted

N/A

Documentation Updated

  • Probably no need to update docs until we move to an exclusively github actions based workflow.

Formatting

  • Ran make formatall.

@hudson-ayers
Copy link
Contributor Author

Oh Github actions run on the PR so you get an example underneath!

@lschuermann lschuermann added this to Work in progress in Project Tracking May 1, 2020
@ppannuto
Copy link
Member

ppannuto commented May 1, 2020

Can you rebase this on latest master? The makefile changes look funny here in a way I think they shouldn't. I tried don't have write permissions on Tock-CI/pr

Copy link
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also include the necessary changes to bors.toml (maybe commented out for now? though honestly I think we should just be bold and start exercising these now; I don't anticipate a lot of problems)

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@bradjc
Copy link
Contributor

bradjc commented May 1, 2020

How does the benchmarks test work? I see:
image

@hudson-ayers
Copy link
Contributor Author

How does the benchmarks test work?

That is because no size changes were detected for this PR. For an example of a PR with a size change on Imix, see here: Tock-CI#9

@hudson-ayers
Copy link
Contributor Author

@ppannuto rebased + addressed other comments

bradjc
bradjc previously approved these changes May 1, 2020
Copy link
Contributor

@bradjc bradjc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a great improvement!

Copy link
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I think I didn't really grok this 'steps' thing with the github workflows before. I like the 1:1 mapping of step to CI target, but I think this is still missing the top-level driver. i.e. you see this:
image

There is a CI job called "format", so I should be able to run make ci-format. I think the thing I want would be an additional phony rule in the makefile for each of the jobs that replicates the steps list, i.e.

# Replicates the list of steps performed by the `format` job in github actions
.PHONY: ci-format
ci-format: ci-formatting ci-documentation

Putting these up in the "meta-targets" section of the makefile

ppannuto
ppannuto previously approved these changes May 4, 2020
Copy link
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

Thanks for putting up with all my renaming :)

I think we agreed we want to move this direction on the core call, and this is something better evaluated by testing and use, so I'm merging aggressively and we shall see how it works!

@hudson-ayers
Copy link
Contributor Author

bors r-

@hudson-ayers
Copy link
Contributor Author

Sorry, as soon as I saw you type the bors command I realized that I had changed the ci job names without updating bors.toml to match the new names

@hudson-ayers
Copy link
Contributor Author

(ready now, pending a review)

@alistair23
Copy link
Contributor

Can you run the emulation-check step as well?

ppannuto
ppannuto previously approved these changes May 4, 2020
Copy link
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

Well, arguably I should have noticed that too! :)

@ppannuto
Copy link
Member

ppannuto commented May 4, 2020

bors r-

Ah! Github cached page or something, sorry didn't see Alistair's comment!


Edit: Nope, just 16s
image

@hudson-ayers
Copy link
Contributor Author

rebased on master (so the qemu code was in the makefile) and added emulation-check to github actions and bors.toml

alistair23
alistair23 previously approved these changes May 4, 2020
ppannuto
ppannuto previously approved these changes May 4, 2020
Copy link
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

For real this time!

@hudson-ayers hudson-ayers dismissed stale reviews from ppannuto and alistair23 via 71b4dd1 May 4, 2020 18:55
@hudson-ayers
Copy link
Contributor Author

bors test

Copy link
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

@hudson-ayers
Copy link
Contributor Author

struggling to understand why bors is failing on this, it works on an identical PR in my test repository (Tock-CI#11). That PR has an identical upstream as well (though it doesn't have travis enabled?)

@ppannuto
Copy link
Member

ppannuto commented May 5, 2020

Interesting: merging this one manually because bors can't do it, specifically:

https://forum.bors.tech/t/resource-not-accessible-by-integration/408/2

GitHub Apps are not allowed to modify GitHub Actions workflow files.

@ppannuto ppannuto merged commit 74cf7e2 into tock:master May 5, 2020
@lschuermann lschuermann removed this from Work in progress in Project Tracking May 18, 2020
bors bot added a commit to tock/libtock-rs that referenced this pull request Jun 22, 2020
203: Add a CI tool that prints a size diff for incoming PRs. r=jrvanwhy a=jrvanwhy

The diff is computed by building the examples with `make examples` both in the PR's target branch and the merge result, and running `diff` on the result.

This is built using GitHub Actions because upstream Tock is moving to GitHub Actions (see tock/tock#1815, and [this core WG discussion](https://github.com/tock/tock/blob/master/doc/wg/core/notes/core-notes-2020-04-24.md#ci-infrastructure-travis-to-github-actions)).

Co-authored-by: Johnathan Van Why <jrvanwhy@google.com>
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.

None yet

5 participants