-
Notifications
You must be signed in to change notification settings - Fork 2.7k
feat(generate-lockfile): Add unstable --publish-time flag #16265
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
Conversation
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.
While the long value_name makes the format very clear, the length is annoying
| fn datetime_completer(current: &std::ffi::OsStr) -> Vec<CompletionCandidate> { | ||
| let mut completions = vec![]; | ||
| let Some(current) = current.to_str() else { | ||
| return completions; | ||
| }; | ||
|
|
||
| if current.is_empty() { | ||
| // While not likely what people want, it can at least give them a starting point to edit | ||
| let timestamp = jiff::Timestamp::now(); | ||
| completions.push(CompletionCandidate::new(timestamp.to_string())); | ||
| } else if let Ok(date) = current.parse::<jiff::civil::Date>() { | ||
| if let Ok(zoned) = jiff::Zoned::default().with().date(date).build() { | ||
| let timestamp = zoned.timestamp(); | ||
| completions.push(CompletionCandidate::new(timestamp.to_string())); | ||
| } | ||
| } | ||
| completions | ||
| } |
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.
Didn't feel like writing my own parser for the sake of handling incomplete date, so I threw this together to at least make it easier to get the format correct so people can modify them from here.
| let mut ws = args.workspace(gctx)?; | ||
| if let Some(publish_time) = publish_time { | ||
| gctx.cli_unstable() | ||
| .fail_if_stable_opt("--publish-time", 5221)?; |
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.
Do we want to close the original issue and create a new one for tracking?
Benefit of this:
- Cargo team has full control over the issue. Original author won't delete that accidentally.
- Sometimes original issue has too many discussion. And we tend to use tracking issue for, well, tracking progress 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.
Sometimes original issue has too many discussion. And we tend to use tracking issue for, well, tracking progress only.
I know the compiler team is concerned about this but it hasn't really felt like it has been too much of a problem for us.
As an end-user, it can be annoying to have to follow the problem you are tracking go through different issues for different stages of the process.
It also splits up the conversation and history.
The part I worry most about, which is less of a problem in this case, is when we close the original issue as the tracking issue may be for one particular solution which we might later reject and then we need to remember to re-open the original issue (I guess that is a reason to create a tracking issue and then close the original on stabilization?)
I lean towards creating a tracking issue but leaving the original open. Does that make sense?
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.
Sounds good to me.
| .arg( | ||
| clap::Arg::new("publish-time") | ||
| .long("publish-time") | ||
| .value_name("yyyy-mm-ddThh:mm:ssZ") |
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.
mm is ambiguous here 😆.
(no need to change)
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 had considered doing MM vs mm but
- People likely pick it up from context
- I wonder how many people remember which is which
- I wanted to emphasize the difference between literals (upper case) and substitutions (lower case)
| self.publish_time = Some(publish_time); | ||
| } | ||
|
|
||
| /// Sort (and filter) the given vector of summaries in-place |
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.
Probably expand this doc comment?
|
|
||
| This is a best-effort filter on allowed packages, including: | ||
| - packages from unsupported registries are always accepted | ||
| - only the latest yank status is respected |
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 had a hard time understanding this. Do need a test with yanked 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.
I had a hard time understanding this.
We do not record each yank or unyank operation and timestamp them, instead only whether it is currently yanked or not. So if a package was yanked before publish-time and then unyanked after, we'll resolve with it unyanked despite that not being the state of the index as of publish-time.
Do need a test with yanked versions?
I'm trying to decide what a test would look like that would be meaningful. it would basically be a test showing that yanking works. Even if we later track yank history, we'd need to update our testing infrastructure to support it and it is unlikely it would be done in a way for us to know to update that test.
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 a package was yanked but this yank happened after the time passed to --publish-time, will it be considered not yanked by the resolver?
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.
cargo generate-lockfile will only look at the current yank status, independent of --publish-time. In your scenario, it is currently yanked so that is the state that will be used.
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.
Wouldn't part of the value of having --publish-time at all be to be able to generate a working lockfile even if all compatible versions of a crate are yanked?
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 your concern is more about wanting to bypass yanked status, we have other places where we are discussing ways to deal with that.
For the given use case of "lock to a specific time", for it to be accurate to the past, we would need the yank status of as of that time. I recognized that this is a shortcoming of this solution by documenting it in the help, calling it out in the PR description ("big caveats'), and leaving it as an unresolved question for the tracking issue. As I said in the PR description, this is more about providing a testing ground for the pubtime Index Summary entry. Even then, I think there are still use cases for this feature as-is.
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.
- only the latest yank status is respected
Should we expand this a bit to say that yank time is not tracked? It is not obvious to users (even me) what "latest yank status" means here, as they may not even know yank is a mutating operation.
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.
Made a small tweak but likely not covering what you are looking for. Maybe it is curse of knowledge but I'm having a hard time thinking of how to further clarify this. maybe provide an example?
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.
Looks good to me. Let's tweak if any other confusion pops up.
| /// incorrectly select a new package that uses a new index schema. A | ||
| /// workaround is to downgrade any packages that are incompatible with the | ||
| /// `--precise` flag of `cargo update`. | ||
| pub v: Option<u32>, |
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.
We should bump a index version, no?
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.
iiuc the index version represents incompatible versions of the schema. This change is forward and backwards compatible so it shouldn't need it bumped.
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.
How were version 2 and 3 (see the comment) incompatible versions? I probably forgot the policy. Looks like every field additions should be treated as the same version?
Also, have we dealt with CURRENT_CACHE_VERSION the on disk summary cache? If a cache was generated by older cargo, and them cargo generate-lockfile --publish-time read from it, would it not respect the pubtime field because of missing?
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 unsure why bindeps bumps the version but it does declare an incompatible version, see
cargo/src/cargo/sources/registry/index/mod.rs
Lines 682 to 699 in b5354b5
| let v = index.v.unwrap_or(1); | |
| tracing::trace!("json parsed registry {}/{}", index.name, index.vers); | |
| let v_max = if bindeps { | |
| INDEX_V_MAX + 1 | |
| } else { | |
| INDEX_V_MAX | |
| }; | |
| if v_max < v { | |
| Ok(IndexSummary::Unsupported(summary, v)) | |
| } else if !valid { | |
| Ok(IndexSummary::Invalid(summary)) | |
| } else if index.yanked.unwrap_or(false) { | |
| Ok(IndexSummary::Yanked(summary)) | |
| } else { | |
| Ok(IndexSummary::Candidate(summary)) | |
| } |
As this new field is set on every Summary, if we bump .v for this, then registries providing this information would break support for all older versions of Cargo.
We did not bump it when we added rust_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.
Also, have we dealt with
CURRENT_CACHE_VERSIONthe on disk summary cache? If a cache was generated by older cargo, and themcargo generate-lockfile --publish-timeread from it, would it not respect thepubtimefield because of missing?
That cache is for what we get back from the registry, before we turn it into Cargo-native types and possibly lose fields, so if the server has it, we'll preserve it.
I do not think we bump this field when doing backports but detect that we need to overwrite it. We didn't do anything for it for rust_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 do not think we bump this field when doing backports but detect that we need to overwrite it. We didn't do anything for it for rust_version.
Yeah I thought about this as well. crates.io index backport shouldn't touch the index version.
Seems reasonable to me.
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Implementation for rust-lang#5221 Use cases: - Improved reproduction steps using cargo scripts without including a lockfile - Debugging issues in the past - Manual stop gap for rust-lang#15973 Unresolved questions: - How do we compensate for the caveats, with one option being to leave this perma-unstable? - How should we deal with Summary `pubtime` parse errors? - How strict should we be on the Summary `pubtime` format? - Should we offer a convenient way of getting a compatible timestamp? Future possibilities: - Ability to lock to a timestamp with `cargo update` updating the timestamp (could be useful for cargo scripts) This seemed like the shortest path to testing the proposed `pubtime` index entries so we can unblock other work on it like rust-lang#15973. While this has some big caveats, the design is straightforward. In contrast, rust-lang#15973 has a lot more design questions (is this a resolver-wide setting or a per-registry setting, what formats are accepted, etc) that could slow down development and testing `pubtime`.
1828830 to
57d0592
Compare
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 adds an unstable --publish-time flag mainly for demonstrating the use of the pubtime field in the crates.io index files. We don't have any plan to stabilize the flag yet.
| /// incorrectly select a new package that uses a new index schema. A | ||
| /// workaround is to downgrade any packages that are incompatible with the | ||
| /// `--precise` flag of `cargo update`. | ||
| pub v: Option<u32>, |
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 do not think we bump this field when doing backports but detect that we need to overwrite it. We didn't do anything for it for rust_version.
Yeah I thought about this as well. crates.io index backport shouldn't touch the index version.
Seems reasonable to me.
|
|
||
| This is a best-effort filter on allowed packages, including: | ||
| - packages from unsupported registries are always accepted | ||
| - only the latest yank status is respected |
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.
Looks good to me. Let's tweak if any other confusion pops up.
What does this PR try to resolve?
Implementation for #5221, #15491
Tracking issues: #16270, #16271
Use cases:
lockfile
This seemed like the shortest path to testing the proposed
pubtimeindex entries (#15491) so we can unblock other work on it like #15973.
While this has some big caveats, the design is straightforward.
In contrast, #15973 has a lot more design questions (is this a
resolver-wide setting or a per-registry setting, what formats are
accepted, etc) that could slow down development and testing
pubtime.How to test and review this PR?
Unresolved questions (deferred to the tracking issues):
this perma-unstable?
pubtimeparse errors?pubtimeformat?Future possibilities:
cargo updateupdating thetimestamp (could be useful for cargo scripts)