Companion failed to merge with "Failed to parse lockfile of polkadot" error

[TriplEight]

Originally posted in github.com/paritytech by @koute

This substrate PR here paritytech/substrate#11722 had a polkadot companion here paritytech/polkadot#5731. The substrate PR was all green (including the companion job), so I typed "bot merge", and I was expecting that the companion gets merged too, but it failed with a "Merge cancelled due to error. Error: Failed to parse lockfile of polkadot" error.

[joao-paulo-parity]

The full error from paritytech/polkadot#5731 (comment)

Merge cancelled due to error. Error: Failed to parse lockfile of polkadot: Error { kind: Parse, msg: "parse error: ambiguous dependency: windows-sys" }

It happened in

parity-processbot/src/companion.rs

Line 104 in 2feca9b

"Failed to parse lockfile of {}: {:?}",

and is related to cargo_lock. From a quick look at cargo_lock's source code (https://github.com/rustsec/rustsec/tree/main/cargo-lock) I couldn't understand why this error happens. It's strange since dependency resolution is cargo's job and I was expecting cargo_lock to only be parsing the file, but apparently it does more.

On another note I've recently found out that Cargo.lock is a TOML file, so we should consider dropping cargo_lock in favor of parsing the lockfiles through https://crates.io/crates/toml directly (it's what cargo_lock does anyways as per https://github.com/rustsec/rustsec/blob/360c89029b65b76c6bd46238489d1aa2e8d473a0/cargo-lock/src/lockfile.rs#L59, but apparently it's doing something extra).

Using cargo_lock is nicer since it handles different Cargo.lock versions, but our target repositories (Substrate, Cumulus and Polkadot) are all using v3 at the moment, so at least for now this wouldn't matter. Anyways I think a better long-term solution would be to find a way of disabling cargo_lock's extraneous behavior and limit it to parsing only.

[joao-paulo-parity]

Ok, I think I figured it out. The problem is that the lockfile had two windows-sys

• https://github.com/koute/polkadot/blob/8ff2627f00c88e5f0997b44f444b67d3a83d3444/Cargo.lock#L12687

• https://github.com/koute/polkadot/blob/8ff2627f00c88e5f0997b44f444b67d3a83d3444/Cargo.lock#L12700

It was incorrect when the bot tried to merge it since the link in https://github.com/koute/polkadot/blob/8ff2627f00c88e5f0997b44f444b67d3a83d3444/Cargo.lock#L6054 doesn't correctly specify which version to use. And the follow-up commit had to fix that in paritytech/polkadot@2abfa1c#diff-13ee4b2252c9e516a0547f2891aa2105c3ca71c6d7a1e682c69be97998dfc87eR6054.

Therefore the problem in that case wasn't with the bot, it was with the commit itself.

That problem should've been caught by the integration checks on CI, so I've created paritytech/pipeline-scripts#61 for this.

Closing since this isn't a processbot bug.

[joao-paulo-parity]

The problems mentioned here might have a solution in the future as per rustsec/rustsec#606, but it relies on cargo_lock 's upstream changes; I don't think it's worthwhile for us to do something about them in processbot directly.