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

Ensure that patch version is selected by lockfile after patching #106

Open
eopb opened this issue Sep 2, 2024 · 3 comments
Open

Ensure that patch version is selected by lockfile after patching #106

eopb opened this issue Sep 2, 2024 · 3 comments
Assignees

Comments

@eopb
Copy link
Owner

eopb commented Sep 2, 2024

Problem

Quoting from https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html#testing-a-bugfix:

The way [patch] works is that it’ll load the dependency at ../path/to/uuid and then whenever crates.io is queried for versions of uuid it’ll also return the local version.

This means that the version number of the local checkout is significant and will affect whether the patch is used. Our manifest declared uuid = "1.0" which means we’ll only resolve to >= 1.0.0, < 2.0.0, and Cargo’s greedy resolution algorithm also means that we’ll resolve to the maximum version within that range. Typically this doesn’t matter as the version of the git repository will already be greater or match the maximum version published on crates.io, but it’s important to keep this in mind!

...

...If you don’t see the local path version getting built then you may need to run cargo update uuid --precise $version where $version is the version of the locally checked out copy of uuid.

This means that it's possible for a patch created with cargo-override to not be selected, despite the version compatibility checks we do. This is due to cargo selecting the latest version with little regard to where it comes from.

Suggested solution

First we need to reproduce this issue. I don't want to risk trying to fix something without demonstrating that cargo works in ways that match our assumptions. This test could just be done on a local machine, and once done, a summary posted here.

For the solution, we can automate the guidance from the article above. We already have the exact version of the patch, so a call to cargo update <pkgid> --precise <version> should be possible. We shouldn't need to check the lockfile before doing this, since the precise update should be a no-op on correct lockfiles. We can do this for every call to cargo-override.

Open questions

What should we use to call cargo update?

@AjithPanneerselvam AjithPanneerselvam self-assigned this Sep 2, 2024
@eopb
Copy link
Owner Author

eopb commented Sep 2, 2024

Note, we have a --locked flag which "Assert that Cargo.lock will remain unchanged".

If this flag is passed, we should not attempt to update the lockfile. Ideally, we should emit a warning if the versions don't align, and --locked is passed, but we can keep that outside of the scope of this issue.

@epage
Copy link
Contributor

epage commented Sep 5, 2024

Technically, a patch will change Cargo.lock, so --locked should always fail unless the patch is already applied.

@futile
Copy link

futile commented Oct 24, 2024

Just ran into this as well, and just used cargo update <package>. This picked up the correct version from my local checkout, and probably also did semantic-version checking that it's allowed given the constraints of the original dep in Cargo.toml. Not sure which is better, just wanted to provide another data point 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants