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 1 commit
Commits
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
Prev Previous commit
Next Next commit
WIP
Forked at: d390eb5
Parent branch: origin/master
  • Loading branch information
cecton committed Jul 17, 2020
commit 6c7bc965daa4cfeb31acdb161a1782d11a61c4b9
6 changes: 6 additions & 0 deletions .editorconfig
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,9 @@ indent_style=space
indent_size=2
tab_width=8
end_of_line=lf

[*.sh]
indent_style=space
indent_size=2
tab_width=2
end_of_line=lf
2 changes: 2 additions & 0 deletions .gitlab-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,8 @@ test-linux-stable: &test
script:
- ./scripts/gitlab/test_linux_stable.sh
- sccache -s
after_script:
- cd substrate && git rev-parse --abbrev-ref HEAD

check-web-wasm: &test
stage: test
Expand Down
91 changes: 82 additions & 9 deletions scripts/gitlab/test_linux_stable.sh
Original file line number Diff line number Diff line change
@@ -1,14 +1,87 @@
#!/bin/sh
url="https://api.github.com/repos/paritytech/polkadot/pulls/${CI_COMMIT_REF_NAME}"
echo "[+] API URL: $url"

pr_title=$(curl -H "Authorization: token ${GITHUB_PR_TOKEN}" "$url" | grep -Po '"title": "\K[^"]+')
echo "[+] PR title: $pr_title"

if echo "$pr_title" | grep -qi '^companion'; then
echo "[!] PR is a companion PR. Build is already done in substrate"
exit 0
github_api_polkadot_pull_url="https://api.github.com/repos/paritytech/polkadot/pulls"
# use github api v3 in order to access the data without authentication
github_header="Authorization: token ${GITHUB_PR_TOKEN}"

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



POLKADOT_PATH=$(pwd)


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


prepare_substrate(){
pr_companion=$1
boldprint "companion pr specified/detected: #${pr_companion}"

# Clone the current Substrate master branch into ./substrate.
# 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/paritytech/substrate.git
cd substrate
SUBSTRATE_PATH=$(pwd)

git fetch origin refs/pull/${pr_companion}/head:pr/${pr_companion}
git checkout pr/${pr_companion}
git merge origin/master

cd "$POLKADOT_PATH"

# Merge master into our branch before building Polkadot to make sure we don't miss
# any commits that are required by Polkadot.
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 is prone to fail when the commit is not a final one in the PR. I.e. if I was renaming the function name and someone merged the use of an old function name in a new place, then it will fail in a not obvious manner.

But besides that, this makes a very good "Pre-merge test".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I don't like these git merge either. My reason is not because it's fallible but because it is testing something different than the actual code in the branch. But again, I just copied that from the original script, I did not intend to put that in question.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, should be resolved elsewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gabreal do you know if those merge are absolutely required? They can make a PR fails for no good reason: https://gitlab.parity.io/parity/polkadot/-/jobs/596036


# Make sure we override the crates in native and wasm build
# patching the git path as described in the link below did not test correctly
# https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html
mkdir .cargo
echo "paths = [ \"$SUBSTRATE_PATH\" ]" > .cargo/config
Copy link
Contributor

Choose a reason for hiding this comment

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

there are lots of warnings in the build log:

warning: path override for crate **** has altered the original list of
dependencies; the dependency on `sp-core` was either added or
modified to not match the previously resolved version

This is currently allowed but is known to produce buggy behavior with spurious
recompiles and changes to the crate graph. Path overrides unfortunately were
never intended to support this feature, so for now this message is just a
warning. In the future, however, this message will become a hard error.

To change the dependency graph via an override it's recommended to use the
`[replace]` feature of Cargo instead of the path override feature. This is
documented online at the url below for more information.

https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html

does it make sense to follow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to some of our colleagues, yes. @tomaka talked about it I think. But this is what we currently use right now in substrate so there are also a lot of warnings there but it works.

Right now it is not a big deal but I suppose at some point we will need to change the mechanism we use to override substrate.

I could provide a change that would use the [replace] but that change will be very intensive. I don't mind working on it but I'm currently booked with cumulus. I can provide my ideas and the code I already have for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mind either. It just produces that much of logs so GitLab log output says it's a bit too much. No big deal though.


mkdir -p target/debug/wbuild/.cargo
cp .cargo/config target/debug/wbuild/.cargo/config
}


# either it's a pull request then check for a companion otherwise use
# substrate:master
if expr match "${CI_COMMIT_REF_NAME}" '^[0-9]\+$' >/dev/null
then
boldprint "this is pull request no ${CI_COMMIT_REF_NAME}"

pr_data_file="$(mktemp)"
# get the last reference to a pr in substrate
curl -sSL -H "${github_header}" -o "${pr_data_file}" \
"${github_api_polkadot_pull_url}/${CI_COMMIT_REF_NAME}"

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

pr_companion="$(echo "${pr_body}" | sed -n -r \
-e 's;^.*substrate companion: paritytech/substrate#([0-9]+).*$;\1;p' \
-e 's;^.*substrate companion: https://github.com/paritytech/substrate/pull/([0-9]+).*$;\1;p' \
| tail -n 1)"

if [ "${pr_companion}" ]
then
prepare_git
prepare_substrate "$pr_companion"
else
boldprint "no companion branch found - building substrate:master"
fi
rm -f "${pr_data_file}"
else
echo "[+] PR is not a companion PR. Proceeding test"
time cargo test --all --release --verbose --locked --features runtime-benchmarks
boldprint "this is not a pull request - building substrate:master"
fi


# Test Polkadot pr or master branch with this Substrate commit.
time cargo test --all --release --verbose --locked --features runtime-benchmarks