Skip to content

Switch to using GitHub Actions for CI #72

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

Merged
merged 12 commits into from
Aug 8, 2021

Conversation

robyoung
Copy link
Contributor

@robyoung robyoung commented Aug 3, 2021

I have tried to keep it functionally identical to the travis workflow. This is a bit tricky for me to test.

There are some minor changes though.

  • Added MSRV of 1.51.0 - I just picked a version out of thin air here.
  • I could not see an easy way of preventing master branch runs for PRs that have passed so this should run on all.
  • For deploying to github pages I'm using a 3rd party action. It looks reasonable and just uses the default repo scoped token.

I have tried to keep it functionally identical to the travis workflow.
There are some minor changes though.

- Added MSRV of 1.51.0 - I just picked a version out of thin air here.
- I could not see an easy way of preventing master branch runs for PRs
  that have passed so this should run on all.
- For deploying to github pages I'm using a 3rd party action. It looks
  reasonable and just uses the default repo scoped token.
@robyoung robyoung requested a review from a team as a code owner August 3, 2021 16:24
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @andre-richter (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-resources labels Aug 3, 2021
@robyoung robyoung mentioned this pull request Aug 3, 2021
Copy link
Member

@adamgreig adamgreig left a comment

Choose a reason for hiding this comment

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

Wow, thanks for putting this together!

On the whole this looks good but I'd like for it to be slightly more in line with our various other mdbook repos to make ongoing maintenance easier:

The main things to change are:

  • use cargo to install mdbook and cache it: example
  • use peaceiris/actions-gh-pages@v3 for gh-pages: example
  • update bors.toml to require the new check passes, instead of the old one (example)

Hopefully those aren't too bad to do, and thanks again for the PR!


jobs:
build:
runs-on: ubuntu-latest
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
runs-on: ubuntu-latest
runs-on: ubuntu-20.04

We've mostly specified specific versions just to ensure there are no surprises when -latest next changes. Same deal applies to the deploy job below, though you could also make that a final and optional step in the build job, I don't mind either way.

@robyoung
Copy link
Contributor Author

robyoung commented Aug 4, 2021

That's great. Thanks for the review. I'll take a look at it later today.

@robyoung
Copy link
Contributor Author

robyoung commented Aug 4, 2021

@adamgreig I have tried to follow the other book repos as much as possible. However, it looks like this repo does a bit more testing so it feels like it needs to be tested across different versions of rust, which the other workflows do not.

Copy link
Member

@adamgreig adamgreig left a comment

Choose a reason for hiding this comment

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

Thanks, looks good! Couple last changes then let's give it a go. Sadly since no actions have been run I don't have a button to try running them for this PR, but once the bors.toml file is updated I can have bors try it instead.

  • I think we can delete ci/install.sh and the new ci/install-mdbook.sh now?
  • Currently nightly builds failing will fail CI, but we probably want to allow nightly to fail; there's an example here
  • .github/bors.toml needs updating or bors will refuse to merge this PR
  • .travis.yml can be deleted too

robyoung and others added 2 commits August 5, 2021 11:19
- Remove redundant install and travis config files
- Allow nightly to fail
- Update bors.toml (I was sure I had done this)
adamgreig
adamgreig previously approved these changes Aug 6, 2021
Copy link
Member

@adamgreig adamgreig left a comment

Choose a reason for hiding this comment

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

Great, thank you again! Let's get this merged and hopefully it works, then I'll merge #70 afterwards.

bors merge

bors bot added a commit that referenced this pull request Aug 6, 2021
72: Switch to using GitHub Actions for CI r=adamgreig a=robyoung

I have tried to keep it functionally identical to the travis workflow. This is a bit tricky for me to test.

There are some minor changes though.
- Added MSRV of 1.51.0 - I just picked a version out of thin air here.
- I could not see an easy way of preventing master branch runs for PRs that have passed so this should run on all.
- For deploying to github pages I'm using a 3rd party action. It looks reasonable and just uses the default repo scoped token.

Co-authored-by: Rob Young <[email protected]>
Co-authored-by: Adam Greig <[email protected]>
@bors
Copy link
Contributor

bors bot commented Aug 6, 2021

Canceled.

@adamgreig
Copy link
Member

Ugh, that's enough whack-a-mole for tonight. Please don't be put off, this is mostly the test script needing some updates for the latest version of nm. The usual game of fixing one CI bug and then finding the next... If you can work out why arm-none-eabi-gcc isn't being found even though it's apparently installed please go ahead, but otherwise I'll try again tomorrow.

@adamgreig adamgreig force-pushed the github-actions branch 6 times, most recently from 1e62dd7 to 0182f7e Compare August 7, 2021 00:58
@adamgreig adamgreig force-pushed the github-actions branch 9 times, most recently from c090191 to df623d6 Compare August 8, 2021 01:18
@adamgreig
Copy link
Member

Whew, that was an endeavour. It turns out you can't copy arm-none-eabi-gcc into a new directory, it needs to stay relative to the directory structure it was extracted from. Who knew.

bors merge

@bors
Copy link
Contributor

bors bot commented Aug 8, 2021

👎 Rejected by too few approved reviews

adamgreig
adamgreig previously approved these changes Aug 8, 2021
Copy link
Member

@adamgreig adamgreig left a comment

Choose a reason for hiding this comment

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

bors merge

bors bot added a commit that referenced this pull request Aug 8, 2021
72: Switch to using GitHub Actions for CI r=adamgreig a=robyoung

I have tried to keep it functionally identical to the travis workflow. This is a bit tricky for me to test.

There are some minor changes though.
- Added MSRV of 1.51.0 - I just picked a version out of thin air here.
- I could not see an easy way of preventing master branch runs for PRs that have passed so this should run on all.
- For deploying to github pages I'm using a 3rd party action. It looks reasonable and just uses the default repo scoped token.

Co-authored-by: Rob Young <[email protected]>
Co-authored-by: Adam Greig <[email protected]>
@adamgreig
Copy link
Member

bors cancel

@bors
Copy link
Contributor

bors bot commented Aug 8, 2021

Canceled.

Copy link
Member

@adamgreig adamgreig left a comment

Choose a reason for hiding this comment

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

bors merge

@bors
Copy link
Contributor

bors bot commented Aug 8, 2021

Build succeeded:

@bors bors bot merged commit 4dfdf69 into rust-embedded:master Aug 8, 2021
@adamgreig
Copy link
Member

🎉 thanks @robyoung!

@adamgreig adamgreig mentioned this pull request Aug 8, 2021
@robyoung
Copy link
Contributor Author

robyoung commented Aug 8, 2021

Woop! Thank you for fixing it. I was totally stumped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-resources
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants