Skip to content

Merge spin locked app into spin app #3231

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Merge spin locked app into spin app #3231

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
80238be
WIP: merge
seun-ja Aug 13, 2025
ffc2b3f
Fix cyclic dependency issue in test
seun-ja Aug 14, 2025
decf0cc
move test to loader
seun-ja Aug 20, 2025
751ce5d
test in app
seun-ja Aug 21, 2025
05d8d77
improved
seun-ja Aug 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 13 additions & 24 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ spin-doctor = { path = "crates/doctor" }
spin-factor-outbound-networking = { path = "crates/factor-outbound-networking" }
spin-http = { path = "crates/http" }
spin-loader = { path = "crates/loader" }
spin-locked-app = { path = "crates/locked-app" }
spin-manifest = { path = "crates/manifest" }
spin-oci = { path = "crates/oci" }
spin-plugins = { path = "crates/plugins" }
Expand Down
8 changes: 5 additions & 3 deletions crates/app/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@ edition = { workspace = true }

[dependencies]
anyhow = { workspace = true }
itertools = { workspace = true }
serde = { workspace = true }
serde_json = { workspace = true }
spin-locked-app = { path = "../locked-app" }
spin-serde = { path = "../serde" }
thiserror = { workspace = true }

[dev-dependencies]
spin-manifest = { path = "../manifest" }
toml = { workspace = true }
spin-factors-test = { path = "../factors-test" }
tokio = { workspace = true }
tokio = { workspace = true }
25 changes: 25 additions & 0 deletions crates/app/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/// Type alias for a [`Result`]s with [`Error`].
pub type Result<T> = std::result::Result<T, Error>;

