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

Switch to GitHub Actions - Part 2 - From within #5088

Merged
merged 30 commits into from
Feb 12, 2020
Merged

Switch to GitHub Actions - Part 2 - From within #5088

merged 30 commits into from
Feb 12, 2020

Conversation

flip1995
Copy link
Member

@flip1995 flip1995 commented Jan 24, 2020

This is a continuation of #5071. This time from a branch inside the rust-lang/rust-clippy repo, not from my fork, since secrets are not available in PRs from forks.

Copying the description of #5071 to here:

Closes #4577

This is just an experiment. I don't think we have a consensus if we should move away from travis/appveyor. We have consensus: #5071 (comment)

GHA would let us run up to 20 concurrent jobs. Since we have 15 integration tests and 4 (linux, linux 32-bit, macos, windows) basic tests, we would be able to run everything concurrently. The org has a limit of 60 jobs across the org, so we limit the matrix of the integration tests to 6 concurrent jobs.

Also IIUC we only have to build Clippy once for every initegration test and then only check the repos. Nope, dependent jobs exist, but they won't keep the artifacts (not even the checkout).

TODO before merge:

changelog: none

@flip1995 flip1995 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 25, 2020
@flip1995
Copy link
Member Author

flip1995 commented Jan 25, 2020

I also created this GHA: flip1995@43d3391

It automatically tags Clippy 1 day after a rust release. But it's pretty hacky, since it updates itself, after tagging the commit by modifying the GHA yaml file.

@flip1995

This comment has been minimized.

Copy link
Member

@phansch phansch left a comment

Choose a reason for hiding this comment

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

Thanks a lot for pushing this through! I think GHA will make things much easier for us.

My comments are only about the shell code in the CI config. I'm generally not a fan of having lots of shell code inside yml files because of the missing syntax highlighting, missing shellcheck support and more possibilities for weird yml syntax mistakes. I think the rust repo also moved lots of their shell code to separate files for the same reasons.

.github/workflows/clippy.yml Outdated Show resolved Hide resolved
.github/workflows/clippy_bors.yml Show resolved Hide resolved
.github/workflows/deploy.yml Outdated Show resolved Hide resolved
@flip1995

This comment has been minimized.

@bors

This comment has been minimized.

bors added a commit that referenced this pull request Jan 30, 2020
[WIP][DNM] Switch to GitHub Actions - Part 2 - From within

This is a continuation of #5071. This time from a branch inside the rust-lang/rust-clippy repo, not from my fork, since secrets are not available in PRs from forks.

Copying the description of #5071 to here:

Closes #4577

~~This is just an experiment. I don't think we have a consensus _if_ we should move away from travis/appveyor.~~ We have consensus: #5071 (comment)

~~GHA would let us run up to 20 concurrent jobs. Since we have 15 integration tests and 4 (linux, linux 32-bit, macos, windows) basic tests, we would be able to run everything concurrently.~~ The org has a limit of 60 jobs across the org, so we limit the matrix of the integration tests to 6 concurrent jobs.

~~Also IIUC we only have to build Clippy once for every initegration test and then only check the repos.~~ Nope, dependent jobs exist, but they won't keep the artifacts (not even the checkout).

TODO before merge:

- [x] Add `DEPLOY_KEY` secret to github repo
- [x] test deployment on test branch `gh-test`#
  - [x] Test normal deployment
  - [x] Test deployment no changes
  - [x] Test deployment of tag
- [x] talk with `@rust-lang/infra` for bors, `@rust-lang/infra` is good with the move (crater also uses GHA+bors)
- [x] ~~Get remark + clippy_dev check to work on pushes (https://github.saobby.my.eu.orgmunity/t5/GitHub-Actions/~Builds-are-not-triggered-with-on-paths/m-p/44075; I contacted GH support about this)~~ That seems to start working again yesterday 🤔 Let's hope it keeps working.
- [ ] impl homu checks for GHA #5071 (comment)
- [ ] Add GHA badge to Cargo.toml (blocked on `rust-lang/crates.io#1838`)
- [ ] Add back travis + appveyor files for transition period (?)

changelog: none
@flip1995

This comment has been minimized.

@bors

This comment has been minimized.

bors added a commit that referenced this pull request Jan 30, 2020
[WIP][DNM] Switch to GitHub Actions - Part 2 - From within

This is a continuation of #5071. This time from a branch inside the rust-lang/rust-clippy repo, not from my fork, since secrets are not available in PRs from forks.

Copying the description of #5071 to here:

Closes #4577

~~This is just an experiment. I don't think we have a consensus _if_ we should move away from travis/appveyor.~~ We have consensus: #5071 (comment)

~~GHA would let us run up to 20 concurrent jobs. Since we have 15 integration tests and 4 (linux, linux 32-bit, macos, windows) basic tests, we would be able to run everything concurrently.~~ The org has a limit of 60 jobs across the org, so we limit the matrix of the integration tests to 6 concurrent jobs.

~~Also IIUC we only have to build Clippy once for every initegration test and then only check the repos.~~ Nope, dependent jobs exist, but they won't keep the artifacts (not even the checkout).

TODO before merge:

- [x] Add `DEPLOY_KEY` secret to github repo
- [x] test deployment on test branch `gh-test`#
  - [x] Test normal deployment
  - [x] Test deployment no changes
  - [x] Test deployment of tag
- [x] talk with `@rust-lang/infra` for bors, `@rust-lang/infra` is good with the move (crater also uses GHA+bors)
- [x] ~~Get remark + clippy_dev check to work on pushes (https://github.saobby.my.eu.orgmunity/t5/GitHub-Actions/~Builds-are-not-triggered-with-on-paths/m-p/44075; I contacted GH support about this)~~ That seems to start working again yesterday 🤔 Let's hope it keeps working.
- [ ] impl homu checks for GHA #5071 (comment)
- [ ] Add GHA badge to Cargo.toml (blocked on `rust-lang/crates.io#1838`)
- [ ] Add back travis + appveyor files for transition period (?)

changelog: none
@flip1995

This comment has been minimized.

@bors

This comment has been minimized.

bors added a commit that referenced this pull request Jan 30, 2020
[WIP][DNM] Switch to GitHub Actions - Part 2 - From within

This is a continuation of #5071. This time from a branch inside the rust-lang/rust-clippy repo, not from my fork, since secrets are not available in PRs from forks.

Copying the description of #5071 to here:

Closes #4577

~~This is just an experiment. I don't think we have a consensus _if_ we should move away from travis/appveyor.~~ We have consensus: #5071 (comment)

~~GHA would let us run up to 20 concurrent jobs. Since we have 15 integration tests and 4 (linux, linux 32-bit, macos, windows) basic tests, we would be able to run everything concurrently.~~ The org has a limit of 60 jobs across the org, so we limit the matrix of the integration tests to 6 concurrent jobs.

~~Also IIUC we only have to build Clippy once for every initegration test and then only check the repos.~~ Nope, dependent jobs exist, but they won't keep the artifacts (not even the checkout).

TODO before merge:

- [x] Add `DEPLOY_KEY` secret to github repo
- [x] test deployment on test branch `gh-test`#
  - [x] Test normal deployment
  - [x] Test deployment no changes
  - [x] Test deployment of tag
- [x] talk with `@rust-lang/infra` for bors, `@rust-lang/infra` is good with the move (crater also uses GHA+bors)
- [x] ~~Get remark + clippy_dev check to work on pushes (https://github.saobby.my.eu.orgmunity/t5/GitHub-Actions/~Builds-are-not-triggered-with-on-paths/m-p/44075; I contacted GH support about this)~~ That seems to start working again yesterday 🤔 Let's hope it keeps working.
- [ ] impl homu checks for GHA #5071 (comment)
- [ ] Add GHA badge to Cargo.toml (blocked on `rust-lang/crates.io#1838`)
- [ ] Add back travis + appveyor files for transition period (?)
@flip1995

This comment has been minimized.

@bors

This comment has been minimized.

@flip1995
Copy link
Member Author

@bors treeclosed=1000

@flip1995
Copy link
Member Author

@bors p=1000

@flip1995
Copy link
Member Author

@bors r=Manishearth,flip1995

Let's go 🚀

@bors
Copy link
Collaborator

bors commented Feb 12, 2020

📌 Commit a0a21a8 has been approved by Manishearth,flip1995

@bors
Copy link
Collaborator

bors commented Feb 12, 2020

⌛ Testing commit a0a21a8 with merge fcb231f...

bors added a commit that referenced this pull request Feb 12, 2020
Switch to GitHub Actions - Part 2 - From within

This is a continuation of #5071. This time from a branch inside the rust-lang/rust-clippy repo, not from my fork, since secrets are not available in PRs from forks.

Copying the description of #5071 to here:

Closes #4577

~~This is just an experiment. I don't think we have a consensus _if_ we should move away from travis/appveyor.~~ We have consensus: #5071 (comment)

~~GHA would let us run up to 20 concurrent jobs. Since we have 15 integration tests and 4 (linux, linux 32-bit, macos, windows) basic tests, we would be able to run everything concurrently.~~ The org has a limit of 60 jobs across the org, so we limit the matrix of the integration tests to 6 concurrent jobs.

~~Also IIUC we only have to build Clippy once for every initegration test and then only check the repos.~~ Nope, dependent jobs exist, but they won't keep the artifacts (not even the checkout).

TODO before merge:

- [x] Add `DEPLOY_KEY` secret to github repo
- [x] test deployment on test branch `gh-test`#
  - [x] Test normal deployment
  - [x] Test deployment no changes
  - [x] Test deployment of tag
- [x] talk with `@rust-lang/infra` for bors, `@rust-lang/infra` is good with the move (crater also uses GHA+bors)
- [x] ~~Get remark + clippy_dev check to work on pushes (https://github.saobby.my.eu.orgmunity/t5/GitHub-Actions/~Builds-are-not-triggered-with-on-paths/m-p/44075; I contacted GH support about this) ~~That seems to start working again yesterday. Let's hope it keeps working.~~ Or not: df9be48. Now it works again: 723786a. I think we should wait, until this is reliable. It appears, that it doesn't work on force pushes (sometimes?): 5814142~~ We need to run the bors tests unconditionally anyway (47138d1) so it doesn't really matter.
- [x] ~~impl homu checks for GHA #5071 (comment) -- I prepared: flip1995/rust-central-station@f40230d. I'd suggest to first add GHA and keep the travis and appveyor checks for a few days and to remove them in a second pass. The bors dummy jobs are added in 1a83b7a and work as expected: #5088 (comment). I opened rust-lang/rust-central-station#578 See #5088 (comment)
- [x] ~Add GHA badge to Cargo.toml (blocked on rust-lang/crates.io # 1838)~ Added a FIXME in
2332b57
- [x] ~Maybe we should also wait until GHA supports yaml anchors. https://github.saobby.my.eu.orgmunity/t5/GitHub-Actions/Support-for-YAML-anchors/td-p/30336/~ WIll probably not be implemented in the near future.
- [x] Add back travis + appveyor files for transition period (!)

changelog: none
This dir was generated by compiling RTIM, when no cache is available
@flip1995
Copy link
Member Author

@bors r=Manishearth,flip1995

@bors
Copy link
Collaborator

bors commented Feb 12, 2020

📌 Commit 3fac289 has been approved by Manishearth,flip1995

@bors
Copy link
Collaborator

bors commented Feb 12, 2020

⌛ Testing commit 3fac289 with merge 71a5379...

bors added a commit that referenced this pull request Feb 12, 2020
Switch to GitHub Actions - Part 2 - From within

This is a continuation of #5071. This time from a branch inside the rust-lang/rust-clippy repo, not from my fork, since secrets are not available in PRs from forks.

Copying the description of #5071 to here:

Closes #4577

~~This is just an experiment. I don't think we have a consensus _if_ we should move away from travis/appveyor.~~ We have consensus: #5071 (comment)

~~GHA would let us run up to 20 concurrent jobs. Since we have 15 integration tests and 4 (linux, linux 32-bit, macos, windows) basic tests, we would be able to run everything concurrently.~~ The org has a limit of 60 jobs across the org, so we limit the matrix of the integration tests to 6 concurrent jobs.

~~Also IIUC we only have to build Clippy once for every initegration test and then only check the repos.~~ Nope, dependent jobs exist, but they won't keep the artifacts (not even the checkout).

TODO before merge:

- [x] Add `DEPLOY_KEY` secret to github repo
- [x] test deployment on test branch `gh-test`#
  - [x] Test normal deployment
  - [x] Test deployment no changes
  - [x] Test deployment of tag
- [x] talk with `@rust-lang/infra` for bors, `@rust-lang/infra` is good with the move (crater also uses GHA+bors)
- [x] ~~Get remark + clippy_dev check to work on pushes (https://github.saobby.my.eu.orgmunity/t5/GitHub-Actions/~Builds-are-not-triggered-with-on-paths/m-p/44075; I contacted GH support about this) ~~That seems to start working again yesterday. Let's hope it keeps working.~~ Or not: df9be48. Now it works again: 723786a. I think we should wait, until this is reliable. It appears, that it doesn't work on force pushes (sometimes?): 5814142~~ We need to run the bors tests unconditionally anyway (47138d1) so it doesn't really matter.
- [x] ~~impl homu checks for GHA #5071 (comment) -- I prepared: flip1995/rust-central-station@f40230d. I'd suggest to first add GHA and keep the travis and appveyor checks for a few days and to remove them in a second pass. The bors dummy jobs are added in 1a83b7a and work as expected: #5088 (comment). I opened rust-lang/rust-central-station#578 See #5088 (comment)
- [x] ~Add GHA badge to Cargo.toml (blocked on rust-lang/crates.io # 1838)~ Added a FIXME in
2332b57
- [x] ~Maybe we should also wait until GHA supports yaml anchors. https://github.saobby.my.eu.orgmunity/t5/GitHub-Actions/Support-for-YAML-anchors/td-p/30336/~ WIll probably not be implemented in the near future.
- [x] Add back travis + appveyor files for transition period (!)

changelog: none
@flip1995
Copy link
Member Author

flip1995 commented Feb 12, 2020

The bors test finished job fail was, because I had to restart the macos check on GHA, because the master toolchain was downloaded with 50kb/s. But then again, I also had to restart the travis macos job because of a timeout.

We should look at the macos job in the next few merges though, since it didn't happen for the first time.

Running the Clippy Test (bors) workflow twice (you can't restart a single job currently) was as fast as running the travis build once.

@bors
Copy link
Collaborator

bors commented Feb 12, 2020

☀️ Test successful - checks-travis, status-appveyor
Approved by: Manishearth,flip1995
Pushing 71a5379 to master...

@bors bors merged commit 3fac289 into master Feb 12, 2020
@flip1995 flip1995 deleted the gha branch February 12, 2020 09:55
@flip1995
Copy link
Member Author

Deployment works for master pushes: f0da14e

Releases are hard to test

bors added a commit that referenced this pull request Feb 19, 2020
…logiq,flip1995

I like to move it, move it

GHA now runs in the background for 6 days (#5088)

Since then ~~15~~ 19 PRs were successfully merged and Travis+Appveyor agreed on the status in every case. ([GitHub PR search query](https://github.com/rust-lang/rust-clippy/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Amerged+merged%3A%3E%3D2020-02-12T15%3A42%3A00+sort%3Aupdated-desc+NOT+%5Bgh-pages%5D+in%3Atitle))

Some PRs were:
- #5163
- #5170
- #5168
- #5173
- #5171
- #5156
- #4809
- #5177
- #5182
- #5183
- #5184
- #5185
- #5186
- #5181
- #5189

Bug with GHA:
- When a rustc PR gets merged between the `integration_build` and the `integration` job, the `integration` job will fail. This happened once in #5162, but not in the past 6 days. Even if it would happen every 4th PR we would save time, since splitting up the integration build and tests saves 5-7 minutes per run and a complete run takes 15-17 minutes
- Sometimes the MacOS build takes up to an hour to download the master toolchain. Until now, this happend 2 or 3 times and can be resolved by a `@bors r3try`+canceling the previous run (restarting single jobs is not supported yet)

## Before merging this, rust-lang/rust-central-station#578 has to get merged

This PR is for starting the discussion and to get consensus (@rust-lang/clippy) on a final move to GHA. If we're ready, I'll contact Pietro, to finalize the move.

changelog: Clippy completely runs on GHA now 🎉

---

BTW: The deployment already runs on GHA, instead of Travis.
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider switching CI to Azure Pipelines
5 participants