chore(ci): Add --locked flag to cargo install step

[obi1kenobi]

Description

This PR tweaks the installation of cargo-semver-checks to make it use the --locked flag.

Installing binaries with their locked dependency versions makes it less likely that you might run into issues caused by bugs in dependency libraries, since your installed dependency versions match the versions used in the binary's own test environment.

This is a recommendation that applies to most Rust binary tools. For example, here's cargo-nextest recommending the same: https://nexte.st/book/installing-from-source.html#installing-from-cratesio

Notes

I don't think this should have any immediate impact on the project one way or another. It's primarily about protecting from possible future issues that could manifest as "cargo-semver-checks is misbehaving" and may be tedious to debug, hence best avoided altogether.

Links to any relevant issues

Open Questions

Change checklist

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• A changelog entry has been made in the appropriate crates

[thomaseizinger]

Thanks for sending this patch!

We don't have a Cargo.lock file in this repository but I guess we would restore one from cache but in that case, we would be restoring the binary too.

Is this flag going to make a difference in that case?

[obi1kenobi]

We don't have a Cargo.lock file in this repository but I guess we would restore one from cache but in that case, we would be restoring the binary too.

Is this flag going to make a difference in that case?

I believe this is asking cargo to use the Cargo.lock uploaded to crates.io as part of the cargo publish of cargo-semver-checks itself. At least that's my understanding based on:

• this conversation on Twitter with the author of cargo-nextest: https://twitter.com/PredragGruevski/status/1590850093976289281
• fixing a lockfile conflict in the cargo-semver-checks GitHub Action when it was being used in a project that had conflicting requirements relative to the cargo install in the action itself (trustfall_core is an 0.0.x crate so every release is considered semver-major).

So I believe it's still desirable, it doesn't matter that this repo doesn't have a Cargo.lock file, and it doesn't have to be cached at all.

[thomaseizinger]

Awesome, thanks for pointing this out! TIL :)

[obi1kenobi]

Should I be worried that the crates.io-published libp2p-mdns and libp2p-quic seem to have failed to compile per the log? That was a rustc compilation error in both, not a cargo-semver-checks issue as far as I can tell.

It seems that libp2p-tls isn't uploaded to crates.io yet, so that's why it's failing. Setting publish = false in its Cargo.toml will make cargo-semver-checks ignore it in workspace runs like so: https://github.com/hyperium/tonic/blob/master/tests/ambiguous_methods/Cargo.toml#L6

[thomaseizinger]

Should I be worried that the crates.io-published libp2p-mdns and libp2p-quic seem to have failed to compile per the log? That was a rustc compilation error in both, not a cargo-semver-checks issue as far as I can tell.

No need to worry, that is a different issue I am fixing: #3131

It seems that libp2p-tls isn't uploaded to crates.io yet, so that's why it's failing. Setting publish = false in its Cargo.toml will make cargo-semver-checks ignore it in workspace runs like so: https://github.com/hyperium/tonic/blob/master/tests/ambiguous_methods/Cargo.toml#L6

Interesting. We do intend to publish it with the next release.

The way I am intending to workaround this at the moment is: https://github.com/libp2p/rust-libp2p/blob/6d61142007165132e74ee437f3c9c9c41598b125/.github/workflows/ci.yml#LL71C9-L71C9

[obi1kenobi]

obi1kenobi/cargo-semver-checks#146 is a tricky one, and I'm not sure what the right answer is at the moment. On one hand, obviously it's unfortunate that this use case needs the workaround. On the other, cargo-semver-checks is explicitly told to check this crate by name. It's not just a namespace run, it's directly for that crate.

It's impossible in cargo-semver-checks right now to distinguish whether the crate is just unpublished (in which case it's fine from a semver perspective) or is actually published on a private registry. In the latter case, a passing outcome may give the user an incorrect impression that all lints were checked and passed, when that is not the case.

So making obi1kenobi/cargo-semver-checks#146 just pass (even while logging a warning) doesn't quite seem to live up to the user's expectations just as much as not passing fails to live up to them as well.

[mxinden]

@thomaseizinger based you adding the send-it label, I am guessing you wanted this to merge? It is missing your approval though.

[mergify]

This pull request has merge conflicts. Could you please resolve them @obi1kenobi? 🙏

[obi1kenobi]

Sorry, these are just my editor stripping trailing whitespace. If they bother you, I can look into making it not do this for this file.

[thomaseizinger]

That is fine, thanks! :)

[thomaseizinger]

@thomaseizinger based you adding the send-it label, I am guessing you wanted this to merge? It is missing your approval though.

I was wondering why it didn't do it yet haha