Skip to content

Conversation

@joao-paulo-parity
Copy link
Contributor

https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/1497187#L142 failed because the Cumulus companion of paritytech/polkadot#5276 was not yet updated to reference the master of Substrate, which had been merged earlier in the chain of dependencies (paritytech/substrate#11183). This was solved in paritytech/cumulus#1161 by updating the Substrate reference manually: paritytech/cumulus@80bddd7.

This PR introduces an extra optional argument, extra_dependencies, which can be used to work around this situation. Its purpose is defining extra dependencies which are higher up in the Companion Build System's dependency chain, e.g. Substrate in the Substrate -> Polkadot -> Cumulus dependency chain for the Polkadot -> Cumulus check. This should fix situations such as https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/1497187#L142 since the crate would not be missing

We'd need to add it to Polkadot here: https://github.com/paritytech/polkadot/blob/c8c1933e01d0b9e91058bc64f153450857d568f4/.gitlab-ci.yml#L344

.check-dependent-project:          &check-dependent-project
  stage:                           stage2
  # this is an artificial job dependency, for pipeline optimization using GitLab's DAGs
  needs:
    - job:                         check-runtime
      artifacts:                   false
  <<:                              *docker-env
  <<:                              *test-pr-refs
  script:
    - git clone
        --depth=1
        "--branch=$PIPELINE_SCRIPTS_TAG"
        https://github.com/paritytech/pipeline-scripts
    - ./pipeline-scripts/check_dependent_project.sh
        paritytech
        polkadot
        --polkadot
        "$DEPENDENT_REPO"
        "$GITHUB_PR_TOKEN"
        "$CARGO_UPDATE_CRATES"
+        "$EXTRA_DEPENDENCIES"

check-dependent-cumulus:
  <<: *check-dependent-project
  variables:
    DEPENDENT_REPO: cumulus
    CARGO_UPDATE_CRATES: "sp-io polkadot-runtime-common"
+    EXTRA_DEPENDENCIES: substrate

I don't know if this deprecates the need for CARGO_UPDATE_CRATES in Polkadot. sp-io is part of Substrate which we'd be patching in entirely, so there's no need to update sp-io individually, but I don't understand why polkadot-runtime-common is needed there.

@joao-paulo-parity joao-paulo-parity requested review from a team and bkchr April 13, 2022 19:12
@bkchr
Copy link
Member

bkchr commented Apr 14, 2022

Wouldn't it be enough to move up update_crates $update_crates_on_default_branch? That should bring the same result?

@joao-paulo-parity
Copy link
Contributor Author

Wouldn't it be enough to move up update_crates $update_crates_on_default_branch?

update_crates had to be moved to after patching due to the corner case documented in https://github.com/paritytech/pipeline-scripts/pull/28/files#diff-df602127cd49392d5fb1da82c27351842395a47e788e0c6a77cf52d690ed1d2aR364. e.g. paritytech/substrate#11183 removed a crate but cargo update failed since it was not able to find the missing crate - it was looking at master instead of that PR because update_crates was done before patching; if it makes any sense to run it, it should be after patching.

I think we'll be able to remove update_crates in favor of this extra_dependencies because I infer they're trying to solve the same corner case. My only doubt about this is related to polkadot-runtime-common: I don't understand why it's necessary in the Polkadot -> Cumulus check since polkadot-runtime-common is a Polkadot package which should already be available in the Polkadot PR. Can't we get rid of this in favor of extra_dependencies: substrate, since it'll patch the whole master of Substrate into the PR anyways?

@bkchr
Copy link
Member

bkchr commented Apr 14, 2022

Sounds reasonable.

@joao-paulo-parity
Copy link
Contributor Author

Per paritytech/polkadot#5328 (comment) this should be ready. Could you take a look @alvicsam ?

First we merge paritytech/polkadot#5328 to avoid deprecating the current update_crates feature, then we merge this PR after.

@joao-paulo-parity joao-paulo-parity merged commit 00e758e into paritytech:master Apr 21, 2022
@joao-paulo-parity joao-paulo-parity deleted the fix branch April 21, 2022 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants