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

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

Closed
alexcrichton opened this issue Jan 28, 2019 · 7 comments
Closed
Labels
A-dependency-resolution Area: dependency resolution and the resolver C-bug Category: bug

Comments

@alexcrichton
Copy link
Member

Extracted from this comment Cargo no longer preserves dependencies on yanked crates when a dependency is updated. The cause of this is #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 alexcrichton added the A-dependency-resolution Area: dependency resolution and the resolver label Jan 28, 2019
@alexcrichton
Copy link
Member Author

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
Copy link
Contributor

Eh2406 commented Jan 28, 2019

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
Copy link
Member Author

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
Copy link
Contributor

Eh2406 commented Jan 31, 2019

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.

bors added a commit that referenced this issue Feb 4, 2019
keep track of crates that are whitelisted to be used even if yanked

This is a start on #6609. It definitely needs tests that the bug is fixed, and to reduce the clones. But for now let's see what CI thinks.
@Eh2406
Copy link
Contributor

Eh2406 commented Feb 11, 2019

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

@Eh2406 Eh2406 added the C-bug Category: bug label Feb 11, 2019
@carols10cents
Copy link
Member

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

@alexcrichton
Copy link
Member Author

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.

bors added a commit that referenced this issue Feb 12, 2019
[BackPort] Conservative updates

This is a back port of #6611 as discussed in #6609
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependency-resolution Area: dependency resolution and the resolver C-bug Category: bug
Projects
None yet
Development

No branches or pull requests

3 participants