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

[docs] Adds basic CI yaml for GitHub Actions #10212

Merged
merged 5 commits into from
Dec 20, 2021

Conversation

SamMorrowDrums
Copy link
Contributor

@SamMorrowDrums SamMorrowDrums commented Dec 17, 2021

Currently there is no documentation for GitHub Actions, so I have attempted to add an Actions Workflow that is equivalent to the other CI snippets in the file. You can view a successful run of this Action in my repo for experimenting with this here: https://github.com/SamMorrowDrums/rust-action-test/actions/runs/1593666172

The Rust code I tested it with is just the boilerplate from cargo init.

@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 @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 17, 2021
@SamMorrowDrums SamMorrowDrums changed the title Adds basic CI yaml for GitHub Actions [docs] Adds basic CI yaml for GitHub Actions Dec 17, 2021
@SamMorrowDrums
Copy link
Contributor Author

Thanks for the review @alexcrichton, it makes sense to remove 3rd party stuff, and simplify further. I think the on push trigger is the right one for this example. I definitely think this example is better for incorporating the comments.

- run: cargo test --verbose
```

This will test all three release channels, see [GitHub Actions documentation](https://docs.github.com/en/actions) for more information.

Choose a reason for hiding this comment

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

Worth calling out that any failure in nightly will fail the build here, unlike the other examples.

See https://github.com/actions/toolkit/issues/399

Choose a reason for hiding this comment

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

Though we could just update the run command to include ||: as a suffix for nightly.

https://unix.stackexchange.com/questions/78408/which-is-more-idiomatic-in-a-bash-script-true-or

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @misalcedo, while I agree that is an issue, what I think might be good is if at some point there is a better Actions link that is specific to Rust CI, then we could point people there - but I'm hesitant to add complexity to the basic script due to @alexcrichton's comments:

I think that continue-on-error and fail-fast would be removed because we don't necessarily have a great reason for deviating from the GitHub Actions defaults.

I think we should make it somewhat terse and not have too many extra bells and whistles.

These lead me to believe that we've reached an acceptable starting place for Rust users with the current version, and that we could improve this example once a better solution to actions/toolkit#399 is is available?

I'm open to suggestions for how to briefly explain this that CI will fail if any of the matrix jobs fail (and nightly being the one that is likely to without user error), but I think I'm otherwise going to leave this as is, and we should re-visit if anything changes that could improve the basic example or if documentation emerges that I could link out to.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to leave as a comment but otherwise for the copy/paste snippet I agree we shouldn't have it be too too complicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a note.

@SamMorrowDrums
Copy link
Contributor Author

OK @alexcrichton all taken on-board. I've added a note that should be this PR complete. If you would prefer that I drop the toolchain stuff, and just switch to the default workflow from GitHub Starter Workflows, just let me know - but otherwise I think this is good as-is.

@alexcrichton
Copy link
Member

@bors: r+

Seems reasonable to me, thanks!

@bors
Copy link
Contributor

bors commented Dec 20, 2021

📌 Commit deee87c has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 20, 2021
@bors
Copy link
Contributor

bors commented Dec 20, 2021

⌛ Testing commit deee87c with merge 47b869c...

@bors
Copy link
Contributor

bors commented Dec 20, 2021

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing 47b869c to master...

@bors bors merged commit 47b869c into rust-lang:master Dec 20, 2021
@SamMorrowDrums SamMorrowDrums deleted the patch-1 branch December 20, 2021 17:19
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 5, 2022
Update cargo

10 commits in fcef61230c3b6213b6b0d233a36ba4ebd1649ec3..358e79fe56fe374649275ca7aebaafd57ade0e8d
2021-12-17 02:30:38 +0000 to 2022-01-04 18:39:45 +0000
- Make rmeta_required no longer depend on whether timing is enabled (rust-lang/cargo#10254)
- The first version of pull request template (rust-lang/cargo#10218)
- Stabilize the `strip` profile option, now that rustc has stable `-C strip` (rust-lang/cargo#10088)
- Update docs for windows ssh-agent. (rust-lang/cargo#10248)
- Fix typo: substract -> subtract (rust-lang/cargo#10244)
- timings: Fix tick mark alignment (rust-lang/cargo#10239)
- Remove unused lifetimes (rust-lang/cargo#10238)
- Make levenshtein distance case insensitive. (rust-lang/cargo#10224)
- [docs] Adds basic CI yaml for GitHub Actions (rust-lang/cargo#10212)
- Add function for parsing already-read manifest (rust-lang/cargo#10209)
@ehuss ehuss added this to the 1.59.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants