[CI] Add cumulus + beefy companions #9010

s3krit · 2021-06-03T12:17:32Z

Adds companion builds to ensure we don't break cumulus & grandpa-bridge-gadget repos.
Actually builds out a generic companion-build script which takes a repo and organisation (& an optional build string) as an argument. Initially for cumulus and beefy we just run a cargo check but I'm open to changing that to something more appropriate if we feel that would be useful.
We can also specify additional dependencies. Consider the case of Cumulus:

Cumulus depends on polkadot and substrate
Polkadot also depends on substrate
If there is a companion PR for polkadot, when building cumulus we also need to tell cumulus to use the companion version of polkadot.

There will be a followup PR that breaks this out into another repo so we can add companion builds to other projects that have dependents we don't want to break, but baby steps :)

Initially, we're almost definitely going to have some teething problems with this PR so I don't want the statuses to be required yet... that can come later.

NOTE: For the companion to be properly detected, you need to comment with the actual name of the repo (see below's beefy companion - you can't just put 'beefy companion:')

Closes: https://github.com/paritytech/ci_cd/issues/139

Polkadot companion: paritytech/polkadot#3379
Cumulus companion: paritytech/cumulus#518
grandpa-bridge-gadget companion: paritytech/grandpa-bridge-gadget#223

adoerr · 2021-06-03T12:33:47Z

While you are at it, do you think it would be possible to include BEEFY as well? Please!! 😄

s3krit · 2021-06-03T14:33:50Z

While you are at it, do you think it would be possible to include BEEFY as well? Please!! smile

since you asked nicely ;)

This reverts commit 6fb44bb.

.gitlab-ci.yml

.maintain/gitlab/check_companion_build.sh

bkchr

When there is a polkadot companion, you will also need to overwrite polkadot in cumulus to use this companion. As otherwise it will fail to build

.maintain/gitlab/check_companion_build.sh

.gitlab-ci.yml

bkchr · 2021-06-29T14:21:59Z

.gitlab-ci.yml

+  variables:
+    COMPANION_ORG: paritytech
+    COMPANION_REPO: cumulus
+    COMPANION_BUILD: "cargo check"


Hmm this isn't really great :P I can understand why you are doing this, but honestly I don't know...

Overall it would be nice to have pre merge checks that runs the entire test suite of cumulus to make sure we don't break anything..

You mean it's better to trigger the entire cumulus CI here? Is it worth doing on every commit? Maybe on some certain files/dirs changes at least?

I mean for cumulus it is not that important at the moment. A cargo test --all --release would do it as well. However, even that takes some time. I would actually like to have this as a precommit check, but we don't have support for this :(

yeah, we're kinda stuck between not being able to use the existing tools for pre-merge pipelines and the inability to use some kind of a pre-commit tool with github.com. This means we'll be looking into how to deal with it nicely, but currently, we're scaling vertically.

I suggest we could generalize these checks by actually triggering the CIs of the relevant repository. As we've been doing it with Simnet recently. This script passes overriding variables to the downstream CI and returns the status of all checks.
It will require some changes in the relevant repos, (with which @paritytech/ci will certainly help), but will be more future-proof.

Now, this triggering script looks like this, it's a bit more specialized for Simnet.

bkchr · 2021-06-29T14:22:38Z

.gitlab-ci.yml

+  variables:
+    COMPANION_ORG: paritytech
+    COMPANION_REPO: cumulus
+    COMPANION_BUILD: "cargo check"


Suggested change

COMPANION_BUILD: "cargo check"

COMPANION_BUILD: "cargo check --benches --all --tests"

How about cargo check --all-targets --workspace?

--all is deprecated in favour of --workspace https://doc.rust-lang.org/cargo/commands/cargo-check.html
--all-targets = --lib --bins --tests --benches --examples

bkchr · 2021-06-29T14:22:45Z

.gitlab-ci.yml

+  variables:
+    COMPANION_ORG: paritytech
+    COMPANION_REPO: grandpa-bridge-gadget
+    COMPANION_BUILD: "cargo check"


Suggested change

COMPANION_BUILD: "cargo check"

COMPANION_BUILD: "cargo check --benches --all --tests"

joao-paulo-parity · 2021-06-29T15:24:05Z

.maintain/common/lib.sh

+
+  pr_body="$(sed -n -r 's/^[[:space:]]+"body": (".*")[^"]+$/\1/p' "${pr_data_file}")"
+
+  echo "${pr_body}" | sed -n -r \


Does sed account for newlines in the JSON string?

This probably never happens but you might have a comment like this:

Something something companion Bla bla another pull request: https://github.com/foo/pull/1

As I understand it, since you're not matching [Cc]ompanion: [regex] all in the same line, that comment would be detected although it should not be.

Using echo -e will output the JSON newlines as actual newlines so you'll be able to match it line-by-line.

joao-paulo-parity · 2021-06-29T15:28:02Z

.maintain/gitlab/check_companion_build.sh

+#shellcheck source=../common/lib.sh
+. "$(dirname "${0}")/../common/lib.sh"
+
+ORGANISATION=$1


nit: Apparently "Organisation" is valid spelling in British English but IMO it'd look better ORGANIZATION

joao-paulo-parity · 2021-06-29T15:35:59Z

.maintain/gitlab/check_companion_build.sh

+if expr match "${CI_COMMIT_REF_NAME}" '^[0-9]\+$' >/dev/null
+then
+  boldprint "this is pull request no ${CI_COMMIT_REF_NAME}"
+  pr_companion="$(get_companion "paritytech/substrate" "$CI_COMMIT_REF_NAME" "$ORGANISATION/$REPO")"


I think I get why "paritytech/substrate" is hardcoded for now (because this script is only used in this repository at the moment?) but it'd be good to have this as another variable for clarity's sake

joao-paulo-parity · 2021-06-29T15:41:39Z

.maintain/gitlab/check_companion_build.sh

+  diener_commands["paritytech/grandpa-bridge-gadget"]='--beefy'
+
+  for dep in "${DEPS[@]}"; do
+    dep_companion="$(get_companion "paritytech/substrate" "$CI_COMMIT_REF_NAME" "$dep")"


since you're already fetching the description once in the lines above, it would be good to reuse it instead of making more API requests

joao-paulo-parity · 2021-06-29T15:47:19Z

.maintain/gitlab/check_companion_build.sh

+      git -C "$dep" checkout "pr/${dep_companion}"
+      git -C "$dep" merge origin/master
+      # Tell this dependency to use the version of substrate in this PR
+      diener patch --crates-to-patch "$SUBSTRATE_DIR" --substrate --path "$dep/Cargo.toml"


SUBSTRATE_DIR and --substrate will have a rework to be dynamic once this script is made more general, right?

joao-paulo-parity · 2021-06-29T15:58:24Z

.maintain/gitlab/check_companion_build.sh

+fi
+
+# Test pr or master branch with this Substrate commit.
+diener patch --crates-to-patch ".." --substrate --path "Cargo.toml"


Changing this ".." to "$SUBSTRATE_DIR" makes the script less confusing to follow. It makes sense because you're doing cd at the start but it's not immediately obvious which is the current directory when this line is reached.

joao-paulo-parity · 2021-06-29T16:02:03Z

.maintain/gitlab/check_companion_build.sh

+git fetch --depth 100 origin
+git merge origin/master


can it just be git pull origin master? I don't quite get the depth numbers but fetching a single branch is better than doing fetch origin (which fetches everything)

joao-paulo-parity · 2021-06-29T16:11:20Z

.maintain/gitlab/check_companion_build.sh

+    boldprint "companion pr specified/detected: #${pr_companion}"
+    git fetch origin "refs/pull/${pr_companion}/head:pr/${pr_companion}"
+    git checkout "pr/${pr_companion}"
+    git merge origin/master


This should be merging whatever branch the PR is currently targetting instead of master (although PRs usually target master). Perhaps it doesn't need to be handled at the moment, but a FIXME note about this detail would be welcome.

joao-paulo-parity · 2021-06-29T16:11:36Z

.maintain/gitlab/check_companion_build.sh

+      git clone --depth 20 "https://github.com/$dep.git" "$dep"
+      git -C "$dep" fetch origin "refs/pull/${dep_companion}/head:pr/${dep_companion}"
+      git -C "$dep" checkout "pr/${dep_companion}"
+      git -C "$dep" merge origin/master


same as https://github.com/paritytech/substrate/pull/9010/files#r660768051

joao-paulo-parity · 2021-06-29T16:29:13Z

.maintain/gitlab/check_companion_build.sh

+  diener_commands["paritytech/substrate"]='--substrate'
+  diener_commands["paritytech/grandpa-bridge-gadget"]='--beefy'
+
+  for dep in "${DEPS[@]}"; do


If https://github.com/paritytech/substrate/pull/9010/files#r660727553 is taken care of, you could try to get rid of this $DEPS and just loop through all companion lines in the description; and if some repository is not supported (might be typo) then just fail the script.

stale · 2021-07-31T14:25:40Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2021-09-05T12:37:16Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

joao-paulo-parity · 2021-09-05T20:25:52Z

FYI a critical bug was found in #9677 (comment). I think it's grave enough to warrant implementing a fix in a separate PR and then rebase this on top of the fix PR.

joao-paulo-parity · 2021-09-09T01:03:33Z

FYI I've asked @s3krit to let me take over this effort. I would like to merge #9721 before moving to this since the changes over there are simpler and the implementation should be finished, right now it's simply waiting for the CI image to be updated (paritytech/scripts#309).

joao-paulo-parity · 2021-09-10T13:20:07Z

The implementation I have ended up being very different from this one, so I've opened a separate PR #9749 instead of pushing code over the work here. We can revisit this if the approach in #9749 does not work well.

stale · 2021-10-10T13:42:15Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

s3krit added A3-in_progress Pull request is in progress. No review needed at this stage. 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 Jun 3, 2021

github-actions bot added the A7-needspolkadotpr label Jun 3, 2021

s3krit changed the title ~~[WIP] [CI] Add cumulus companions~~ [WIP] [CI] Add cumulus + beefy companions Jun 3, 2021

s3krit changed the title ~~[WIP] [CI] Add cumulus + beefy companions~~ [CI] Add cumulus + beefy companions Jun 3, 2021

s3krit marked this pull request as ready for review June 3, 2021 16:38

s3krit requested a review from a team as a code owner June 3, 2021 16:38

github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jun 3, 2021

s3krit requested review from TriplEight and joao-paulo-parity June 3, 2021 16:38

s3krit added 6 commits June 3, 2021 18:43

add cumulus companions

bff60a7

remove references to any specific repo in check_companion_build.sh

deaeab7

naughty naughty very naughty (revert me)

21c3d80

add custom build string option

98030be

add beefy

4b68a08

Revert "naughty naughty very naughty (revert me)"

5f05ce8

This reverts commit 6fb44bb.

s3krit force-pushed the mp-cumulus-companions branch from 5e44cc5 to 5f05ce8 Compare June 3, 2021 16:44

out with the old

07747bc

TriplEight suggested changes Jun 4, 2021

View reviewed changes

.gitlab-ci.yml Outdated Show resolved Hide resolved

.gitlab-ci.yml Outdated Show resolved Hide resolved

.gitlab-ci.yml Outdated Show resolved Hide resolved

.gitlab-ci.yml Outdated Show resolved Hide resolved

.maintain/gitlab/check_companion_build.sh Outdated Show resolved Hide resolved

bkchr reviewed Jun 4, 2021

View reviewed changes

joao-paulo-parity suggested changes Jun 4, 2021

View reviewed changes

.maintain/gitlab/check_companion_build.sh Outdated Show resolved Hide resolved

.gitlab-ci.yml Show resolved Hide resolved

s3krit force-pushed the mp-cumulus-companions branch from 33c68bb to c3a1648 Compare June 7, 2021 12:32

add anchors

00a925b

s3krit force-pushed the mp-cumulus-companions branch from c3a1648 to 00a925b Compare June 7, 2021 13:00

move check-companion-build to test stage

3b90256

github-actions bot removed the A7-needspolkadotpr label Jun 7, 2021

update comments

46988ba

s3krit changed the title ~~[WIP][CI] Add cumulus + beefy companions~~ [CI] Add cumulus + beefy companions Jun 29, 2021

s3krit requested review from TriplEight, joao-paulo-parity and bkchr June 29, 2021 13:20

bkchr reviewed Jun 29, 2021

View reviewed changes

joao-paulo-parity suggested changes Jun 29, 2021

View reviewed changes

joao-paulo-parity reviewed Jun 29, 2021

View reviewed changes

Xanewok mentioned this pull request Jul 6, 2021

Attempting to fix publishing #9140

Merged

stale bot added A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. and removed A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. labels Jul 31, 2021

s3krit added 2 commits August 6, 2021 13:42

Merge branch 'master' into mp-cumulus-companions

c96f997

use jq for parsing PRs

a7ee4a3

joao-paulo-parity mentioned this pull request Aug 16, 2021

Enhance checks for companion pull requests paritytech/parity-processbot#304

Closed

3 tasks

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Sep 5, 2021

stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Sep 5, 2021

joao-paulo-parity mentioned this pull request Sep 7, 2021

Update check_polkadot_companion_build #9721

Closed

joao-paulo-parity mentioned this pull request Sep 10, 2021

Check all project dependents instead of just Polkadot #9749

Merged

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Oct 10, 2021

bkchr closed this Oct 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CI] Add cumulus + beefy companions #9010

