Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
bff60a7
add cumulus companions
s3krit Jun 3, 2021
deaeab7
remove references to any specific repo in check_companion_build.sh
s3krit Jun 3, 2021
21c3d80
naughty naughty very naughty (revert me)
s3krit Jun 3, 2021
98030be
add custom build string option
s3krit Jun 3, 2021
4b68a08
add beefy
s3krit Jun 3, 2021
5f05ce8
Revert "naughty naughty very naughty (revert me)"
s3krit Jun 3, 2021
07747bc
out with the old
s3krit Jun 3, 2021
00a925b
add anchors
s3krit Jun 7, 2021
3b90256
move check-companion-build to test stage
s3krit Jun 7, 2021
cd97118
ugh...
s3krit Jun 7, 2021
98f83ce
fix
s3krit Jun 7, 2021
a9427dd
Merge branch 'master' of https://github.com/paritytech/substrate into…
s3krit Jun 10, 2021
359820c
dynamically patch crates as needed
s3krit Jun 10, 2021
3002789
Merge branch 'master' into mp-cumulus-companions
s3krit Jun 28, 2021
8c5363d
Merge remote-tracking branch 'origin/master' into mp-cumulus-companions
s3krit Jun 28, 2021
dc8d301
fix reviews
s3krit Jun 28, 2021
c90db6c
first attempt at deeper dependencies
s3krit Jun 28, 2021
7df4a60
include lib.sh
s3krit Jun 28, 2021
1b8c9d8
Merge remote-tracking branch 'origin/master' into mp-cumulus-companions
s3krit Jun 28, 2021
8181635
patch things correctly
s3krit Jun 28, 2021
c476a9d
improve comments, test something neat
s3krit Jun 28, 2021
f5f920d
Apply suggestions from code review
s3krit Jun 29, 2021
46988ba
update comments
s3krit Jun 29, 2021
c96f997
Merge branch 'master' into mp-cumulus-companions
s3krit Aug 6, 2021
a7ee4a3
use jq for parsing PRs
s3krit Aug 6, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 32 additions & 10 deletions .gitlab-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ node-bench-regression-guard:
CI_IMAGE: "paritytech/node-bench-regression-guard:latest"
before_script: [""]
script:
- 'node-bench-regression-guard --reference artifacts/benches/master-*
- 'node-bench-regression-guard --reference artifacts/benches/master-*
--compare-with artifacts/benches/$CI_COMMIT_REF_NAME-$CI_COMMIT_SHORT_SHA'

cargo-check-subkey:
Expand Down Expand Up @@ -428,19 +428,41 @@ check-polkadot-companion-status:
script:
- ./.maintain/gitlab/check_polkadot_companion_status.sh

check-polkadot-companion-build:
stage: build
.check-companion-build: &check-companion-build
stage: test
<<: *docker-env
<<: *test-refs-no-trigger
needs:
- job: test-linux-stable-int
artifacts: false
script:
- ./.maintain/gitlab/check_polkadot_companion_build.sh
- ./.maintain/gitlab/check_companion_build.sh "$COMPANION_ORG" "$COMPANION_REPO" "$COMPANION_BUILD" "$COMPANION_DEPENDENCY"
after_script:
- cd polkadot && git rev-parse --abbrev-ref HEAD
- cd "$COMPANION_REPO" && git rev-parse --abbrev-ref HEAD
allow_failure: true

check-polkadot-companion-build:
<<: *check-companion-build
variables:
COMPANION_ORG: paritytech
COMPANION_REPO: polkadot
COMPANION_DEPENDENCY: paritytech/grandpa-bridge-gadget
allow_failure: true

check-cumulus-companion-build:
<<: *check-companion-build
variables:
COMPANION_ORG: paritytech
COMPANION_REPO: cumulus
COMPANION_BUILD: "cargo check"
Copy link
Member

Choose a reason for hiding this comment

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

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..

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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 :(

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@TriplEight TriplEight Aug 2, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

COMPANION_DEPENDENCY: paritytech/polkadot
allow_failure: true

check-beefy-companion-build:
<<: *check-companion-build
variables:
COMPANION_ORG: paritytech
COMPANION_REPO: grandpa-bridge-gadget
COMPANION_BUILD: "cargo check"
Copy link
Member

Choose a reason for hiding this comment

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

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

allow_failure: true

test-browser-node:
stage: build
<<: *docker-env
Expand Down Expand Up @@ -568,7 +590,7 @@ build-rust-doc:
- buildah push --format=v2s2 "$IMAGE_NAME:latest"
after_script:
- buildah logout "$IMAGE_NAME"
# pass artifacts to the trigger-simnet job
# pass artifacts to the trigger-simnet job
- echo "IMAGE_NAME=${IMAGE_NAME}" | tee -a ./artifacts/$PRODUCT/build.env
- IMAGE_TAG="$(cat ./artifacts/$PRODUCT/VERSION)"
- echo "IMAGE_TAG=${IMAGE_TAG}" | tee -a ./artifacts/$PRODUCT/build.env
Expand Down Expand Up @@ -702,7 +724,7 @@ trigger-simnet:
- if: $CI_COMMIT_REF_NAME == "master"
needs:
- job: publish-docker-substrate
# `build.env` brings here `$IMAGE_NAME` and `$IMAGE_TAG` (`$VERSION` here,
# `build.env` brings here `$IMAGE_NAME` and `$IMAGE_TAG` (`$VERSION` here,
# i.e. `2643-0.8.29-5f689e0a-6b24dc54`).
variables:
TRGR_PROJECT: ${CI_PROJECT_NAME}
Expand Down
44 changes: 35 additions & 9 deletions .maintain/common/lib.sh
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,18 @@

api_base="https://api.github.com/repos"


set_github_token(){
# These will exist if the function is called in Gitlab.
# If the function's called in Github, we should have GITHUB_ACCESS_TOKEN set
# already.
if [ -n "$GITHUB_RELEASE_TOKEN" ]; then
GITHUB_TOKEN="$GITHUB_RELEASE_TOKEN"
elif [ -n "$GITHUB_PR_TOKEN" ]; then
GITHUB_TOKEN="$GITHUB_PR_TOKEN"
fi
}

# Function to take 2 git tags/commits and get any lines from commit messages
# that contain something that looks like a PR reference: e.g., (#1234)
sanitised_git_logs(){
Expand Down Expand Up @@ -66,15 +78,7 @@ has_label(){
repo="$1"
pr_id="$2"
label="$3"

# These will exist if the function is called in Gitlab.
# If the function's called in Github, we should have GITHUB_ACCESS_TOKEN set
# already.
if [ -n "$GITHUB_RELEASE_TOKEN" ]; then
GITHUB_TOKEN="$GITHUB_RELEASE_TOKEN"
elif [ -n "$GITHUB_PR_TOKEN" ]; then
GITHUB_TOKEN="$GITHUB_PR_TOKEN"
fi
set_github_token

out=$(curl -H "Authorization: token $GITHUB_TOKEN" -s "$api_base/$repo/pulls/$pr_id")
[ -n "$(echo "$out" | tr -d '\r\n' | jq ".labels | .[] | select(.name==\"$label\")")" ]
Expand Down Expand Up @@ -115,3 +119,25 @@ has_runtime_changes() {
return 1
fi
}

# Given an origin repo & PR,and a repository, will check if the given PR has a
# companion PR in the target repo
get_companion() {
origin_repo="$1"
pr_num="$2"
companion_repo="$3"
pr_data_file="$(mktemp)"
set_github_token
github_header="Authorization: token ${GITHUB_TOKEN}"
# get the last reference to a pr in the target repo
curl -sSL -H "${github_header}" -o "${pr_data_file}" \
"${api_base}/$origin_repo/pulls/$pr_num"

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

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

Choose a reason for hiding this comment

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

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.

-e "s;^.*[Cc]ompanion.*$companion_repo#([0-9]+).*$;\1;p" \
-e "s;^.*[Cc]ompanion.*https://github.com/$companion_repo/pull/([0-9]+).*$;\1;p" \
| tail -n 1
rm -f "${pr_data_file}"
}
123 changes: 123 additions & 0 deletions .maintain/gitlab/check_companion_build.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
#!/usr/bin/env bash
#
# check if a pr is compatible with companion pr or master if not available
#
# to override one that was just mentioned mark companion pr in the body of the
# pr like
#
# $REPO companion: $ORGANISATION/$REPO#567
#
# $ORGANISATION and $REPO are set using $1 and $2. You can also specify a custom
# build command with $3
# Every argument after $3 is for specifying *additional* dependencies this
# project has that depend on substrate, which might also have companion PRs.

# Example: Cumulus relies on both substrate and polkadot. If this substrate PR
# requires a companion build on polkadot, when we are testing that cumulus builds
# with this commit of substrate, we *also* need to clone & patch polkadot, and tell
# cumulus to use this cloned+patched version, else the build is guaranteed to fail
# (since it doesn't have the changes to polkadot that were required in the polkadot
# companion PR)

# So invoke this script like (arguments in [] indicate optional arguments)
# ./check_companion_build.sh paritytech cumulus ['cargo test --release'] [paritytech/polkadot]

set -e

# Include the common functions library
#shellcheck source=../common/lib.sh
. "$(dirname "${0}")/../common/lib.sh"

ORGANISATION=$1
Copy link
Contributor

Choose a reason for hiding this comment

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

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

REPO=$2
BUILDSTRING=${3:-cargo test --workspace --release}
DEPS=("${@:4}")
SUBSTRATE_DIR="$(pwd)"

boldprint () { printf "|\n| \033[1m%s\033[0m\n|\n" "${@}"; }
boldcat () { printf "|\n"; while read -r l; do printf "| \033[1m%s\033[0m\n" "${l}"; done; printf "|\n" ; }

boldcat <<-EOT
check_${REPO}_companion_build
==============================
this job checks if there is a string in the description of the pr like
$REPO companion: $ORGANISATION/$REPO#567
it will then run cargo check from this ${REPO}'s branch with substrate code
from this pull request. otherwise, it will uses master instead
EOT

# Set the user name and email to make merging work
git config --global user.name 'CI system'
git config --global user.email '<>'

# Merge master into our branch before building to make sure we don't miss
# any commits that are required.
git fetch --depth 100 origin
git merge origin/master
Comment on lines +63 to +64
Copy link
Contributor

Choose a reason for hiding this comment

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

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)


# Clone the current master branch into a local directory
# NOTE: we need to pull enough commits to be able to find a common
# ancestor for successfully performing merges below.
git clone --depth 20 "https://github.com/${ORGANISATION}/${REPO}.git"

cd "$REPO"

# either it's a pull request then check for a companion otherwise use
# master
# shellcheck disable=SC2003
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")"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

if [ "$pr_companion" ]
then
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

else
boldprint "no companion branch found - building ${REPO}:master"
fi

# If this repo has any additional dependencies, we should check whether they
# are mentioned as companions as well. If they are, we patch this repo to
# use that companion build as well. See example at top of this script
# Note: Will only work with repos supported by diener
declare -A diener_commands
diener_commands=()
diener_commands["paritytech/polkadot"]='--polkadot'
diener_commands["paritytech/substrate"]='--substrate'
diener_commands["paritytech/grandpa-bridge-gadget"]='--beefy'

for dep in "${DEPS[@]}"; do
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

dep_companion="$(get_companion "paritytech/substrate" "$CI_COMMIT_REF_NAME" "$dep")"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

if [ "$dep_companion" ]; then
echo "Companion PR found for $dep, need to patch $REPO to use that"
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
Copy link
Contributor

Choose a reason for hiding this comment

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

# Tell this dependency to use the version of substrate in this PR
diener patch --crates-to-patch "$SUBSTRATE_DIR" --substrate --path "$dep/Cargo.toml"
Copy link
Contributor

@joao-paulo-parity joao-paulo-parity Jun 29, 2021

Choose a reason for hiding this comment

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

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

# then we tell this repository to point at our locally cloned dependency
diener patch --crates-to-patch "$dep" "${diener_commands[$dep]}" --path "Cargo.toml"
fi

done

else
boldprint "this is not a pull request - building ${REPO}:master"
fi

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

Choose a reason for hiding this comment

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

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.


eval "$BUILDSTRING"
92 changes: 0 additions & 92 deletions .maintain/gitlab/check_polkadot_companion_build.sh

This file was deleted.