Updated after initial review

Signed-off-by: itowlson <ivan.towlson@fermyon.com>

Author: itowlson

Cargo.lock

@@ -21,42 +21,59 @@ pub async fn build(
     component_ids: &[String],
     skip_target_checks: bool,
 ) -> Result<()> {
-    let (components, deployment_targets, manifest) = component_build_configs(manifest_file)
+    let build_info = component_build_configs(manifest_file)
         .await
         .with_context(|| {
-        format!(
-            "Cannot read manifest file from {}",
-            quoted_path(manifest_file)
-        )
-    })?;
+            format!(
+                "Cannot read manifest file from {}",
+                quoted_path(manifest_file)
+            )
+        })?;
     let app_dir = parent_dir(manifest_file)?;
 
-    let build_result = build_components(component_ids, components, app_dir);
+    let build_result = build_components(component_ids, build_info.components(), app_dir);
 
-    if let Err(e) = &manifest {
+    // Emit any required warnings now, so that they don't bury any errors.
+    if let Some(e) = build_info.load_error() {
+        // The manifest had errors. We managed to attempt a build anyway, but we want to
+        // let the user know about them.
         terminal::warn!("The manifest has errors not related to the Wasm component build. Error details:\n{e:#}");
+        // Checking deployment targets requires a healthy manifest (because trigger types etc.),
+        // if any of these were specified, warn they are being skipped.
+        let should_have_checked_targets =
+            !skip_target_checks && build_info.has_deployment_targets();
+        if should_have_checked_targets {
+            terminal::warn!(
+                "The manifest error(s) prevented Spin from checking the deployment targets."
+            );
+        }
     }
 
+    // If the build failed, exit with an error at this point.
     build_result?;
 
-    if let Ok(manifest) = &manifest {
-        if !skip_target_checks {
-            let resolution_context =
-                spin_environments::ResolutionContext::new(manifest_file.parent().unwrap()).await?;
-            let errors = spin_environments::validate_application_against_environment_ids(
-                &deployment_targets,
-                manifest,
-                &resolution_context,
-            )
-            .await?;
+    let Some(manifest) = build_info.manifest() else {
+        // We can't proceed to checking (because that needs a full healthy manifest), and we've
+        // already emitted any necessary warning, so quit.
+        return Ok(());
+    };
 
-            for error in &errors {
-                terminal::error!("{error}");
-            }
+    if !skip_target_checks {
+        let resolution_context =
+            spin_environments::ResolutionContext::new(manifest_file.parent().unwrap()).await?;
+        let errors = spin_environments::validate_application_against_environment_ids(
+            build_info.deployment_targets(),
+            manifest,
+            &resolution_context,
+        )
+        .await?;
 
-            if !errors.is_empty() {
-                anyhow::bail!("All components built successfully, but one or more was incompatible with one or more of the deployment targets.");
-            }
+        for error in &errors {
+            terminal::error!("{error}");
+        }
+
+        if !errors.is_empty() {
+            anyhow::bail!("All components built successfully, but one or more was incompatible with one or more of the deployment targets.");
         }
     }
 

crates/build/src/lib.rs

@@ -4,32 +4,96 @@ use std::{collections::BTreeMap, path::Path};
 
 use spin_manifest::{schema::v2, ManifestVersion};
 
-pub type DeploymentTarget = String;
-pub type DeploymentTargets = Vec<DeploymentTarget>;
+pub enum ManifestBuildInfo {
+    Loadable {
+        components: Vec<ComponentBuildInfo>,
+        deployment_targets: Vec<String>,
+        manifest: spin_manifest::schema::v2::AppManifest,
+    },
+    Unloadable {
+        components: Vec<ComponentBuildInfo>,
+        has_deployment_targets: bool,
+        load_error: spin_manifest::Error,
+    },
+}
+
+impl ManifestBuildInfo {
+    pub fn components(&self) -> Vec<ComponentBuildInfo> {
+        match self {
+            Self::Loadable { components, .. } => components.clone(),
+            Self::Unloadable { components, .. } => components.clone(),
+        }
+    }
+
+    pub fn load_error(&self) -> Option<&spin_manifest::Error> {
+        match self {
+            Self::Loadable { .. } => None,
+            Self::Unloadable { load_error, .. } => Some(load_error),
+        }
+    }
+
+    pub fn deployment_targets(&self) -> &[String] {
+        match self {
+            Self::Loadable {
+                deployment_targets, ..
+            } => deployment_targets,
+            Self::Unloadable { .. } => &[],
+        }
+    }
+
+    pub fn has_deployment_targets(&self) -> bool {
+        match self {
+            Self::Loadable {
+                deployment_targets, ..
+            } => !deployment_targets.is_empty(),
+            Self::Unloadable {
+                has_deployment_targets,
+                ..
+            } => *has_deployment_targets,
+        }
+    }
+
+    pub fn manifest(&self) -> Option<&spin_manifest::schema::v2::AppManifest> {
+        match self {
+            Self::Loadable { manifest, .. } => Some(manifest),
+            Self::Unloadable { .. } => None,
+        }
+    }
+}
 
 /// Returns a map of component IDs to [`v2::ComponentBuildConfig`]s for the
 /// given (v1 or v2) manifest path. If the manifest cannot be loaded, the
 /// function attempts fallback: if fallback succeeds, result is Ok but the load error
 /// is also returned via the second part of the return value tuple.
-pub async fn component_build_configs(
-    manifest_file: impl AsRef<Path>,
-) -> Result<(
-    Vec<ComponentBuildInfo>,
-    DeploymentTargets,
-    Result<spin_manifest::schema::v2::AppManifest, spin_manifest::Error>,
-)> {
+pub async fn component_build_configs(manifest_file: impl AsRef<Path>) -> Result<ManifestBuildInfo> {
     let manifest = spin_manifest::manifest_from_file(&manifest_file);
     match manifest {
         Ok(mut manifest) => {
             spin_manifest::normalize::normalize_manifest(&mut manifest);
-            let bc = build_configs_from_manifest(&manifest);
-            let dt = deployment_targets_from_manifest(&manifest);
-            Ok((bc, dt, Ok(manifest)))
+            let components = build_configs_from_manifest(&manifest);
+            let deployment_targets = deployment_targets_from_manifest(&manifest);
+            Ok(ManifestBuildInfo::Loadable {
+                components,
+                deployment_targets,
+                manifest,
+            })
         }
-        Err(e) => {
-            let bc = fallback_load_build_configs(&manifest_file).await?;
-            let dt = fallback_load_deployment_targets(&manifest_file).await?;
-            Ok((bc, dt, Err(e)))
+        Err(load_error) => {
+            // The manifest didn't load, but the problem might not be build-affecting.
+            // Try to fall back by parsing out only the bits we need. And if something
+            // goes wrong with the fallback, give up and return the original manifest load
+            // error.
+            let Ok(components) = fallback_load_build_configs(&manifest_file).await else {
+                return Err(load_error.into());
+            };
+            let Ok(has_deployment_targets) = has_deployment_targets(&manifest_file).await else {
+                return Err(load_error.into());
+            };
+            Ok(ManifestBuildInfo::Unloadable {
+                components,
+                has_deployment_targets,
+                load_error,
+            })
         }
     }
 }
@@ -49,7 +113,7 @@ fn build_configs_from_manifest(
 
 fn deployment_targets_from_manifest(
     manifest: &spin_manifest::schema::v2::AppManifest,
-) -> DeploymentTargets {
+) -> Vec<String> {
     manifest.application.targets.clone()
 }
 
@@ -75,31 +139,23 @@ async fn fallback_load_build_configs(
     })
 }
 
-async fn fallback_load_deployment_targets(
-    manifest_file: impl AsRef<Path>,
-) -> Result<DeploymentTargets> {
+async fn has_deployment_targets(manifest_file: impl AsRef<Path>) -> Result<bool> {
     let manifest_text = tokio::fs::read_to_string(manifest_file).await?;
     Ok(match ManifestVersion::detect(&manifest_text)? {
-        ManifestVersion::V1 => Default::default(),
+        ManifestVersion::V1 => false,
         ManifestVersion::V2 => {
             let table: toml::value::Table = toml::from_str(&manifest_text)?;
-            let target_environments = table
+            table
                 .get("application")
                 .and_then(|a| a.as_table())
                 .and_then(|t| t.get("targets"))
                 .and_then(|arr| arr.as_array())
-                .map(|v| v.as_slice())
-                .unwrap_or_default()
-                .iter()
-                .filter_map(|t| t.as_str())
-                .map(|s| s.to_owned())
-                .collect();
-            target_environments
+                .is_some_and(|arr| !arr.is_empty())
         }
     })
 }
 
-#[derive(Deserialize)]
+#[derive(Clone, Deserialize)]
 pub struct ComponentBuildInfo {
     #[serde(default)]
     pub id: String,

crates/build/src/manifest.rs

@@ -33,6 +33,8 @@ pub async fn compose<'a, L: ComponentSourceLoader>(
     Composer::new(loader).compose(component).await
 }
 
+/// A Spin component dependency. This abstracts over the metadata associated with the
+/// dependency. The abstraction allows both manifest and lockfile types to participate in composition.
 #[async_trait::async_trait]
 pub trait DependencyLike {
     fn inherit(&self) -> InheritConfiguration;
@@ -44,6 +46,8 @@ pub enum InheritConfiguration {
     Some(Vec<String>),
 }
 
+/// A Spin component. This abstracts over the list of dependencies for the component.
+/// The abstraction allows both manifest and lockfile types to participate in composition.
 #[async_trait::async_trait]
 pub trait ComponentLike {
     type Dependency: DependencyLike;

crates/compose/src/lib.rs

@@ -11,12 +11,11 @@ bytes = "1.1"
 futures = "0.3"
 futures-util = "0.3"
 id-arena = "2"
-indexmap = "2.2.6"
-miette = "7.2.0"
+indexmap = "2"
 oci-distribution = { git = "https://github.com/fermyon/oci-distribution", rev = "7e4ce9be9bcd22e78a28f06204931f10c44402ba" }
-semver = "1.0"
-serde = { version = "1.0", features = ["derive"] }
-serde_json = "1.0"
+semver = "1"
+serde = { version = "1", features = ["derive"] }
+serde_json = "1"
 spin-common = { path = "../common" }
 spin-componentize = { path = "../componentize" }
 spin-compose = { path = "../compose" }

crates/environments/Cargo.toml

@@ -1,5 +1,6 @@
 use anyhow::Context;
 
+/// Loads the given `TargetEnvironment` from a registry.
 pub async fn load_environment(env_id: impl AsRef<str>) -> anyhow::Result<TargetEnvironment> {
     use futures_util::TryStreamExt;
 
@@ -37,10 +38,20 @@ pub async fn load_environment(env_id: impl AsRef<str>) -> anyhow::Result<TargetE
     TargetEnvironment::new(env_id.to_owned(), bytes)
 }
 
+/// A parsed document representing a deployment environment, e.g. Spin 2.7,
+/// SpinKube 3.1, Fermyon Cloud. The `TargetEnvironment` provides a mapping
+/// from the Spin trigger types supported in the environment to the Component Model worlds
+/// supported by that trigger type. (A trigger type may support more than one world,
+/// for example when it supports multiple versions of the Spin or WASI interfaces.)
+///
+/// In terms of implementation, internally the environment is represented by a
+/// WIT package that adheres to a specific naming convention (that the worlds for
+/// a given trigger type are exactly whose names begin `trigger-xxx` where
+/// `xxx` is the Spin trigger type).
 pub struct TargetEnvironment {
     name: String,
     decoded: wit_parser::decoding::DecodedWasm,
-    package: wit_parser::Package, // saves unwrapping it every time
+    package: wit_parser::Package,
     package_id: id_arena::Id<wit_parser::Package>,
     package_bytes: Vec<u8>,
 }
@@ -68,11 +79,14 @@ impl TargetEnvironment {
         })
     }
 
+    /// Returns true if the given trigger type provides the world identified by
+    /// `world` in this environment.
     pub fn is_world_for(&self, trigger_type: &TriggerType, world: &wit_parser::World) -> bool {
         world.name.starts_with(&format!("trigger-{trigger_type}"))
             && world.package.is_some_and(|p| p == self.package_id)
     }
 
+    /// Returns true if the given trigger type can run in this environment.
     pub fn supports_trigger_type(&self, trigger_type: &TriggerType) -> bool {
         self.decoded
             .resolve()
@@ -81,6 +95,7 @@ impl TargetEnvironment {
             .any(|(_, world)| self.is_world_for(trigger_type, world))
     }
 
+    /// Lists all worlds supported for the given trigger type in this environment.
     pub fn worlds(&self, trigger_type: &TriggerType) -> Vec<String> {
         self.decoded
             .resolve()
@@ -93,10 +108,10 @@ impl TargetEnvironment {
 
     /// Fully qualified world name (e.g. fermyon:spin/http-trigger@2.0.0)
     fn world_qname(&self, world: &wit_parser::World) -> String {
-        let version_suffix = match self.package_version() {
-            Some(version) => format!("@{version}"),
-            None => "".to_owned(),
-        };
+        let version_suffix = self
+            .package_version()
+            .map(|version| format!("@{version}"))
+            .unwrap_or_default();
         format!(
             "{}/{}{version_suffix}",
             self.package_namespaced_name(),
@@ -114,10 +129,12 @@ impl TargetEnvironment {
         format!("{}:{}", self.package.name.namespace, self.package.name.name)
     }
 
+    /// The package version for the environment package.
     pub fn package_version(&self) -> Option<&semver::Version> {
         self.package.name.version.as_ref()
     }
 
+    /// The Wasm-encoded bytes of the environment package.
     pub fn package_bytes(&self) -> &[u8] {
         &self.package_bytes
     }