/// Errors returned by methods in this crate.
#[derive(Debug, thiserror::Error)]
pub enum Error {
/// An error propagated from the `spin_core` crate.
#[error(transparent)]
Core(anyhow::Error),
/// An error from a `DynamicHostComponent`.
#[error("host component error: {0:#}")]
HostComponent(#[source] anyhow::Error),
/// An error from a `Loader` implementation.
#[error(transparent)]
Loader(anyhow::Error),
/// An error indicating missing or unexpected metadata.
#[error("metadata error: {0}")]
Metadata(String),
/// An error indicating failed JSON (de)serialization.
#[error("json error: {0}")]
Json(#[from] serde_json::Error),
/// A validation error that can be presented directly to the user.
#[error(transparent)]
Validation(anyhow::Error),
}
108 changes: 83 additions & 25 deletions crates/app/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,22 @@ use std::sync::Arc;

use serde::Deserialize;
use serde_json::Value;
use spin_locked_app::MetadataExt;

use locked::{ContentPath, LockedApp, LockedComponent, LockedComponentSource, LockedTrigger};
use crate::locked::{
ContentPath, LockedApp, LockedComponent, LockedComponentSource, LockedTrigger,
};

pub use spin_locked_app::locked;
pub use spin_locked_app::values;
pub use spin_locked_app::{Error, MetadataKey, Result};
pub use crate::locked::Variable;

pub use locked::Variable;
use crate::error::{Error, Result};
use crate::metadata::MetadataExt as _;

mod error;
pub mod locked;
mod metadata;
pub mod values;

pub use metadata::MetadataKey;

/// MetadataKey for extracting the application name.
pub const APP_NAME_KEY: MetadataKey = MetadataKey::new("name");
Expand Down Expand Up @@ -121,7 +128,7 @@ impl App {
return Ok(None);
};
let metadata = T::deserialize(value).map_err(|err| {
Error::MetadataError(format!(
Error::Metadata(format!(
"invalid metadata value for {trigger_type:?}: {err:?}"
))
})?;
Expand Down Expand Up @@ -186,7 +193,7 @@ impl App {
) -> Result<LockedApp> {
self.validate_retained_components_exist(retained_components)?;
for validator in validators {
validator(&self, retained_components).map_err(Error::ValidationError)?;
validator(&self, retained_components).map_err(Error::Validation)?;
}
let (component_ids, trigger_ids): (HashSet<String>, HashSet<String>) = self
.triggers()
Expand All @@ -211,7 +218,7 @@ impl App {
.collect::<HashSet<_>>();
for c in retained_components {
if !app_components.contains(*c) {
return Err(Error::ValidationError(anyhow::anyhow!(
return Err(Error::Validation(anyhow::anyhow!(
"Specified component \"{c}\" not found in application"
)));
}
Expand Down Expand Up @@ -310,10 +317,10 @@ impl<'a> AppTrigger<'a> {
let id = &self.locked.id;
let common_config: CommonTriggerConfig = self.typed_config()?;
let component_id = common_config.component.ok_or_else(|| {
Error::MetadataError(format!("trigger {id:?} missing 'component' config field"))
Error::Metadata(format!("trigger {id:?} missing 'component' config field"))
})?;
self.app.get_component(&component_id).ok_or_else(|| {
Error::MetadataError(format!(
Error::Metadata(format!(
"missing component {component_id:?} configured for trigger {id:?}"
))
})
Expand All @@ -337,29 +344,80 @@ pub fn retain_components(

#[cfg(test)]
mod test {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional that tests are being removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was moved into the spin-factor-test crate to avoid the cyclic dependency issue the compiler frowns at.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

factors-test seems like a rather strange place for it - I thought retaining components was a pure LockedApp operation whereas factors-test seems to be more about instantiation and runtime configuration. What was the cyclic dependency that you ran into?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test inside the module test_retain_components_filtering_for_only_component_works calls build_locked_app from spin-factor-test, which returns LockedApp.

LockedApp previously lived inside spin-locked_app; hence, both spin-app and spin-factor-test depend on spin-locked-app, no conflict.

With the merge, LockedApp lives inside spin-app, that means spin-factor-test, which hosts build_locked_app, would have to depend on spin-app to import LockedApp (which the function returns alongside other functions), while spin-app depends on spin-factor-test for build_locked_app

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like the tail wagging the dog. build_locked_app is a tiny ancillary function that basically wraps spin_loader::from_file. I don't think we should be locating tests based on where we happen to have existing helpers - I'd strongly prefer to keep the test with the thing it's testing and figure out how to support it in that location.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you've moved the retain_components test to spin_loader, which still seems like you're letting the ancillary build_locked_app control where you put the test. The test should go alongside retain_components and then you should figure out what supporting infrastructure it needs in that location. If that involves reworking or jettisoning build_locked_app then so be it - build_locked_app is not the important thing in the test and should not be driving the design.

Looking at existing code, I'm a bit puzzled at why the compiler is getting mad now anyway. The app crate already has a dev-dependency on factors-test, and factors-test already has a dependency on app. Is it because factors-test really only depended on the LockedApp re-export and the compiler realised it wasn't a true circle?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think because of the cyclic issue even though spin-app has spin-factors-test as a dev-dependence, the compiler keeps having issues knowing which LockedApp retain_components returns.

the crate `spin_app` is compiled multiple times, possibly with different configurations

Full error:

error[E0308]: mismatched types
   --> crates/app/src/lib.rs:373:40
    |
373 |         locked_app = retain_components(locked_app, &["empty"], &[&does_nothing_validator]).unwrap();
    |                      ----------------- ^^^^^^^^^^ expected `locked::LockedApp`, found `spin_app::locked::LockedApp`
    |                      |
    |                      arguments to this function are incorrect
    |
note: the crate `spin_app` is compiled multiple times, possibly with different configurations
   --> crates/app/src/locked.rs:46:1
    |
46  | pub struct LockedApp {
    | ^^^^^^^^^^^^^^^^^^^^ this is the expected type `locked::LockedApp`
    |
   ::: /Users/seunaminu/Documents/GitHub/spin/crates/app/src/locked.rs:46:1
    |
46  | pub struct LockedApp {
    | ^^^^^^^^^^^^^^^^^^^^ this is the found type `spin_app::locked::LockedApp`
    |
   ::: crates/app/src/lib.rs:350:9
    |
350 |     use spin_factors_test::build_locked_app;
    |         ----------------- one version of crate `spin_app` used here, as a dependency of crate `spin_factors_test`
    = help: you can use `cargo tree` to explore your dependency tree
note: function defined here
   --> crates/app/src/lib.rs:338:8
    |
338 | pub fn retain_components(
    |        ^^^^^^^^^^^^^^^^^
339 |     locked: LockedApp,
    |     -----------------

error[E0308]: mismatched types
   --> crates/app/src/lib.rs:373:22
    |
372 |         let mut locked_app = build_locked_app(&manifest).await.unwrap();
    |                              ------------------------------------------ expected due to this value
373 |         locked_app = retain_components(locked_app, &["empty"], &[&does_nothing_validator]).unwrap();
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `spin_app::locked::LockedApp`, found `locked::LockedApp`
    |
note: the crate `spin_app` is compiled multiple times, possibly with different configurations
    |
   ::: /Users/seunaminu/Documents/GitHub/spin/crates/app/src/locked.rs:46:1
    |
46  | pub struct LockedApp {
    | ^^^^^^^^^^^^^^^^^^^^ this is the expected type `spin_app::locked::LockedApp`
   --> crates/app/src/locked.rs:46:1
    |
46  | pub struct LockedApp {
    | ^^^^^^^^^^^^^^^^^^^^ this is the found type `locked::LockedApp`
    |
   ::: crates/app/src/lib.rs:350:9
    |
350 |     use spin_factors_test::build_locked_app;
    |         ----------------- one version of crate `spin_app` used here, as a dependency of crate `spin_factors_test`
    = help: you can use `cargo tree` to explore your dependency tree

For more information about this error, try `rustc --explain E0308`.
error: could not compile `spin-app` (lib test) due to 2 previous errors
warning: build failed, waiting for other jobs to finish...

use spin_factors_test::build_locked_app;
use std::collections::HashSet;

use spin_manifest::schema::v2::{self, ComponentSpec, Trigger};

use super::*;

fn locked_trigger(
trigger_type: String,
mut trigger: v2::Trigger,
) -> anyhow::Result<LockedTrigger> {
fn reference_id(spec: v2::ComponentSpec) -> toml::Value {
let v2::ComponentSpec::Reference(id) = spec else {
unreachable!("should have already been normalized");
};
id.as_ref().into()
}

if let Some(id) = trigger.component.map(reference_id) {
trigger.config.insert("component".into(), id);
}

Ok(LockedTrigger {
id: trigger.id,
trigger_type,
trigger_config: trigger.config.try_into()?,
})
}

pub fn locked_app() -> LockedApp {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - I think this is (unfortunately) the least worst option and I appreciate you persevering with it.

I do think you can simplify the implementation a bit, e.g.:

  • The triggers collection can just be a variable rather than needing a whole function and a spin-manifest dependency:
vec![LockedTrigger {
    id: "trigger".into(),
    trigger_type: "dummy".into(),
    trigger_config: toml::from_str(r#"component = "empty""#).unwrap(),
}];

I think this is terser and clearer - spin-manifest has a lot of twiddly stuff that is useful for dev flexibility but causes only noise here.

  • LockedComponent.source.content can be Default::default() (again, the loader cares but retain_components doesn't)

LockedApp {
spin_lock_version: Default::default(),
must_understand: Default::default(),
metadata: Default::default(),
host_requirements: Default::default(),
variables: Default::default(),
triggers: vec![locked_trigger(
"http".to_owned(),
Trigger {
id: Default::default(),
component: Some(ComponentSpec::Reference(
"empty".to_string().try_into().unwrap(),
)),
components: Default::default(),
config: Default::default(),
},
)
.unwrap()],
components: vec![LockedComponent {
id: "empty".to_owned(),
metadata: Default::default(),
source: LockedComponentSource {
content_type: "application/wasm".to_owned(),
content: locked::ContentRef {
source: Some("does-not-exist.wasm".to_owned()),
inline: None,
digest: None,
},
},
env: Default::default(),
files: Default::default(),
config: Default::default(),
dependencies: Default::default(),
host_requirements: Default::default(),
}],
}
}

fn does_nothing_validator(_: &App, _: &[&str]) -> anyhow::Result<()> {
Ok(())
}

#[tokio::test]
async fn test_retain_components_filtering_for_only_component_works() {
let manifest = toml::toml! {
spin_manifest_version = 2

[application]
name = "test-app"

[[trigger.test-trigger]]
component = "empty"

[component.empty]
source = "does-not-exist.wasm"
};
let mut locked_app = build_locked_app(&manifest).await.unwrap();
let mut locked_app = locked_app();
locked_app = retain_components(locked_app, &["empty"], &[&does_nothing_validator]).unwrap();
let components = locked_app
.components
Expand Down
0 crates/locked-app/src/locked.rs → crates/app/src/locked.rs
View file
Open in desktop
File renamed without changes.
6 changes: 2 additions & 4 deletions crates/locked-app/src/metadata.rs → crates/app/src/metadata.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,16 +57,14 @@ pub trait MetadataExt {
self.get_value(key.as_ref())
.map(T::deserialize)
.transpose()
.map_err(|err| {
Error::MetadataError(format!("invalid metadata value for {key:?}: {err:?}"))
})
.map_err(|err| Error::Metadata(format!("invalid metadata value for {key:?}: {err:?}")))
}

/// Get a required value from a metadata map, returning an error
/// if it is not present
fn require_typed<'a, T: Deserialize<'a>>(&'a self, key: MetadataKey<T>) -> Result<T> {
self.get_typed(key)?
.ok_or_else(|| Error::MetadataError(format!("missing required metadata {key:?}")))
.ok_or_else(|| Error::Metadata(format!("missing required metadata {key:?}")))
}
}

Expand Down
0 crates/locked-app/src/values.rs → crates/app/src/values.rs
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion crates/core/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ spin-componentize = { workspace = true }
spin-factor-wasi = { path = "../factor-wasi" }
spin-factors = { path = "../factors" }
spin-factors-test = { path = "../factors-test" }
spin-locked-app = { path = "../locked-app" }
spin-app = { path = "../app" }
tokio = { workspace = true, features = ["macros", "rt", "rt-multi-thread"] }
wasmtime-wasi = { workspace = true }

Expand Down
Loading
Loading