Merge pull request #1927 from lann/v2-doctor

doctor: Unbreak `spin doctor` for Manifest V2

Author: lann

Cargo.lock

@@ -13,10 +13,18 @@ spin-common = { path = "../common" }
 spin-manifest = { path = "../manifest" }
 tempfile = "3.3.0"
 terminal = { path = "../terminal" }
-tokio = "1"
-toml = "0.7"
-toml_edit = "0.19"
+tokio = { version = "1", features = ["process"] }
+toml = "0.8.2"
+toml_edit = "0.20.2"
 tracing = { workspace = true }
 
 [dev-dependencies]
+glob = "0.3.1"
 tempfile = "3"
+tokio = { version = "1", features = ["macros", "rt"] }
+ui-testing = { path = "../ui-testing" }
+
+[[test]]
+name = "ui"
+path = "tests/ui.rs"
+harness = false

crates/doctor/Cargo.toml

@@ -1,11 +1,10 @@
 //! Spin doctor: check and automatically fix problems with Spin apps.
 #![deny(missing_docs)]
 
-use std::{fmt::Debug, fs, future::Future, path::PathBuf, pin::Pin, sync::Arc};
+use std::{collections::VecDeque, fmt::Debug, fs, path::PathBuf};
 
 use anyhow::{ensure, Context, Result};
 use async_trait::async_trait;
-use tokio::sync::Mutex;
 use toml_edit::Document;
 
 /// Diagnoses for app manifest format problems.