[CI] Add cumulus + beefy companions #9010

s3krit commented Jun 3, 2021 •

edited

Loading

adoerr commented Jun 3, 2021

s3krit commented Jun 3, 2021

bkchr left a comment

bkchr Jun 29, 2021

TriplEight Jun 29, 2021

bkchr Jul 1, 2021

TriplEight Jul 1, 2021

TriplEight Aug 2, 2021 •

edited

Loading

TriplEight Aug 2, 2021

bkchr Jun 29, 2021

gilescope Aug 6, 2021

bkchr Jun 29, 2021

joao-paulo-parity Jun 29, 2021

joao-paulo-parity Jun 29, 2021

joao-paulo-parity Jun 29, 2021

joao-paulo-parity Jun 29, 2021

joao-paulo-parity Jun 29, 2021 •

edited

Loading

joao-paulo-parity Jun 29, 2021

joao-paulo-parity Jun 29, 2021

joao-paulo-parity Jun 29, 2021

joao-paulo-parity Jun 29, 2021

joao-paulo-parity Jun 29, 2021

stale bot commented Jul 31, 2021

stale bot commented Sep 5, 2021

joao-paulo-parity commented Sep 5, 2021

joao-paulo-parity commented Sep 9, 2021 •

edited

Loading

joao-paulo-parity commented Sep 10, 2021

stale bot commented Oct 10, 2021

	COMPANION_BUILD: "cargo check"
	COMPANION_BUILD: "cargo check --benches --all --tests"


		pr_body="$(sed -n -r 's/^[[:space:]]+"body": (".*")[^"]+$/\1/p' "${pr_data_file}")"

		echo "${pr_body}" \| sed -n -r \

[CI] Add cumulus + beefy companions #9010

[CI] Add cumulus + beefy companions #9010

Conversation

s3krit commented Jun 3, 2021 • edited Loading

adoerr commented Jun 3, 2021

s3krit commented Jun 3, 2021

bkchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TriplEight Aug 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joao-paulo-parity Jun 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Jul 31, 2021

stale bot commented Sep 5, 2021

joao-paulo-parity commented Sep 5, 2021

joao-paulo-parity commented Sep 9, 2021 • edited Loading

joao-paulo-parity commented Sep 10, 2021

stale bot commented Oct 10, 2021

s3krit commented Jun 3, 2021 •

edited

Loading

TriplEight Aug 2, 2021 •

edited

Loading

joao-paulo-parity Jun 29, 2021 •

edited

Loading

joao-paulo-parity commented Sep 9, 2021 •

edited

Loading