-
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
Changes from all commits
a83ebb9
c802c3e
1340d22
b9bdb38
57d0592
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,6 @@ | ||
| use clap_complete::engine::ArgValueCompleter; | ||
| use clap_complete::engine::CompletionCandidate; | ||
|
|
||
| use crate::command_prelude::*; | ||
|
|
||
| use cargo::ops; | ||
|
|
@@ -9,13 +12,46 @@ pub fn cli() -> Command { | |
| .arg_manifest_path() | ||
| .arg_lockfile_path() | ||
| .arg_ignore_rust_version_with_help("Ignore `rust-version` specification in packages") | ||
| .arg( | ||
| clap::Arg::new("publish-time") | ||
| .long("publish-time") | ||
| .value_name("yyyy-mm-ddThh:mm:ssZ") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had considered doing
|
||
| .add(ArgValueCompleter::new(datetime_completer)) | ||
| .help("Latest publish time allowed for registry packages (unstable)") | ||
| .help_heading(heading::MANIFEST_OPTIONS) | ||
| ) | ||
| .after_help(color_print::cstr!( | ||
| "Run `<bright-cyan,bold>cargo help generate-lockfile</>` for more detailed information.\n" | ||
| )) | ||
| } | ||
|
|
||
| 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 | ||
| } | ||
|
Comment on lines
+28
to
+45
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
|
||
| pub fn exec(gctx: &mut GlobalContext, args: &ArgMatches) -> CliResult { | ||
| let ws = args.workspace(gctx)?; | ||
| let publish_time = args.get_one::<String>("publish-time"); | ||
| let mut ws = args.workspace(gctx)?; | ||
| if let Some(publish_time) = publish_time { | ||
| gctx.cli_unstable() | ||
| .fail_if_stable_opt("--publish-time", 5221)?; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good to me. |
||
| ws.set_resolve_publish_time(publish_time.parse().map_err(anyhow::Error::from)?); | ||
| } | ||
| ops::generate_lockfile(&ws)?; | ||
| Ok(()) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ pub struct VersionPreferences { | |
| prefer_patch_deps: HashMap<InternedString, HashSet<Dependency>>, | ||
| version_ordering: VersionOrdering, | ||
| rust_versions: Vec<PartialVersion>, | ||
| publish_time: Option<jiff::Timestamp>, | ||
| } | ||
|
|
||
| #[derive(Copy, Clone, Default, PartialEq, Eq, Hash, Debug)] | ||
|
|
@@ -53,6 +54,10 @@ impl VersionPreferences { | |
| self.rust_versions = vers; | ||
| } | ||
|
|
||
| pub fn publish_time(&mut self, publish_time: jiff::Timestamp) { | ||
| self.publish_time = Some(publish_time); | ||
| } | ||
|
|
||
| /// Sort (and filter) the given vector of summaries in-place | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably expand this doc comment? |
||
| /// | ||
| /// Note: all summaries presumed to be for the same package. | ||
|
|
@@ -63,6 +68,7 @@ impl VersionPreferences { | |
| /// 3. `first_version`, falling back to [`VersionPreferences::version_ordering`] when `None` | ||
| /// | ||
| /// Filtering: | ||
| /// - `publish_time` | ||
| /// - `first_version` | ||
| pub fn sort_summaries( | ||
| &self, | ||
|
|
@@ -77,6 +83,15 @@ impl VersionPreferences { | |
| .map(|deps| deps.iter().any(|d| d.matches_id(*pkg_id))) | ||
| .unwrap_or(false) | ||
| }; | ||
| if let Some(max_publish_time) = self.publish_time { | ||
| summaries.retain(|s| { | ||
| if let Some(summary_publish_time) = s.pubtime() { | ||
| summary_publish_time <= max_publish_time | ||
| } else { | ||
| true | ||
| } | ||
| }); | ||
| } | ||
| summaries.sort_unstable_by(|a, b| { | ||
| let prefer_a = should_prefer(&a.package_id()); | ||
| let prefer_b = should_prefer(&b.package_id()); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.