multiple companions

[gilescope]

https://github.com/paritytech/pipeline-scripts/blob/9f2e909fad823eae317f2789aa7b7e4adc8df9ab/check_dependent_project.sh

seems to assume there's only one companion PR but there could be multiple. (polkadot and cumulus)

Also if there are then polkadot is going to want to bu pulled into the cumulus companion build.

UPDATE: multiple companions are supported and refer to each other. It is only the edge case of when there is no cumulus PR and yet there is a substrate PR that is not handled.

[TriplEight]

AFAIU this script should be added to cumulus CI, too

[joao-paulo-parity]

seems to assume there's only one companion PR but there could be multiple. (polkadot and cumulus)

As demonstrated by https://gitlab.parity.io/parity/substrate/-/jobs/1182722#L141, all companions mentioned in the description are considered

Also if there are then polkadot is going to want to bu pulled into the cumulus companion build.

It is true that we do not patch transitive dependencies. Given how the merge order works (paritytech/parity-processbot#327) in that case you should describe the Cumulus companion in the Polkadot PR so that it gets merged after the Polkadot one.

As a workaround for the Substrate check we can have some logic which skips the Cumulus check on Substrate if it is already specified in one of the companions (not even always possible because you can have a Cumulus PR which needs both the Substrate lockfile update and the Polkadot lockfile update) but it won't work flawlessly since things are not merged at the same time (the problem described in "Risk of desynchronization for merge failures") and so you could have the following scenario:

1. You create a Cumulus companion for a Polkadot PR
2. The Polkadot PR is specified as a companion on a Substrate PR
3. The Cumulus check is skipped on Substrate because it is specified in a Polkadot companoin
4. The Substrate PR is merged, but the Polkadot companion fails to be merged for some reason
5. Now all Cumulus checks in Substrate are failing because the chain of Substrate PR -> Polkadot PR -> Cumulus PR has not been merged at the same time

This scenario could be fixed by "predictable references" because we'd be able to merge the whole chain of PRs at the same time, but it implies a redesign of the companion build system. The monorepo proposal is radical but it does not sound bad either because we'd be able to get rid of the companion build system, which would be great.

[bkchr]

This issue exists for multiple companions or also for when there is only a Polkadot companion. The problem is that when we check Cumulus, we don't use the Polkadot companion to replace the Polkadot crates, we only do this for the Substrate crates. The solution for this is rather simple, we should also replace the Polkadot crates using the Polkadot companion branch when checking cumulus. So for the Cumulus companion check of Substrate:

1. Check out Cumulus, either master or the referenced companion branch.
2. Check out Polkadot companion, if there is one specified.
3. Run cargo update -p sp-io -p polkadot-runtime-common
4. Run diener patch -p ${PATH_TO_POLKADOT_CHECKOUT} inside the cumulus checkout.
5. Run diener patch -p ${PATH_TO_SUBSTRATE_CHECKOUT} inside the cumulus checkout.
6. Run the check command.

[TriplEight]

@bkchr thanks for the write-up.
Tell me please, what are the chances of having this sort of custom logic for the other new companions? Not a big deal, just a decision point either these actions should be executed within the script or a concrete CI job.
Won't 4 conflict with 5?

[bkchr]

Tell me please, what are the chances of having this sort of custom logic for the other new companions?

What other companions?

Won't 4 conflict with 5?

No, both are doing things.