Skip to content

Set up GitHub Actions #276

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 4 commits into from
Oct 26, 2020
Merged

Set up GitHub Actions #276

merged 4 commits into from
Oct 26, 2020

Conversation

jonas-schievink
Copy link
Contributor

No description provided.

@rust-highfive
Copy link

r? @adamgreig

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m labels Oct 26, 2020
@jonas-schievink
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Oct 26, 2020
@jonas-schievink
Copy link
Contributor Author

bors try

@bors
Copy link
Contributor

bors bot commented Oct 26, 2020

try

Already running a review

@jonas-schievink
Copy link
Contributor Author

bors try-

@jonas-schievink
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Oct 26, 2020
@bors
Copy link
Contributor

bors bot commented Oct 26, 2020

try

Build succeeded:

@jonas-schievink
Copy link
Contributor Author

@adamgreig ready for review :)

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.

Thank you so much!! Looks good, couple of nits. Only shame is that merging this to master won't help #275 finally merge, sigh.

@adamgreig
Copy link
Member

Ok, looks good to me. My only remaining concern is a policy one around whether we want to enforce rustfmt by failing PRs that don't have it run; we don't currently do this on any (I think?) wg repos. I know it's a tired fight but I at least don't want it to happen de facto. Sadly there's presumably not many good options...

  • allowing CI to pass despite rustfmt failure makes it basically meaningless
  • no current way to have bors or github apply rustfmt for us
  • not doing it presumably means users with auto-rustfmt will keep adding accidental formatting changes to their PRs until it's all rustfmt-clean anyway

I don't auto-rustfmt so I'm sort of meh about the whole situation but I don't think I can muster any compelling arguments against.

On another note, I see the Clippy run fails because the PR is from a fork; do we expect that will work after this PR is merged, or should we change the clippy template too? I know GHA supports running the base branch's CI workflow on the PR's git commit, which might allow clippy annotations for PRs-from-forks, but maybe this will already work since it's what we're using elsewhere (or maybe it doesn't work elsewhere either, and we don't notice when making non-fork PRs?).

@jonas-schievink
Copy link
Contributor Author

I see the Clippy run fails because the PR is from a fork; do we expect that will work after this PR is merged, or should we change the clippy template too?

This unfortunately fundamentally doesn't work on GitHub Actions. I think the Clippy action will write the clippy output to the console instead though.

@adamgreig
Copy link
Member

This unfortunately fundamentally doesn't work on GitHub Actions. I think the Clippy action will write the clippy output to the console instead though.

Ah, I see for example over on probe-rs that's what we get (e.g.). I wonder if it's possible if we use pull_request_target instead of pull_request, which gives access to secrets to the action and might allow clippy to post annotations. I've used it in stm32-rs to be able to push generated files to another repository triggered by PRs from forks. Anyway we can experiment with it later, there's no need for it to block this.

@adamgreig
Copy link
Member

Oh, we already do rustfmt on cortex-m-rt, so that fight has been and gone. Let's do this!

bors merge

@jonas-schievink
Copy link
Contributor Author

I wonder if it's possible if we use pull_request_target instead of pull_request, which gives access to secrets to the action and might allow clippy to post annotations.

Oh, sounds like they added this specifically for clippy-style actions, neat!

@bors
Copy link
Contributor

bors bot commented Oct 26, 2020

Build succeeded:

@bors bors bot merged commit e42899b into rust-embedded:master Oct 26, 2020
@jonas-schievink jonas-schievink deleted the gha branch October 26, 2020 22:10
adamgreig added a commit that referenced this pull request Jan 12, 2022
276: Replace __ONCE__ with Cargo links key r=jonas-schievink a=adamgreig

This would fix #275. We use the links key in cortex-m already [here](https://github.com/rust-embedded/cortex-m/blob/master/Cargo.toml#L16). See also rust-embedded/wg#467.

Co-authored-by: Adam Greig <adam@adamgreig.com>
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-cortex-m
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants