Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cargo semver checks ignores my lockfile #675

Open
epage opened this issue Feb 27, 2024 · 8 comments
Open

Cargo semver checks ignores my lockfile #675

epage opened this issue Feb 27, 2024 · 8 comments
Labels
A-cli Area: engine around the lints C-enhancement Category: raise the bar on expectations

Comments

@epage
Copy link
Collaborator

epage commented Feb 27, 2024

See

For more context, see also zulip

According to the zulip thread, this is done because

  • "baseline" for crates.io won't have a lockfile and will use the latest dependencies. If one of those is a public dependency and is re-exported, then we need to make sure the versions are aligned
    • In cargo's case, we are verifying a git revision which I don't think this applies here
  • to verify the latest versions of your dependencies

The problems with doing this

  • This adds non-determinism to CI jobs, increasing the chance of failure (the problem Cargo ran into)
  • There are times when "latest dependencies" can't be calculated
    • e.g. There are corner cases with multi-major-version version requirements where the wrong version gets selected
@obi1kenobi obi1kenobi added A-cli Area: engine around the lints C-enhancement Category: raise the bar on expectations labels Feb 27, 2024
@obi1kenobi
Copy link
Owner

Thanks!

What would be an alternative to what we're doing today?

Could c-s-c somehow ask cargo to reuse the dependency versions of another lockfile while generating the baseline rustdoc JSON? While what we're doing today could be argued to be a hack, any alternative seems like an even worse hack.

There are corner cases with multi-major-version version requirements where the wrong version gets selected

Could you say more about this, or point me to an issue where I can learn about it? We defer dependency version selection to cargo so of course if cargo does the wrong thing then c-s-c will as well.

@obi1kenobi obi1kenobi changed the title Cargo semver checks blows away my lockfile Cargo semver checks ignores my lockfile Feb 27, 2024
@obi1kenobi
Copy link
Owner

Updated the title to avoid potential confusion. cargo-semver-checks would never delete your lockfile, it merely ignores it.

@epage epage changed the title Cargo semver checks ignores my lockfile Cargo semver checks blows away my lockfile Feb 27, 2024
@epage
Copy link
Collaborator Author

epage commented Feb 27, 2024

I disagree with changing the title and reverted it back. It doesn't just ignore it, it changes my repo. My experience with a lot of developers is that they see a change in the repo and assume its intentional and commit it, making this another source of problems. A general rule of well-formed cargo commands is they should only modify the lockfile when there is a mismatch with Cargo.toml.

@obi1kenobi
Copy link
Owner

It doesn't just ignore it, it changes my repo.

Could you provide a repro where running cargo semver-checks in a clean checkout makes a change in the repo? If this is accurate, it's a serious bug that I'd like to look into immediately.

The current implementation is designed to create its own lockfile in a separate directory inside the target dir, and should never ever mess with the repo's Cargo.lock or any other file in the project. If that isn't the case, something is seriously wrong.

@epage epage changed the title Cargo semver checks blows away my lockfile Cargo semver checks ignores my lockfile Feb 27, 2024
@epage
Copy link
Collaborator Author

epage commented Feb 27, 2024

Thanks for the clarification that it only ignores and doesn't modify my repo. I've reverted the title back to you version.

@obi1kenobi
Copy link
Owner

Thanks! If you have a sec, I'd love your thoughts on this earlier comment as well 👇

What would be an alternative to what we're doing today?

Could c-s-c somehow ask cargo to reuse the dependency versions of another lockfile while generating the baseline rustdoc JSON? While what we're doing today could be argued to be a hack, any alternative seems like an even worse hack.

There are corner cases with multi-major-version version requirements where the wrong version gets selected

Could you say more about this, or point me to an issue where I can learn about it? We defer dependency version selection to cargo so of course if cargo does the wrong thing then c-s-c will as well.

@epage
Copy link
Collaborator Author

epage commented Feb 27, 2024

I'll re-phrase my earlier comment: well-formed cargo commands should respect the existing lockfile.

When running with a git baseline, there is no problem as a lockfile is present if its intended to be.

Yes, there are questions on what to do with registry baselines. I don't have a magical end-all answer. This is something that will likely take exploration. Possible ideas

There are corner cases with multi-major-version version requirements where the wrong version gets selected

Could you say more about this, or point me to an issue where I can learn about it? We defer dependency version selection to cargo so of course if cargo does the wrong thing then c-s-c will as well.

I was vague because I haven't been touching those projects in a well. The rough idea is that cargo resolves dependencies maximally so if you have two paths to a dependency with links (or its used in public APIs, or both) and one has a multi-major version version requirement, the latest will be picked while a more normal version requirement will pick a lower version. rust-lang/cargo#9029 is the resolver issue for this.

@obi1kenobi
Copy link
Owner

Thanks, that makes sense! I subscribed to the issue you linked as well — I didn't know cargo heuristically decided whether to include the lockfile in the crate.

copy over the existing lockfile and let Cargo fill in the pieces

To generate rustdoc for the current code, we create a new project and declare a path dependency on the crate being checked. As pseudocode, we do more or less the equivalent of:

cargo new --lib placeholder && \
    cd placeholder && \
    cargo add --path /path/to/crate/being/checked && \
    cargo doc -p crate_being_checked --output-format json

Would copying over the existing project's lockfile and letting Cargo fill in the pieces work here? That sounds ideal, but my prior assumption was that it wouldn't work since the lockfile is for a completely different project. I'd love to be pleasantly surprised about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Area: engine around the lints C-enhancement Category: raise the bar on expectations
Projects
None yet
Development

No branches or pull requests

2 participants