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 2 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
36 changes: 12 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
5 changes: 3 additions & 2 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,12 @@ 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]
toml = { workspace = true }
spin-factors-test = { path = "../factors-test" }
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),
}
65 changes: 18 additions & 47 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 @@ -334,39 +341,3 @@ pub fn retain_components(
) -> Result<LockedApp> {
App::new("unused", locked).retain_components(components, validators)
}

#[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 super::*;

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();
locked_app = retain_components(locked_app, &["empty"], &[&does_nothing_validator]).unwrap();
let components = locked_app
.components
.iter()
.map(|c| c.id.to_string())
.collect::<HashSet<_>>();
assert!(components.contains("empty"));
assert!(components.len() == 1);
}
}
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
2 changes: 1 addition & 1 deletion crates/core/tests/integration_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ use std::{

use anyhow::Context;
use serde_json::json;
use spin_app::locked::LockedApp;
use spin_core::{AsState, Component, Config, Engine, State, Store, StoreBuilder, Trap};
use spin_factor_wasi::{DummyFilesMounter, WasiFactor};
use spin_factors::{App, AsInstanceState, RuntimeFactors};
use spin_locked_app::locked::LockedApp;
use tokio::{fs, io::AsyncWrite};
use wasmtime_wasi::I32Exit;

Expand Down
2 changes: 1 addition & 1 deletion crates/expressions/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ edition = { workspace = true }
anyhow = { workspace = true }
async-trait = { workspace = true }
futures = { workspace = true }
spin-locked-app = { path = "../locked-app" }
spin-app = { path = "../app" }
thiserror = { workspace = true }

[dev-dependencies]
Expand Down
2 changes: 1 addition & 1 deletion crates/expressions/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ mod template;

use std::{borrow::Cow, collections::HashMap, fmt::Debug};

use spin_locked_app::Variable;
use spin_app::Variable;

pub use async_trait;

Expand Down
2 changes: 1 addition & 1 deletion crates/factor-key-value/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ anyhow = { workspace = true }
serde = { workspace = true }
spin-core = { path = "../core" }
spin-factors = { path = "../factors" }
spin-locked-app = { path = "../locked-app" }
spin-app = { path = "../app" }
spin-resource-table = { path = "../table" }
spin-telemetry = { path = "../telemetry" }
spin-world = { path = "../world" }
Expand Down
2 changes: 1 addition & 1 deletion crates/factor-key-value/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ use std::{
};

use anyhow::ensure;
use spin_app::MetadataKey;
use spin_factors::{
ConfigureAppContext, Factor, FactorData, FactorInstanceBuilder, InitContext, PrepareContext,
RuntimeFactors,
};
use spin_locked_app::MetadataKey;

/// Metadata key for key-value stores.
pub const KEY_VALUE_STORES_KEY: MetadataKey<Vec<String>> = MetadataKey::new("key_value_stores");
Expand Down
2 changes: 1 addition & 1 deletion crates/factor-llm/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ serde = { workspace = true }
spin-factors = { path = "../factors" }
spin-llm-local = { path = "../llm-local", optional = true }
spin-llm-remote-http = { path = "../llm-remote-http" }
spin-locked-app = { path = "../locked-app" }
spin-app = { path = "../app" }
spin-telemetry = { path = "../telemetry" }
spin-world = { path = "../world" }
tokio = { workspace = true, features = ["sync"] }
Expand Down
Loading
Loading