-
Notifications
You must be signed in to change notification settings - Fork 218
feat(cli): add cratesfyi command to queue rebuilds for specific broken nightly dates #2996
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
base: master
Are you sure you want to change the base?
Changes from all commits
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -10,6 +10,7 @@ use crate::{ | |||||
| utils::{ConfigName, get_config, get_crate_priority, report_error, retry, set_config}, | ||||||
| }; | ||||||
| use anyhow::Context as _; | ||||||
| use chrono::NaiveDate; | ||||||
| use fn_error_context::context; | ||||||
| use futures_util::{StreamExt, stream::TryStreamExt}; | ||||||
| use sqlx::Connection as _; | ||||||
|
|
@@ -22,6 +23,8 @@ use tracing::{debug, error, info, instrument, warn}; | |||||
| /// collapsed in the UI. | ||||||
| /// For normal build priorities we use smaller values. | ||||||
| pub(crate) const REBUILD_PRIORITY: i32 = 20; | ||||||
| // TODO what value should we use here? | ||||||
|
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'm not sure which other priority values are used, other than the default value above. Can anyone confirm whether the default I used below (30) is acceptable?
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. I think a higher value would be good, I'm thinking about 10. Currently in use: ( that being said, I should put these priorities into some structured things, where it's documented what they mean :) ) |
||||||
| pub(crate) const BROKEN_RUSTDOC_REBUILD_PRIORITY: i32 = 30; | ||||||
|
|
||||||
| #[derive(Debug, Clone, Eq, PartialEq, serde::Serialize)] | ||||||
| pub(crate) struct QueuedCrate { | ||||||
|
|
@@ -726,9 +729,75 @@ pub async fn queue_rebuilds( | |||||
| Ok(()) | ||||||
| } | ||||||
|
|
||||||
| /// Queue rebuilds for failed crates due to a faulty version of rustdoc | ||||||
| /// | ||||||
| /// It is assumed that the version of rustdoc matches the one of rustc, which is persisted in the DB. | ||||||
| /// The priority of the resulting rebuild requests will be lower than previously failed builds. | ||||||
| /// If a crate is already queued to be rebuilt, it will not be requeued. | ||||||
| /// Start date is inclusive, end date is exclusive. | ||||||
| #[instrument(skip_all)] | ||||||
| pub async fn queue_rebuilds_faulty_rustdoc( | ||||||
| conn: &mut sqlx::PgConnection, | ||||||
| build_queue: &AsyncBuildQueue, | ||||||
| start_nightly_date: &NaiveDate, | ||||||
| end_nightly_date: &Option<NaiveDate>, | ||||||
| ) -> Result<i32> { | ||||||
| let end_nightly_date = | ||||||
| end_nightly_date.unwrap_or_else(|| start_nightly_date.succ_opt().unwrap()); | ||||||
| let mut results = sqlx::query!( | ||||||
| r#" | ||||||
| SELECT c.name, | ||||||
| r.version AS "version: Version" | ||||||
| FROM crates AS c | ||||||
| JOIN releases AS r | ||||||
| ON c.latest_version_id = r.id | ||||||
|
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. This join would only queue rebuilds for
IMO when we rebuild for a broken nightly, we also have to fix older releases, even where this a newer one.
Suggested change
|
||||||
| AND r.rustdoc_status = TRUE | ||||||
|
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. small logic bug: on build failures, But, thinking about that a little more: We also had cases where a broken nightly version didn't lead to build failures, just broken docs.
Suggested change
|
||||||
| JOIN LATERAL ( | ||||||
|
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. you can join |
||||||
| SELECT b.id, | ||||||
| b.build_status, | ||||||
| b.rustc_nightly_date, | ||||||
| COALESCE(b.build_finished, b.build_started) AS last_build_attempt | ||||||
| FROM builds AS b | ||||||
| WHERE b.rid = r.id | ||||||
| ORDER BY last_build_attempt DESC | ||||||
| LIMIT 1 | ||||||
| ) AS b ON b.build_status = 'failure' AND b.rustc_nightly_date >= $1 AND b.rustc_nightly_date < $2 | ||||||
|
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. small improvement idea, now that I'm thinking about it in more detail, and only if you have time right now, I would merge without for now and add it myself if needed: We also had cases where nightly rustdoc was broken, but builds were successful, only docs were broken. |
||||||
|
|
||||||
| "#, start_nightly_date, end_nightly_date | ||||||
| ) | ||||||
| .fetch(&mut *conn); | ||||||
|
|
||||||
| let mut results_count = 0; | ||||||
| while let Some(row) = results.next().await { | ||||||
| let row = row?; | ||||||
|
|
||||||
| if !build_queue | ||||||
| .has_build_queued(&row.name, &row.version) | ||||||
| .await? | ||||||
| { | ||||||
| results_count += 1; | ||||||
| info!( | ||||||
| "queueing rebuild for {} {} (priority {})...", | ||||||
| &row.name, &row.version, BROKEN_RUSTDOC_REBUILD_PRIORITY | ||||||
| ); | ||||||
|
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. this is |
||||||
| build_queue | ||||||
| .add_crate( | ||||||
| &row.name, | ||||||
| &row.version, | ||||||
| BROKEN_RUSTDOC_REBUILD_PRIORITY, | ||||||
| None, | ||||||
| ) | ||||||
| .await?; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| Ok(results_count) | ||||||
| } | ||||||
|
|
||||||
| #[cfg(test)] | ||||||
| mod tests { | ||||||
| use super::*; | ||||||
| use crate::db::types::BuildStatus; | ||||||
| use crate::test::{FakeBuild, TestEnvironment, V1, V2}; | ||||||
| use chrono::Utc; | ||||||
| use std::time::Duration; | ||||||
|
|
@@ -767,6 +836,171 @@ mod tests { | |||||
| Ok(()) | ||||||
| } | ||||||
|
|
||||||
| /// Verifies whether a rebuild is queued for a crate that previously failed with a nightly version of rustdoc. | ||||||
| #[tokio::test(flavor = "multi_thread")] | ||||||
| async fn test_rebuild_broken_rustdoc_specific_date_simple() -> Result<()> { | ||||||
| let env = TestEnvironment::with_config( | ||||||
| TestEnvironment::base_config() | ||||||
| .max_queued_rebuilds(Some(100)) | ||||||
|
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. good thinking, but not needed in this case. This setting is for our massive "rebuild the world" rebuilder thread, which tries to keep all latest versions on the latest rustdoc version. In our new case here, you're not using the setting to limit your rebuilds, also we don't want to limit them, because they're fixing stuff.
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. ( also not needed in the other tests below) |
||||||
| .build()?, | ||||||
| ) | ||||||
| .await?; | ||||||
|
|
||||||
| for i in 1..5 { | ||||||
| let nightly_date = NaiveDate::from_ymd_opt(2020, 10, i).unwrap(); | ||||||
| env.fake_release() | ||||||
| .await | ||||||
| .name(&format!("foo{}", i)) | ||||||
| .version(V1) | ||||||
| .builds(vec![ | ||||||
| FakeBuild::default() | ||||||
| .rustc_version( | ||||||
| format!( | ||||||
| "rustc 1.84.0-nightly (e7c0d2750 {})", | ||||||
| nightly_date.format("%Y-%m-%d") | ||||||
| ) | ||||||
| .as_str(), | ||||||
| ) | ||||||
| .build_status(BuildStatus::Failure), | ||||||
| ]) | ||||||
| .create() | ||||||
| .await?; | ||||||
| } | ||||||
|
|
||||||
| let build_queue = env.async_build_queue(); | ||||||
| assert!(build_queue.queued_crates().await?.is_empty()); | ||||||
|
|
||||||
| let mut conn = env.async_db().async_conn().await; | ||||||
| queue_rebuilds_faulty_rustdoc( | ||||||
| &mut conn, | ||||||
| build_queue, | ||||||
| &NaiveDate::from_ymd_opt(2020, 10, 3).unwrap(), | ||||||
| &None, | ||||||
| ) | ||||||
| .await?; | ||||||
|
|
||||||
| let queue = build_queue.queued_crates().await?; | ||||||
| assert_eq!(queue.len(), 1); | ||||||
| assert_eq!(queue[0].name, "foo3"); | ||||||
| assert_eq!(queue[0].version, V1); | ||||||
| assert_eq!(queue[0].priority, BROKEN_RUSTDOC_REBUILD_PRIORITY); | ||||||
|
|
||||||
| Ok(()) | ||||||
| } | ||||||
|
|
||||||
| /// Verified whether a rebuild is NOT queued since the latest build for the specific crate is marked as successful. | ||||||
| #[tokio::test(flavor = "multi_thread")] | ||||||
| async fn test_rebuild_broken_rustdoc_specific_date_skipped() -> Result<()> { | ||||||
| let env = TestEnvironment::with_config( | ||||||
| TestEnvironment::base_config() | ||||||
| .max_queued_rebuilds(Some(100)) | ||||||
| .build()?, | ||||||
| ) | ||||||
| .await?; | ||||||
|
|
||||||
| env.fake_release() | ||||||
| .await | ||||||
| .name("foo") | ||||||
| .version(V1) | ||||||
| .builds(vec![ | ||||||
| FakeBuild::default() | ||||||
| .rustc_version( | ||||||
| format!( | ||||||
| "rustc 1.84.0-nightly (e7c0d2750 {})", | ||||||
| NaiveDate::from_ymd_opt(2020, 10, 1) | ||||||
| .unwrap() | ||||||
| .format("%Y-%m-%d") | ||||||
| ) | ||||||
| .as_str(), | ||||||
| ) | ||||||
| .build_status(BuildStatus::Failure), | ||||||
| FakeBuild::default() | ||||||
| .rustc_version( | ||||||
| format!( | ||||||
| "rustc 1.84.0-nightly (e7c0d2750 {})", | ||||||
| NaiveDate::from_ymd_opt(2020, 10, 1) | ||||||
| .unwrap() | ||||||
| .format("%Y-%m-%d") | ||||||
| ) | ||||||
| .as_str(), | ||||||
| ) | ||||||
| .build_status(BuildStatus::Success), | ||||||
| ]) | ||||||
| .create() | ||||||
| .await?; | ||||||
|
|
||||||
| let build_queue = env.async_build_queue(); | ||||||
| assert!(build_queue.queued_crates().await?.is_empty()); | ||||||
|
|
||||||
| let mut conn = env.async_db().async_conn().await; | ||||||
| queue_rebuilds_faulty_rustdoc( | ||||||
| &mut conn, | ||||||
| build_queue, | ||||||
| &NaiveDate::from_ymd_opt(2020, 10, 1).unwrap(), | ||||||
| &None, | ||||||
| ) | ||||||
| .await?; | ||||||
|
|
||||||
| let queue = build_queue.queued_crates().await?; | ||||||
| assert_eq!(queue.len(), 0); | ||||||
|
|
||||||
| Ok(()) | ||||||
| } | ||||||
|
|
||||||
| #[tokio::test(flavor = "multi_thread")] | ||||||
| async fn test_rebuild_broken_rustdoc_date_range() -> Result<()> { | ||||||
| let env = TestEnvironment::with_config( | ||||||
| TestEnvironment::base_config() | ||||||
| .max_queued_rebuilds(Some(100)) | ||||||
| .build()?, | ||||||
| ) | ||||||
| .await?; | ||||||
|
|
||||||
| for i in 1..6 { | ||||||
| let nightly_date = NaiveDate::from_ymd_opt(2020, 10, i).unwrap(); | ||||||
| env.fake_release() | ||||||
| .await | ||||||
| .name(&format!("foo{}", i)) | ||||||
| .version(V1) | ||||||
| .builds(vec![ | ||||||
| FakeBuild::default() | ||||||
| .rustc_version( | ||||||
| format!( | ||||||
| "rustc 1.84.0-nightly (e7c0d2750 {})", | ||||||
| nightly_date.format("%Y-%m-%d") | ||||||
| ) | ||||||
| .as_str(), | ||||||
| ) | ||||||
| .build_status(BuildStatus::Failure), | ||||||
| ]) | ||||||
| .create() | ||||||
| .await?; | ||||||
| } | ||||||
|
|
||||||
| let build_queue = env.async_build_queue(); | ||||||
| assert!(build_queue.queued_crates().await?.is_empty()); | ||||||
|
|
||||||
| let mut conn = env.async_db().async_conn().await; | ||||||
| queue_rebuilds_faulty_rustdoc( | ||||||
| &mut conn, | ||||||
| build_queue, | ||||||
| &NaiveDate::from_ymd_opt(2020, 10, 3).unwrap(), | ||||||
| &NaiveDate::from_ymd_opt(2020, 10, 5), | ||||||
| ) | ||||||
| .await?; | ||||||
|
|
||||||
| let queue = build_queue.queued_crates().await?; | ||||||
| assert_eq!(queue.len(), 2); | ||||||
| assert_eq!(queue[0].name, "foo3"); | ||||||
| assert_eq!(queue[0].version, V1); | ||||||
| assert_eq!(queue[0].priority, BROKEN_RUSTDOC_REBUILD_PRIORITY); | ||||||
| assert_eq!(queue[1].name, "foo4"); | ||||||
| assert_eq!(queue[1].version, V1); | ||||||
| assert_eq!(queue[1].priority, BROKEN_RUSTDOC_REBUILD_PRIORITY); | ||||||
|
|
||||||
| Ok(()) | ||||||
| } | ||||||
|
|
||||||
| #[tokio::test(flavor = "multi_thread")] | ||||||
| async fn test_still_rebuild_when_full_with_failed() -> Result<()> { | ||||||
| let env = TestEnvironment::with_config( | ||||||
|
|
||||||
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 see the behaviour is that when I only give a start date, it's not treated as start, but we only rebuild one nightly.
I feel like this is actually a good default,
but it could be cool if we could make that information part of the docs, perhaps even as help-string for the arg?
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.
wow, just realized that
clapis actually already using the docstring as help-text :D