Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
crates.io: Crate Deletions #3660
crates.io: Crate Deletions #3660
Changes from all commits
75d4196
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Does this mean that even if there is a crate that depends on this crate, we still allow users to delete it within 72 hours?
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.
This seems different from the npm policy. Not sure if this is going to lead us to the pypi situation.(At least we have time limitation 😆)
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.
yes
I admit that I don't remember why I didn't include the restriction in this case 😅
I don't think it is a case that will come up particularly often though. If this is a new crate (< 72 hours) then it usually won't have the popularity yet to get a lot of reverse dependencies in that time frame. And if the popularity happens to exist because of the popularity of the author then that author will most likely be aware of the consequences of deleting a popular new crate.
As I mentioned before in other comments, we can always adjust the rules if we notice that something isn't quite working as intended :)
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.
I feel like this will replace yank in a lot of situations and feel like more friction or guard rails are needed.
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.
This does seem worrisome to me. I think depending on a deleted crate would be very disruptive for those users. I'm uncertain of the exact behavior, but I think users of those crates will either get a confusing download error (if using Cargo.lock), or a confusing resolution error (if not using Cargo.lock).
I think it would be helpful to make sure the UI has loud and explicit warnings about the consequences of this.
Would it be possible to also warn owners of reverse dependencies when this happens?
Would it be possible to include a marker in the index similar to yanked so that cargo can provide a better error message?
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.
Well, there aren't any owners of reverse dependencies, because "there are no reverse dependencies" is a requirement for deletion in this RFC.
I do think it's a good idea to include a marker in the index to provide a better error message for people who depend on the deleted crate in things which are not published to crates.io.
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.
I'm responding specifically to the version deletion, which does allow reverse dependencies. IIUC, the "no reverse dependencies" is for entire crate deletions only.
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.
I agree. This is certainly not an action that should be taken lightly. If implemented as a button on the website it should have a confirmation step that highlights the consequences of this action. If implemented as a cargo plugin or in cargo itself a similar confirmation step will probably make sense.
^
version requirements the reverse dependency would fall back to the previous version, so I'm not sure if a warning is necessary in this casegenerally, yes. when I thought about this question yesterday I thought a
{ "version": "1.2.3", "deleted": true }
would be sufficient, but I have a feeling that older cargo versions won't like such an index record particularly much.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.
hmm, so if I understand correctly a
v: 3
withdeleted: true
isn't really helping if old cargo would essentially ignore thev
anddeleted
fields 🤔I assume if we only included the
v
,version
anddeleted
fields then old cargo would consider the whole index file invalid? or would it only consider these records invalid?anyway, I'm open to adjusting the index to add the
deleted
information in some way if it helps cargo to display better errors and if we can figure out a way to make it work reasonably well with older cargo versions :)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.
No, cargo parses line-by-line, so it would just skip over deleted versions.
I'm not personally too concerned about older cargos. I'm more concerned about current cargo's ability to display a reasonable message.
For the record, I still think this 72-hour policy is dangerous and am concerned about the consequences.
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.
@Turbo87 I noticed that this conversation was marked as resolved, but I didn't see a response or corresponding update to the RFC to resolve it. Would it be possible to at least add the index considerations to the unresolved section?
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.
sure, I'll add that :)
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.
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.
Otherwise, this may produce the result that people are hesitant to count on new versions even of well-established crates.
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.
I don't know that only looking at published version requirements is enough for version deletion. A new crate with no reverse dependencies is unlikely to be picked up and used from git repositories within its first 72 hours. A new version of a widely used crate is likely to have dozens of repos updating to it within a day via dependabot or similar mechanisms.
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.
I'm not sure about the dependencies requirement. I see two scenarios:
crate A is used as a dependency of another library crate B published on crates.io: in this case the dependency declaration doesn't need to be bumped to always match the latest version of crate B since the implicit
^
version requirement makes cargo always choose the latest available version and it makes it possible to for downstream projects to choose which version to use (e.g. to not yet upgrade to https://github.com/servo/rust-url/releases/tag/v2.5.1 as a recent example 😅).crate A is used as a dev-dependency of another library, or as a proper dependency in some application: in this case you usually want to track the latest version and potentially even pin that version if you're using services like renovatebot. such services can already deal with deletions on registries like npm so I would assume they could also handle it for crates.io. worst case: you try to compile a project and cargo greets to with a "can not find matching version" error. I expect these cases to be pretty rare though, and I assume in most cases the crate author is likely to publish a fixed version afterwards anyway.
It is also worth keeping the technical complexity of such a check in mind. We would have to iterate through all dependency records in our database for this crate and run them against all of the versions of the crate. I'm not sure how viable that would be.
I'm open to a downloads requirement though, but as @Nemo157 pointed out, services like renovatebot will likely cause a large number of downloads immediately after release for the more popular packages, which will prevent them from ever deleting a version.
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.
I wouldn't agree with this assumption. Some projects immediately bump the dependency declaration. The reason for doing that is that without testing minimal versions, there's no way to know if you are depending on something that was updated in a newer version. The only safe path is to also update
Cargo.toml
to the latest version.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.
In a recent Cargo team meeting, we were discussing whether we should abandon
-Zminimal-version
and instead be more proactively moving version requirements up, which would hit this case more often.See https://internals.rust-lang.org/t/zminimal-versions-cargo-update-and-cargo-upgrade/21335
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.
In my opinion, if we start with a more strict policy, it will be easier to relax it later. However, if we just publish this policy, it might be difficult to add additional requirements later on.
I'm curious to know if I used that version (= 1.2.3) and released my crate in a very short time. what should I do after this particular version of my dependency crate is deleted and what happens when cargo resolves my dependency? What happens to packages that depend on my crate? If this happens, do I need to remove my crate as well?
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.
if you're the author of a crate depending on a deleted version then you should probably widen the dependency to a previous version of that crate and release a new version with that widened dependency declaration.
I don't think so. Removing/Deleting crates should be reserved for critical situations like the detection of malicious code or if you unintentionally published something that wasn't intended to be public. You can publish a new version of your crate and if you want you can yank the version of your crate that depends on the deleted dependency version.
as far as I remember cargo will show an error that it can't find a matching version for the dependency
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.
This comes across as a pretty glib stance towards breaking people and requiring ecosystem churn because of it. "Re-release" is no always a trivial answer.
That implies that you are wanting a specific set of behaviors, or culture, around removing packages but this RFC does nothing to establish this culture. For ideas on how to instill such a culture, see #3660 (comment)
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.
Are we expecting to at least clearly flag in the UI or index that the crate existed under some other author / version series? I could see this being very confusing when looking at e.g. crater results or other cases where we try to pull old crates.
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.
A marker in the index is one of several topics discussed at #3660 (comment)
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.
Right, I saw that, but I guess I interpreted that as mostly being when the version isn't reused - it's not obvious (lacking dates?) that we have a mechanism for identifying a crate after this RFC other than by hash of it's contents, since the same name and version can refer to ~infinitely many different hashes afaict.
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.
I'm not sure what this impossibility is referencing? If you publish a
2.0.0
, realize you left an API token or something in it and delete it, then you can still publish a2.0.1
as a new major version. Blocking a single version number indefinitely seems fine to me as there's ~infinite mostly equivalent numbers to use instead (the one case where there isn't is if you were publishing with gaps in your patch numbers and going back to fill in a missing patch number, but I've never seen that done with semver).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.
I agree. And there's a lot of advantage in saying "this old version might stop existing, but it'll never show up again with a different hash". (Also keep in mind that people might have cached local builds with the old version.)
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.
one place where i could see that running into problems is where you're trying to track exact versions of some 3rd-party library, where if they publish a particular version, you'd want to publish the exact same version number even if you mistakenly published something with that version number before
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.
@Nemo157 I personally agree, but I've seen enough cases of "the marketing team" pushing for "the big 2.0.0 release" and if that version number has already been burned it becomes a problem. But that's why it's in the "Unresolved questions" section, I'm open to input on what the community prefers. I tend to follow what other successful projects/communities/registries are doing unless there are good reasons against it, and since npm blocks deleted version numbers I would personally vote for that option too.