Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Move polkadot build check to polkadot and check status in substrate #6706

Closed
wants to merge 37 commits into from
Closed

Move polkadot build check to polkadot and check status in substrate #6706

wants to merge 37 commits into from

Conversation

cecton
Copy link
Contributor

@cecton cecton commented Jul 22, 2020 •
edited
Loading

More details in paritytech/polkadot#1426

polkadot companion: paritytech/polkadot#1426

cecton and others added 30 commits July 14, 2020 13:50
@cecton
Initial commit
a245e96 
Forked at: 60e3a69
Parent branch: origin/master
@cecton
Move task_manager.rs to mod.rs
fe1c372
@cecton
Graceful shutdown for the task manager
163501a
@cecton
Await all background task JoinHandle at the same time
cf228c0
@cecton
Add tests
551268b
@cecton
Make future() wait also for exit signal + fix essential task failed
b3c0648 
Probably related to paritytech/cumulus#111
@cecton
add comments for non-obvious code
2855ada
@cecton
Use clean_shutdown() in sc-cli
db34d96
@cecton
Adapt code and upgrade tokio in sc-cli
2dcd0b3
@cecton
cleanup spacing in doc
42333f4
@cecton
Add license
1198324
@cecton
I guess actually running the clean shutdown would be a good idea
56c8139
@cecton
fix tests
ef3a054
@gnunicorn
Merge remote-tracking branch 'origin/master' into cecton-async-gracef…
6f656cf 
…ul-shutdown
@cecton @gnunicorn
Update client/cli/src/runner.rs
4e6b8d2 
Co-authored-by: Benjamin Kampmann <ben@gnunicorn.org>
@cecton
Improve error logging
5dbe179
@cecton
disable other tests (can't reproduce on my machine)
c133c59
@cecton
Revert "disable other tests (can't reproduce on my machine)"
1fb6481 
This reverts commit c133c59.
@cecton
It is possible that the tasks are ended first
502aba4
@cecton
Revert "It is possible that the tasks are ended first"
1b91a8c 
This reverts commit 502aba4.
@cecton
Use single threaded scheduler for more predictability
ee5e13c
@cecton
enable_time
4e15214
@cecton
Revert "enable_time"
fbcefa4 
This reverts commit 4e15214.
@cecton
Revert "Use single threaded scheduler for more predictability"
23b642a 
This reverts commit ee5e13c.
@cecton
Revert "Revert "It is possible that the tasks are ended first""
5b5becb 
This reverts commit 1b91a8c.
@cecton
This cannot be verified either with a threaded pool
bc431b6
@cecton
Merge commit 1be0295 (no conflict)
aaf038c 
Parent branch: origin/master
Forked at: 4da2926
@cecton
Merge commit 47be8d9 (no conflict)
2a1a2ac 
Parent branch: origin/master
Forked at: 4da2926
@cecton
Merge commit 5c43b2b (no conflict)
c079e31 
Parent branch: origin/master
Forked at: 4da2926
@cecton
Merge commit cd67889 (no conflict)
fbcb752 
Parent branch: origin/master
Forked at: 4da2926
cecton and others added 3 commits July 21, 2020 18:48
@cecton
Merge commit 86f8594 (conflicts)
e4ad84a 
Parent branch: origin/master
Forked at: 4da2926
@cecton @bkchr
Apply suggestions from code review
b5e5fe2 
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
@cecton
Merge commit e3bb2ce (no conflict)
5fd98a6 
Parent branch: origin/master
Forked at: 4da2926
@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Jul 22, 2020
@cecton cecton mentioned this pull request Jul 22, 2020
Closed
@cecton cecton force-pushed the cecton-ignore-test-for-companion branch from a66a759 to 5f07368 Compare July 22, 2020 06:24
@cecton
Merge commit 5c43b2b (no conflict)
233397c 
Parent branch: origin/master
Forked at: 4da2926
@cecton cecton added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jul 22, 2020
@cecton cecton requested a review from TriplEight July 22, 2020 07:23
@cecton
Merge commit 5fd98a6 (no conflict)
7b0359f 
Parent branch: origin/master
Forked at: 4da2926
@cecton
Copy link
Contributor Author

cecton commented Jul 22, 2020

@TriplEight as you can see now the polkadot-companion-status step fails because the polkadot companion PR has not been approved yet.

(I still need to finish some things in polkadot .gitlab-ci.yml)

TriplEight
TriplEight approved these changes Jul 22, 2020
View reviewed changes
Copy link
Contributor

@TriplEight TriplEight left a comment

Choose a reason for hiding this comment

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

CI and bash parts look good!

@cecton
Copy link
Contributor Author

cecton commented Jul 22, 2020

@TriplEight It is almost ready to merge but I won't be able to do it alone:

  1. The build in polkadot must pass. It started failing but I couldn't reproduce it locally. Logs here.
  2. The requirement for continuous-integration/gitlab-check-polkadot-companion-build on GitHub must be removed (as this step has been removed)
  3. I updated the doc but I think it would be best to make an announcement on the channel because with this change, the companion PR must be in the description of the polkadot pr too (both links must be present)

(The reason for 3. is because on substrate it checks the status of the polkadot PR while on Polkadot it needs to know which branch of substrate needs to be used for the build)

@s3krit
Copy link
Contributor

s3krit commented Jul 22, 2020

We should probably also remove or edit .github/workflows/polkadot-companion-labels.yml also - Its current purpose is to monitor the status of the companion-build job and apply a label to the PR if it fails. I get the suspicion it's not actually been that useful an action anyway.

  1. The requirement for continuous-integration/gitlab-check-polkadot-companion-build on GitHub must be removed (as this step has been removed)

We'll need to get someone in the dev-admin team to do this.

@cecton
Copy link
Contributor Author

cecton commented Jul 22, 2020

(Wait before doing so because there is still this problem of Cargo.lock to update in polkadot)

@cecton
Copy link
Contributor Author

cecton commented Jul 22, 2020

I'm closing this PR because after discussion (and checking history) I realized that this solution won't solve the issue I currently have with the companion pr process.

@cecton cecton closed this Jul 22, 2020
@cecton cecton deleted the cecton-ignore-test-for-companion branch July 22, 2020 14:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TriplEight TriplEight TriplEight approved these changes

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cecton @s3krit @TriplEight @gnunicorn