@@ -19,87 +18,98 @@ pub mod wasm;
 
 /// Configuration for an app to be checked for problems.
 pub struct Checkup {
-    manifest_path: PathBuf,
-    diagnostics: Vec<Box<dyn BoxingDiagnostic>>,
+    patient: PatientApp,
+    diagnostics: VecDeque<Box<dyn BoxingDiagnostic>>,
+    unprocessed_diagnoses: VecDeque<Box<dyn Diagnosis>>,
 }
 
 impl Checkup {
     /// Return a new checkup for the app manifest at the given path.
-    pub fn new(manifest_path: impl Into<PathBuf>) -> Self {
+    pub fn new(manifest_path: impl Into<PathBuf>) -> Result<Self> {
+        let patient = PatientApp::new(manifest_path)?;
         let mut checkup = Self {
-            manifest_path: manifest_path.into(),
-            diagnostics: vec![],
+            patient,
+            diagnostics: Default::default(),
+            unprocessed_diagnoses: Default::default(),
         };
-        checkup.add_diagnostic::<manifest::version::VersionDiagnostic>();
-        checkup.add_diagnostic::<manifest::trigger::TriggerDiagnostic>();
-        checkup.add_diagnostic::<rustlang::target::TargetDiagnostic>(); // Do toolchain checks _before_ build checks
-        checkup.add_diagnostic::<wasm::missing::WasmMissingDiagnostic>();
         checkup
+            .add_diagnostic::<manifest::version::VersionDiagnostic>()
+            .add_diagnostic::<manifest::trigger::TriggerDiagnostic>()
+            .add_diagnostic::<rustlang::target::TargetDiagnostic>() // Do toolchain checks _before_ build check
+            .add_diagnostic::<wasm::missing::WasmMissingDiagnostic>();
+        Ok(checkup)
+    }
+
+    /// Returns the [`PatientApp`] being checked.
+    pub fn patient(&self) -> &PatientApp {
+        &self.patient
     }
 
     /// Add a detectable problem to this checkup.
     pub fn add_diagnostic<D: Diagnostic + Default + 'static>(&mut self) -> &mut Self {
-        self.diagnostics.push(Box::<D>::default());
+        self.diagnostics.push_back(Box::<D>::default());
         self
     }
 
-    fn patient(&self) -> Result<PatientApp> {
-        let path = &self.manifest_path;
+    /// Returns the next detected problem.
+    pub async fn next_diagnosis(&mut self) -> Result<Option<PatientDiagnosis>> {
+        while self.unprocessed_diagnoses.is_empty() {
+            let Some(diagnostic) = self.diagnostics.pop_front() else {
+                return Ok(None);
+            };
+            self.unprocessed_diagnoses = diagnostic
+                .diagnose_boxed(&self.patient)
+                .await
+                .unwrap_or_else(|err| {
+                    tracing::debug!("Diagnose failed: {err:?}");
+                    vec![]
+                })
+                .into()
+        }
+        Ok(Some(PatientDiagnosis {
+            patient: &mut self.patient,
+            diagnosis: self.unprocessed_diagnoses.pop_front().unwrap(),
+        }))
+    }
+}
+
+/// An app "patient" to be checked for problems.
+#[derive(Clone)]
+pub struct PatientApp {
+    /// Path to an app manifest file.
+    pub manifest_path: PathBuf,
+    /// Parsed app manifest TOML document.
+    pub manifest_doc: Document,
+}
+
+impl PatientApp {
+    fn new(manifest_path: impl Into<PathBuf>) -> Result<Self> {
+        let path = manifest_path.into();
         ensure!(
             path.is_file(),
             "No Spin app manifest file found at {path:?}"
         );
 
-        let contents = fs::read_to_string(path)
+        let contents = fs::read_to_string(&path)
             .with_context(|| format!("Couldn't read Spin app manifest file at {path:?}"))?;
 
         let manifest_doc: Document = contents
             .parse()
             .with_context(|| format!("Couldn't parse manifest file at {path:?} as valid TOML"))?;
 
-        Ok(PatientApp {
-            manifest_path: path.into(),
+        Ok(Self {
+            manifest_path: path,
             manifest_doc,
         })
     }
-
-    /// Find problems with the configured app, calling the given closure with
-    /// each problem found.
-    pub async fn for_each_diagnosis<F>(&self, mut f: F) -> Result<usize>
-    where
-        F: for<'a> FnMut(
-            Box<dyn Diagnosis + 'static>,
-            &'a mut PatientApp,
-        ) -> Pin<Box<dyn Future<Output = Result<()>> + 'a>>,
-    {
-        let patient = Arc::new(Mutex::new(self.patient()?));
-        let mut count = 0;
-        for diagnostic in &self.diagnostics {
-            let patient = patient.clone();
-            let diags = diagnostic
-                .diagnose_boxed(&*patient.lock().await)
-                .await
-                .unwrap_or_else(|err| {
-                    tracing::debug!("Diagnose failed: {err:?}");
-                    vec![]
-                });
-            count += diags.len();
-            for diag in diags {
-                let mut patient = patient.lock().await;
-                f(diag, &mut patient).await?;
-            }
-        }
-        Ok(count)
-    }
 }
 
-/// An app "patient" to be checked for problems.
-#[derive(Clone)]
-pub struct PatientApp {
-    /// Path to an app manifest file.
-    pub manifest_path: PathBuf,
-    /// Parsed app manifest TOML document.
-    pub manifest_doc: Document,
+/// A PatientDiagnosis bundles a [`Diagnosis`] with its (borrowed) [`PatientApp`].
+pub struct PatientDiagnosis<'a> {
+    /// The diagnosis
+    pub diagnosis: Box<dyn Diagnosis>,
+    /// A reference to the patient this diagnosis applies to
+    pub patient: &'a mut PatientApp,
 }
 
 /// The Diagnose trait implements the detection of a particular Spin app problem.

crates/doctor/src/lib.rs

@@ -18,6 +18,11 @@ impl Diagnostic for TriggerDiagnostic {
     async fn diagnose(&self, patient: &PatientApp) -> Result<Vec<Self::Diagnosis>> {
         let manifest: toml::Value = toml_edit::de::from_document(patient.manifest_doc.clone())?;
 
+        if manifest.get("spin_manifest_version") == Some(&Value::Integer(2)) {
+            // Not applicable to manifest V2
+            return Ok(vec![]);
+        }
+
         let mut diags = vec![];
 
         // Top-level trigger config

crates/doctor/src/manifest/trigger.rs

@@ -24,19 +24,28 @@ impl Diagnostic for VersionDiagnostic {
         let test: VersionProbe =
             from_document(doc.clone()).context("failed to decode VersionProbe")?;
 
-        if let Some(value) = test.spin_manifest_version.or(test.spin_version.clone()) {
-            if value.as_str() != Some("1") {
+        if let Some(value) = test.spin_manifest_version {
+            if corrected_version(&value).is_some() {
                 return Ok(vec![VersionDiagnosis::WrongValue(value)]);
-            } else if test.spin_version.is_some() {
-                return Ok(vec![VersionDiagnosis::OldVersionKey]);
             }
+        } else if test.spin_version.is_some() {
+            return Ok(vec![VersionDiagnosis::OldVersionKey]);
         } else {
             return Ok(vec![VersionDiagnosis::MissingVersion]);
         }
         Ok(vec![])
     }
 }
 
+fn corrected_version(value: &Value) -> Option<toml_edit::Value> {
+    match value {
+        Value::String(s) if s == "1" => None,
+        Value::Integer(2) => None,
+        Value::Integer(1) => Some("1".into()),
+        _ => Some(2.into()),
+    }
+}
+
 #[derive(Debug, Deserialize)]
 struct VersionProbe {
     spin_manifest_version: Option<Value>,
@@ -60,7 +69,7 @@ impl Diagnosis for VersionDiagnosis {
             Self::MissingVersion => "Manifest missing 'spin_manifest_version' key".into(),
             Self::OldVersionKey => "Manifest using old 'spin_version' key".into(),
             Self::WrongValue(val) => {
-                format!(r#"Manifest 'spin_manifest_version' must be "1", not {val}"#)
+                format!(r#"Manifest 'spin_manifest_version' must be "1" or 2, not {val}"#)
             }
         }
     }
@@ -78,16 +87,23 @@ impl Diagnosis for VersionDiagnosis {
 impl ManifestTreatment for VersionDiagnosis {
     fn summary(&self) -> String {
         match self {
-            Self::MissingVersion => "Add spin_manifest_version to manifest",
-            Self::OldVersionKey => "Replace 'spin_version' with 'spin_manifest_version'",
-            Self::WrongValue(_) => r#"Set manifest version to "1""#,
+            Self::MissingVersion => "Add spin_manifest_version to manifest".into(),
+            Self::OldVersionKey => "Replace 'spin_version' with 'spin_manifest_version'".into(),
+            Self::WrongValue(value) => format!(
+                "Set manifest version to {}",
+                corrected_version(value).unwrap()
+            ),
         }
-        .into()
     }
 
     async fn treat_manifest(&self, doc: &mut Document) -> anyhow::Result<()> {
         doc.remove(SPIN_VERSION);
-        let item = Item::Value("1".into());
+
+        let item = Item::Value(match self {
+            Self::MissingVersion => 2.into(),
+            Self::OldVersionKey => "1".into(),
+            Self::WrongValue(value) => corrected_version(value).unwrap(),
+        });
         if let Some(existing) = doc.get_mut(SPIN_MANIFEST_VERSION) {
             *existing = item;
         } else {
@@ -115,12 +131,6 @@ mod tests {
         run_correct_test::<VersionDiagnostic>("manifest_version").await;
     }
 
-    #[tokio::test]
-    async fn test_missing() {
-        let diag = run_broken_test::<VersionDiagnostic>("manifest_version", "missing_key").await;
-        assert!(matches!(diag, VersionDiagnosis::MissingVersion));
-    }
-
     #[tokio::test]
     async fn test_old_key() {
         let diag = run_broken_test::<VersionDiagnostic>("manifest_version", "old_key").await;

crates/doctor/src/manifest/version.rs

@@ -68,7 +68,7 @@ pub struct TestPatient {
 
 impl TestPatient {
     fn new(manifest_temp: TempPath) -> Result<Self> {
-        let inner = Checkup::new(&manifest_temp).patient()?;
+        let inner = PatientApp::new(&manifest_temp)?;
         Ok(Self {
             inner,
             _manifest_temp: manifest_temp,

crates/doctor/src/test.rs

@@ -1,4 +1,4 @@
-spin_manifest_version = 2
+spin_manifest_version = 1
 
 # comment preserved
 name = "app-name"