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

Do not update semantically equivalent lockfiles with --frozen/--locked. #4714

Merged
merged 1 commit into from
Nov 12, 2017

Conversation

Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Nov 12, 2017

A previous patch in #4684 attempted to fix this, but didn't work for the
case where the [root] crate wasn't the first crate in the sorted package
array.

cc @matklad -- fixes a problem noted in #4684 (comment) which appears to have gone unfixed

r? @alexcrichton

Beta backport will be posted as soon as this is reviewed. Merges cleanly.

// just ignore it if we are in the `--frozen` mode.
if !lock_update_allowed && orig.starts_with("[root]") {
orig = orig.replacen("[root]", "[[package]]", 1);
match (orig.parse::<toml::Value>(), current.parse::<toml::Value>()) {
Copy link
Member

Choose a reason for hiding this comment

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

😭

I've checked that toml::Value uses BTree for maps, but of course it is irrelevant here, because [[package]] is an array, and not a map.

///
/// For example, even with [root] being present in one and not in the other,
/// the semantic meaning is still the same, so return true.
pub fn semantically_eq(self, other: Self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

I am somewhat afraid that these needs to be updated every time we add new field to EncodableResolve.

Perhaps we should have fn normalize(&mut self), and then use natural equality?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @matklad in that we may want to pursue alternative means if we can, for example can into_resolve be used below and then we would equate two instances of Resolve?

A previous patch in rust-lang#4684 attempted to fix this, but didn't work for the
case where the [root] crate wasn't the first crate in the sorted package
array.
@Mark-Simulacrum
Copy link
Member Author

Updated.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Nov 12, 2017

📌 Commit 89023dc has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Nov 12, 2017

⌛ Testing commit 89023dc with merge abd137a...

bors added a commit that referenced this pull request Nov 12, 2017
Do not update semantically equivalent lockfiles with --frozen/--locked.

A previous patch in #4684 attempted to fix this, but didn't work for the
case where the [root] crate wasn't the first crate in the sorted package
array.

cc @matklad -- fixes a problem noted in #4684 (comment) which appears to have gone unfixed

r? @alexcrichton

Beta backport will be posted as soon as this is reviewed. Merges cleanly.
@bors
Copy link
Contributor

bors commented Nov 12, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing abd137a to master...

@bors bors merged commit 89023dc into rust-lang:master Nov 12, 2017
bors added a commit that referenced this pull request Nov 12, 2017
…hton

[beta] Do not update semantically equivalent lockfiles with --frozen/--locked.

A previous patch in #4684 attempted to fix this, but didn't work for the
case where the [root] crate wasn't the first crate in the sorted package
array.

Backport of #4714.
@Mark-Simulacrum Mark-Simulacrum deleted the frozen-freezes branch November 12, 2017 23:03
@ehuss ehuss added this to the 1.23.0 milestone Feb 6, 2022
@ehuss ehuss modified the milestones: 1.23.0, 1.22.0 Feb 14, 2022
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.

5 participants