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

Add '--to-lockfile' flag to cargo-upgrade #337

Merged
merged 3 commits into from
Oct 27, 2019
Merged

Conversation

tofay
Copy link
Contributor

@tofay tofay commented Sep 28, 2019

When this flag is present, all dependencies in 'Cargo.toml'
are upgraded to the locked version as recorded in 'Cargo.lock'.

Fixes #297

@tofay
Copy link
Contributor Author

tofay commented Sep 28, 2019

I saw issue 297 whilst I was working on #336, so had a go at implementing it. This is marked as a WIP as I've not yet scrutinized the test that I've added here, or added sufficient comments.

@tofay tofay changed the title WIP: Add '--to-lockfile' flag to cargo-upgrade Add '--to-lockfile' flag to cargo-upgrade Sep 29, 2019
Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the PR.

I've left some suggestions and questions and a rebase is needed as well.

tempdir = "0.3.7"

[build-dependencies]
serde = { version = "1.0", git= "https://github.com/serde-rs/serde.git" }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel a bit uneasy with all the dependencies here, why is it not enough to have the same dependencies as in tests/fixtures/workspace/one/Cargo.toml?

The local lock file must be present and up to date if
this flag is passed.

Does this mean CI will fail each time a new version is released (or git commit is pushed)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used cargo's terminology here: a lockfile is up-to-date if it satisfies the Cargo.toml, not if all the dependencies are at the most recent semver compatible version. See docstring for --locked at https://doc.rust-lang.org/cargo/commands/cargo-metadata.html.

It's a little confusing, but I thought it better to be consistent with cargo. WDYT?

The network isn't accessed when the --to-lockfile flag is passed, so CI won't fail. I've simplified the test as you suggested, and deliberately added a lockfile with an older version of rand to demonstrate.


If the '--to-lockfile' flag is supplied, all dependencies will be upraged to the currently locked
version as recorded in the local lock file. The local lock file must be present and up to date if
this flag is passed."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you clarify in the docs what you mean by MUST, e.g. will it result in an error, how does it work with the offline mode, etc

@ordian
Copy link
Collaborator

ordian commented Oct 27, 2019

@tofay merge conflicts needs to be resolved as well, let me know when the PR is ready

tofay and others added 2 commits October 27, 2019 17:23
When this flag is present, all dependencies in 'Cargo.toml'
are upgraded to the locked version as recorded in 'Cargo.lock'.

Multiple manifests in a workspace may request the same dependency
at different versions. The version of each dependency is
updated to the corresponding semver compatible version in the
lockfile.
@tofay tofay requested a review from ordian October 27, 2019 19:14
Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Co-Authored-By: Andronik Ordian <[email protected]>
@ordian
Copy link
Collaborator

ordian commented Oct 27, 2019

bors r+

bors bot added a commit that referenced this pull request Oct 27, 2019
337: Add '--to-lockfile' flag to cargo-upgrade r=ordian a=tofay

When this flag is present, all dependencies in 'Cargo.toml'
are upgraded to the locked version as recorded in 'Cargo.lock'.

Fixes #297

Co-authored-by: Tom Fay <[email protected]>
Co-authored-by: Tom Fay <[email protected]>
@tofay
Copy link
Contributor Author

tofay commented Oct 27, 2019

Thanks for the quick turnaround!

@bors
Copy link
Contributor

bors bot commented Oct 27, 2019

@bors bors bot merged commit 6fed4c2 into killercup:master Oct 27, 2019
@jplatte
Copy link
Collaborator

jplatte commented Oct 28, 2019

I only saw this after the release, so sorry for the late feedback, but shouldn't this option be called from-lockfile? to-lockfile to me implies writing the lockfile.

@ordian
Copy link
Collaborator

ordian commented Oct 28, 2019

@jplatte hey, thanks for the feedback. It reads as upgrade (Cargo.toml) to (the state of) lockfile to me (cargo upgrade only deals with Cargo.toml upgrades as opposed to cargo update), although I understand the confusion. The name was suggested here btw.

@jplatte
Copy link
Collaborator

jplatte commented Oct 28, 2019

Yeah I get how you can read upgrade --to-lockfile as "upgrade to the lockfile's state". Probably isn't worth renaming it unless more people complain about the naming. I'm not even sure if upgrade --from-lockfile would be better.

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

Successfully merging this pull request may close these issues.

Update dependency versions in Cargo.toml to that of Cargo.lock
3 participants