Companion: move check-polkadot-companion-build to polkadot #1426

cecton · 2020-07-17T10:03:55Z

substrate companion: paritytech/substrate#6706

Forked at: d390eb5 Parent branch: origin/master

Parent branch: origin/master Forked at: d390eb5

Forked at: d390eb5 Parent branch: origin/master

cecton · 2020-07-17T15:48:32Z

@gabreal I got this (maybe) excellent idea to reduce the build time and improve the current workflow.

We don't need to run the polkadot build step on substrate, we can only check the status of the polkadot companion PR and let the polkadot build does its job.

To do that I have inverted the way things are done: instead of pulling polkadot and testing in the substrate build, I pull substrate and I do the normal check in the polkadot build. The substrate companion PR must be in description.

Someone who works on a substrate PR would still need to provide a companion PR but when the polkadot branch is failing they only need to make the polkadot pr work pass. (They will also need to restart the status check build in substrate but that step is fast.)

VS now:

When someone works on a substrate PR, they need to make the polkadot build pass in polkadot and in substrate.

I believe this is also more complete because I can share the code with the other steps in polkadot (check-web-wasm, etc...).

If you agree with this change I will make a common bash "functions" file and tweek the other build steps of polkadot.

TriplEight

Thanks for the PR, the idea sounds really good!

I didn't get what paritytech/substrate#6654 has to do with this change.

Does this PR mean that a companion check should move from Substrate CI to here?

And regarding the test_linux_stable CI job itself: it used to test polkadot, now it tests substrate, where do we test polkadot then?

TriplEight · 2020-07-18T07:43:02Z

scripts/gitlab/test_linux_stable.sh

+  # 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


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?

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.

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.

scripts/gitlab/test_linux_stable.sh

TriplEight · 2020-07-18T07:51:48Z

scripts/gitlab/test_linux_stable.sh

+
+  # 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


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

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.

ok, should be resolved elsewhere

@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

scripts/gitlab/test_linux_stable.sh

cecton · 2020-07-18T08:41:57Z

I didn't get what paritytech/substrate#6654 has to do with this change.

Nothing at all! I needed a substrate PR for testing and I didn't want to create one explicitly for that. Sorry, I've done a mess with github 😅

Does this PR mean that a companion check should move from Substrate CI to here?

Yes that's the whole point. We could remove the companion-check-build and keep only the companion-check-status (in substrate)

And regarding the test_linux_stable CI job itself: it used to test polkadot, now it tests substrate, where do we test polkadot then?

No no it's testing polkadot XD I copied the code of companion-check-build and I inverted the repositories so it's cloning substrate but it's actually testing polkadot.

The path is changing back to polkadot here: https://github.com/paritytech/polkadot/pull/1426/files#diff-bd5a097042de94b88468bfb88d6257f4R38 (line 38)

TriplEight

I think it's good to go. Where are the corresponding changes to substrate CI?

scripts/gitlab/test_linux_stable.sh

Co-authored-by: Denis Pisarev <denis.pisarev@parity.io>

cecton · 2020-07-20T12:45:20Z

@TriplEight I haven't done a PR in substrate to remove the polkadot build yet. I'm off today and tomorrow, I will finish it on Thursday.

cecton · 2020-07-22T05:57:59Z

Ok I think I need to work on this fast. This change would also solve issues like this one: #1080

The substrate branch has been merged but the polkadot companion is stalling. Meaning that the substrate branch is doing okay but potentially the polkadot master is not working with substrate master anymore.

@gabreal can I also have your feedback on my PR here? My intention is:

remove check-polkadot-companion-build in substrate
make check-polkadot-companion-status in substrate check that the build passes
make all the steps in polkadot use the substrate companion PR if any

Improvements with that:

one less build to do (less CI runtime)
polkadot and substrate will be more in sync as substrate won't be mergeable without the companion passing
nobody will need to change the branch in polkadot, it will use the right one by default (master if not a companion pr)
(no need to do the cargo update -p sp-io anymore)

Parent branch: origin/master Forked at: d390eb5

Forked at: d390eb5 Parent branch: origin/master

…ason...)

cecton · 2020-07-22T07:21:40Z

scripts/gitlab/check_runtime.sh

+    failed_runtime_checks+=($RUNTIME)
+  fi
+done

