-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Enable yanked dependencies with an unstable flag #5967
Conversation
r? @matklad (rust_highfive has picked a reviewer for you, use r? to override) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! The implementation and idea here are fine by me, but I'm wary to merge perma-unstable features. We've had a very bad track record of features in rustc which are merged but have no path to stabilization, and I would prefer to avoid the same in Cargo.
To that end, do others on @rust-lang/cargo perhaps see this flag ever becoming stable? Do we think we could ever stabilize functionality like this?
I don't think we should have a stable way to do this, and if Crater didn't need it I'd argue we shouldn't have an unstable way to do this either. |
Several of our unstable functionality are because Crater needs it. In addition to |
I suggest delisting such features from docs and |
28472fa
to
632ace6
Compare
@pietroalbini to get a sense of scale here, how many crates fail to compile due to a dependence on yanked crates? What percentage of all crater-tested crates does this cover? |
@Eh2406 do we have other unstable functionality specifically for crater? One possible one, |
At the moment ~170, which is 1% of the crates we're testing. The other option we have right now is to manually blacklist them, or have them sit in the "Crater errors" section of the report (which is not ideal, I'd like to have nothing in it so we can catch new ones easily).
Crater doesn't use |
when I said it it looked like #5961 was going to be in the list as well. |
Ah sorry I thought I replied sooner. @pietroalbini 1% seems like a pretty small number and given the historical resistance (and current resistance) to this feature, would you be ok adding such crates to the blacklist? |
☔ The latest upstream changes (presumably #5995) made this pull request unmergeable. Please resolve the merge conflicts. |
I'd prefer to wait on this until I import all the GitHub repositories into Crater, to see if the numbers change. |
Ok, the number of yanked crates didn't really change. Closing this. |
This PR adds a new unstable CLI flag,
-Z allow-yanked-deps
, that allows to depend on yanked crates even if there is no existing lockfile pointing at them (it basically disables the yanked check).Crater needs this because it regenerates lockfiles on every run, so there are tons of crates that fail to build because they depend on yanked deps (like futures 0.2). Crater already uses unstable flags during lockfile generation, so I don't really care if this is stabilized or not.
Closes #4225
cc rust-lang/crater#243