Merge spin locked app into spin app

[seun-ja]

Resolves #3145

Merges spin-locked-app into spin-app.

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

Also, Error's variants were changed because of clippy warnings. They shouldn't end with Error

[itowlson]

Intentional that tests are being removed?

[seun-ja]

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

[itowlson]

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?

[seun-ja]

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

[itowlson]

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.

[itowlson]

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?

[seun-ja]

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...

[itowlson]

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)