-		source file directories:
-		- runtime
+if [ ${#failed_runtime_checks} -gt 0 ]; then
+  boldcat <<EOT
+wasm source files changed or the spec version in the substrate reference in
+the Cargo.lock but not the spec/impl version. If changes made do not alter
+logic, just bump 'impl_version'. If they do change logic, bump
+'spec_version'.

-		versions file: ${VERSIONS_FILE}
+source file directories:
+- runtime

-		EOT
+version files: ${failed_runtime_checks[@]}
+EOT

-		exit 1
-	fi
-done
+  exit 1
+fi


I added this change to this PR. It's not really related, I just wanted to fix it.

Instead of failing at the first runtime check, it now collects all the runtime check that fails and print them in a single build.

I also converted the file from tabs to spaces because all other bash scripts in this repo are using spaces.

Parent branch: origin/master Forked at: d390eb5

Forked at: d390eb5 Parent branch: origin/master

s3krit

Thanks for fixing the indentation in check_runtime.sh, I'll blame my editor for that...
Also big thanks for taking the time to split these jobs off into their own shell scripts. And for making use of lib.sh!

All but one of my suggestions were just to get rid of Shellcheck nags - I try to keep the scripts I work on compliant with that where possible - even in cases where (for example with it complaining about shell globbing), the input we provide will never actually cause a problem.

I'll just leave this as a comment for now, due to ongoing discussion on Matrix.

scripts/gitlab/check_runtime.sh

s3krit · 2020-07-22T12:01:41Z

scripts/gitlab/lib.sh

@@ -96,3 +96,79 @@ curl -XPOST -d "$1" "https://matrix.parity.io/_matrix/client/r0/rooms/$2/send/m.
 # Pretty-printing functions
 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" ; }
+
+prepare_git() {


BIG fan of more people starting to use lib.sh 🎉

scripts/gitlab/lib.sh

Co-authored-by: s3krit <pugh@s3kr.it>

cecton · 2020-07-22T14:15:26Z

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.

I still want to solve the problem. I believe the first solution I implemented has minimal cost and would actually have an impact: 36802ad

But I'm still closing this PR to keep track of the proposal and all the code and ideas.

cc @gavofyork @bkchr

cecton added 2 commits July 17, 2020 12:02

Initial commit

e5e4e79

Forked at: d390eb5 Parent branch: origin/master

Ignore build for companion PR

36802ad

cecton self-assigned this Jul 17, 2020

github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jul 17, 2020

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 17, 2020

cecton added 5 commits July 17, 2020 12:14

Fix redirection syntax not supported

3c3318d

Merge commit 28c26d6 (no conflict)

d9584b5

Parent branch: origin/master Forked at: d390eb5

Merge commit 7071a02 (no conflict)

eda599c

Parent branch: origin/master Forked at: d390eb5

Replace jq by grep

905551f

WIP

6c7bc96

Forked at: d390eb5 Parent branch: origin/master

cecton requested review from gabreal, s3krit and TriplEight July 17, 2020 15:48

TriplEight reviewed Jul 18, 2020

View reviewed changes

TriplEight suggested changes Jul 20, 2020

View reviewed changes

scripts/gitlab/test_linux_stable.sh Outdated Show resolved Hide resolved

scripts/gitlab/test_linux_stable.sh Outdated Show resolved Hide resolved

Apply suggestions from code review

38cf177

Co-authored-by: Denis Pisarev <denis.pisarev@parity.io>

cecton mentioned this pull request Jul 22, 2020

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

Closed

cecton changed the title ~~Companion test~~ Companion: move check-polkadot-companion-build to polkadot Jul 22, 2020

paritytech-ci added the D2-breaksapi label Jul 22, 2020

cecton force-pushed the cecton-ignore-test-for-companion branch from 17e10fa to 38cf177 Compare July 22, 2020 06:32

cecton added 5 commits July 22, 2020 08:39

Merge commit 1b7882a (no conflict)

0d00e35

Parent branch: origin/master Forked at: d390eb5

WIP

d062be6

Forked at: d390eb5 Parent branch: origin/master

Convert tabs to space to match .editorconfig

6f18a20

Collect all failing runtime checks and print them together

b687b14

Removed here-document leading space (works only with tabs for some re…

32d831f

…ason...)

cecton added 2 commits July 22, 2020 09:08

more concise

3a8f8b8

Increase spec_version

0003994

cecton commented Jul 22, 2020

View reviewed changes

Merge commit 4da8b5e (conflicts)

9ddeb6f

Parent branch: origin/master Forked at: d390eb5

cecton marked this pull request as ready for review July 22, 2020 07:53

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 Jul 22, 2020

cecton added 4 commits July 22, 2020 10:02

WIP

f0037fb

Forked at: d390eb5 Parent branch: origin/master

WIP

69ddd54

Forked at: d390eb5 Parent branch: origin/master

WIP

0080e32

Forked at: d390eb5 Parent branch: origin/master

Fix wrong shebang

6bac55c

cecton requested a review from TriplEight July 22, 2020 11:31

s3krit reviewed Jul 22, 2020

View reviewed changes

cecton and others added 2 commits July 22, 2020 14:41

Apply suggestions from code review

1c9dbe1

Co-authored-by: s3krit <pugh@s3kr.it>

more shellcheck

063fbe3

cecton closed this Jul 22, 2020

cecton deleted the cecton-ignore-test-for-companion branch July 22, 2020 14:15

cecton mentioned this pull request Jul 29, 2020

Companion: ignore checks for companion PR #1455

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Companion: move check-polkadot-companion-build to polkadot #1426

Companion: move check-polkadot-companion-build to polkadot #1426

cecton commented Jul 17, 2020 •

edited

Loading

cecton commented Jul 17, 2020

TriplEight left a comment

TriplEight Jul 18, 2020

cecton Jul 18, 2020

TriplEight Jul 20, 2020

TriplEight Jul 18, 2020

cecton Jul 18, 2020

TriplEight Jul 20, 2020

cecton Jul 22, 2020

cecton commented Jul 18, 2020

TriplEight left a comment

cecton commented Jul 20, 2020

cecton commented Jul 22, 2020 •

edited

Loading

cecton Jul 22, 2020

s3krit left a comment

s3krit Jul 22, 2020

cecton commented Jul 22, 2020 •

edited

Loading

Companion: move check-polkadot-companion-build to polkadot #1426

Companion: move check-polkadot-companion-build to polkadot #1426

Conversation

cecton commented Jul 17, 2020 • edited Loading

cecton commented Jul 17, 2020

TriplEight 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecton commented Jul 18, 2020

TriplEight left a comment

Choose a reason for hiding this comment

cecton commented Jul 20, 2020

cecton commented Jul 22, 2020 • edited Loading

Choose a reason for hiding this comment

s3krit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecton commented Jul 22, 2020 • edited Loading

cecton commented Jul 17, 2020 •

edited

Loading

cecton commented Jul 22, 2020 •

edited

Loading

cecton commented Jul 22, 2020 •

edited

Loading