search

rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.59k stars 2.39k forks source link

Conservative updates no longer respect yanked crates since Rust 1.26.0 #6609

Closed alexcrichton closed 5 years ago

alexcrichton commented 5 years ago

Extracted from this comment Cargo no longer preserves dependencies on yanked crates when a dependency is updated. The cause of this is https://github.com/rust-lang/cargo/pull/5180 which was deployed in Rust 1.26.0, and the cause is evidenced in the logs of the resolver:

[2019-01-28T15:10:53Z DEBUG cargo::ops::resolve] poisoning registry `https://github.com/rust-lang/crates.io-index` because foo v0.1.0 (/home/alex/code/cd5e090d06a9af26add19650bad988b4) looks like it changed itertools

I haven't dug too much into this yet, but wanted to create a dedicated issue for this!

cc @Eh2406, you're likely very interested in this!

alexcrichton commented 5 years ago

Long-term I've always felt that our way of handling "locked" registries and such is a bit of a hack so I'd love to change it all, but I have no idea how we'd change it all!

In the near-term I think a fix for this would be to update the registry source to take a whitelist of crates that are allowed to be yanked. We'd just take all PackageId instances from the previous lock file and from the same registry source and just pass them all back into the registry when we construct the registry source.

The line here would then be removed in favor of just checking against the internal whitelist which was specified at construction time. This way "poisoning the registry" shouldn't affect the precise() dependency list and yanked dependencies should be available for dependencies.

Eh2406 commented 5 years ago

I don't know the code around/that-calls the resolver well enough to have a holistic solution for the long-term, but I do have ~opinions~ thoughts for parts I'd like to see. ... #5718 ... #5529 ... fuzz testing ... change the order of resolution ... only generating detailed errores when we will be showing them ... Maybe we should have a separate tracking issue for that redesign?

On topick. Yes, the plan of having a list of allowed yanks that is determined from the lockfile sounds correct. I don't know that code well enough to immediately see how that line relates to #5180. You can do it, or walk me through it as you wish given the time pressure from the ring situation.

alexcrichton commented 5 years ago

I talked some with @Eh2406 on Discord about this today and I think they're gonna try to tackle this in the near future.

Eh2406 commented 5 years ago

I am at a meetup. I was discussing what I was working on. As I was describing the bug each person, one by one, sead "hey we have been hitting that bug". So I think this is much more problematic than the reports suggests. When we get this fixed maybe we should consider backporting.

Eh2406 commented 5 years ago

Should we backport #6611 or should we just close this?

carols10cents commented 5 years ago

Given the number of people experiencing this, I would be in favor of backporting 👍

alexcrichton commented 5 years ago

Looks like this change is in nightly and we haven't received any alarming reports. This is a very longstanding bug in Cargo, but recent ecosystem events have caused it to become much worse. It's also (I personally think at least) a pretty low-risk patch.

I'm down for a backport! @Eh2406 would you be interested in doing the backport?

I'm gonna go ahead and close this since the bug is fixed on master in the meantime.