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)