Companion build system and how to improve it

[joao-paulo-parity]

How it works

Currently the most in-depth explanation is available as a presentation:

• Slides: https://docs.google.com/presentation/d/12ksmejR_UXC1tIHD2f4pQQZ1uw5NK3n8enmwkTCPOpw
• Video: https://drive.google.com/file/d/1E4Fd3aO2QRJuoUBI4j0Zp4027yGeHeer/view?usp=sharing

Parity has repositories which are dependencies of others; for instance, Substrate is a dependency of Polkadot. To avoid breakage between those projects, the companion build system was established. It works as follows:

1. Create a Substrate PR which has breaking changes for Polkadot
2. Create a companion PR on Polkadot in order to keep up with the Substrate PR and avoid breakage
   2.1 This PR's CI is expected to fail at first because the lockfile has not yet been updated to refer to the Substrate PR
3. Reference the companion in Substrate PR's description. Since a companion is specified, the Substrate -> Polkadot CI integration, which would normally point to Polkadot master, will now point to the companion. This integration is required to pass before the Substrate PR is merged.
4. When the Substrate PR is merged to master, processbot will update the Polkadot PR's lockfile to reference Substrate master, which now includes the Substrate PR's code, and so the Polkadot PR is expected to pass now.

Problems

Substrate -> Polkadot CI integration check is incomplete

The Substrate -> Polkadot CI integration check right now only does cargo check. It does not test the whole CI of Polkadot and so we're not very confident about the merge of Polkadot's MR working ahead-of-time, since some other required check might fail on the Polkadot PR.

Solutions:

Refactor the Substrate -> Polkadot CI integration to actually run the whole Polkadot CI ahead-of-time (https://github.com/paritytech/ci_cd/issues/234)

or

Provided predictable references are implemented, the bot would check if the Polkadot PR is using the tag. When bot merge is triggered for the Substrate PR, the bot would update the tag, retrigger Polkadot CI and both sides to pass on CI before going through with the merge.

Risk of desynchronization for merge failures

Since the Substrate PR introduces breaking changes for Polkadot, should the Polkadot companion failed to be merged, because the Substrate PR is merged first (and we hope that the Polkadot one will be merged soon after), all future Substrate PRs could be blocked until the companion is merged because Polkadot master does not yet have the code to keep up with the Substrate PR's changes.

Solution: Implement a merge queue in processbot in order to prevent PRs without companions from being merged until PRs which have a companion are merged.

Pushing lockfile updates from the bot is unsafe

processbot is expected to push the commit for updating the lockfile on the companion, but some malicious code could be pushed instead if an attacker were to impersonate the bot and/or take control of it. Having the bot push commits is specially problematic because such commits bypass the peer-review process.

Solution: Provided predictable references are implemented, the bot would not have to push the lockfile update because the Polkadot PR already has the right reference set up.

Redesign

Those proposals focus on overcoming some of the problems mentioned in the previous sections.

Alternative 1: Predictable References

#383

Alternative 2: Monorepo

The companion build system as a whole could be deprecated if the dependencies and dependents lived in a single repository.

Alternative 3: Master checks with follow-up issues

This alternative entails removing the Companion Build System in favor of tickets to manage the update of downstream projects ad-hoc. Notably it is a departure from the companion build system's preventive approach where breakages are caught before merge because for this alternative the breakages would only be caught after merge. It would work like the following:

1. Run integration checks between Substrate <-> Polkadot on Substrate's master CI
2. If the check fails, create a ticket "Fix breakages on Polkadot" and assign the commit authors of breaking changes to that ticket

[joao-paulo-parity]

Note that if we manage to redesign the companion build system such that the Substrate PR does not have to be merged first (which is a problem currently, as demonstrated by the "Risk of desynchronization for merge failures" point), it would be possible to re-implement the status checking on both sides which was removed because the companion is failing at first.

[mutantcornholio]

I guess, first we need to answer "The Monorepo Question" here.
It's "How independent should modules be? Should failure in one module down the dependencies stream block the change? How thorough do we need to be here?"

If we decide that binding all the modules together and blocking the merge until all checks down the stream pass, monorepo is the best answer.

If we want to be able to ship a version of a library without forcing an upgrade on a companion (are "companion" and "dependent module" terms interchangeable?), this scenario must be considered as first-class in the integration pipeline.

[joao-paulo-parity]

It's "How independent should modules be? Should failure in one module down the dependencies stream block the change? How thorough do we need to be here?"

Q: How independent should modules be?

A: They should be separate projects but breakages in the dependencies should not affect the dependents, e.g. an breaking API change in Substrate should require some adjustment on Polkadot.

Q: Should failure in one module down the dependencies stream dependents block the change?

A: Yes. All the PRs involved should be in a good state.

Q: How thorough do we need to be here?

Ideally the CI of each project would be run before any merge happens (https://github.com/paritytech/ci_cd/issues/234), however that's not how the current system works, as described in https://docs.google.com/presentation/d/12ksmejR_UXC1tIHD2f4pQQZ1uw5NK3n8enmwkTCPOpw/edit#slide=id.g1092a3d03f1_0_139.

Monorepo is the best answer

Since this post was written I have received negative feedback from other people about the monorepo idea

• Affects the projects' brands (as in business brand): although Polkadot depends on Substrate, it is still a separate project and brand. Putting both in a single repository somehow affects the brand but of course I'm not qualified to understand this fully.

• Allegedly makes CI more complicated (?)

• Messes with the commit histories

I think the branding issues from the first item are the most relevant. Between the currently available proposals I think #383 is the least disruptive, although I agree that the monorepo idea looks to be easier to use and maintain overall.

If we want to be able to ship a version of a library without forcing an upgrade on a companion

No request has been received about this, so we can only assume Parity doesn't find value in that.

are "companion" and "dependent module" terms interchangeable?

"Companion" is how "dependent" is known historically. I don't know who decided to use the word "companion" but I do know that the word has nothing to do with the "dependencies linked to dependents" ideas in the system, so I started using the word "dependent" which I think is more obvious.