Support building only specific components by itowlson · Pull Request #1515 · spinframework/spin

itowlson · 2023-05-18T01:56:16Z

spin doctor is set up to build only the component that the user asks it to fix, but the spin build side of that was not ready in time. Now it is.

# Happy path
ivan@hecate:~/testing/kvmadness$ spin build -c kvmadness
Executing the build command for component kvmadness: cargo build --target wasm32-wasi --release
    Finished release [optimized] target(s) in 0.03s
Successfully ran the build command for component 'kvmadness'.

# No-op path
ivan@hecate:~/testing/kvmadness$ spin build -c moarmadness
Component 'moarmadness' does not have a build command.
For information on specifying a build command, see https://developer.fermyon.com/spin/build#setting-up-for-spin-build.

# Sad path
$ spin build -c moarmadness2
Error: There is no component 'moarmadness2' in the manifest.

Closes #903 [lann: should cover the use case]

lann · 2023-05-18T12:42:34Z

crates/build/src/lib.rs

+                    eprintln!("Component '{id}' does not have a build command.");
+                    eprintln!("For information on specifying a build command, see https://developer.fermyon.com/spin/build#setting-up-for-spin-build.");


Should this return an error result? That would signal to doctor that the fix didn't work, and might be helpful for other kinds of automation.

The doctor should never call this for components without a build command - it checks before recommending the "build" treatment.

Yeah I dithered about the error result. I think I kind of reached the conclusion that it hadn't failed, there was just nothing to do. And so I was a bit wary about Rust printing Error: ... which we can't currently stop it doing. But if other folks would prefer an error than I'm happy to go along with that.

Looking at the existing flow, "no components have a build command" is currently not an error (and prints to stdout rather than stderr so I am being inconsistent there at least).

I am reworking it as you suggested to take a list of components so this will unify it to the existing flow. I will use that existing flow for now; we can change it to be an error if we want.

lann · 2023-05-18T12:43:36Z

Ah yes, I purposefully left this unimplemented to....drive developer engagement?

lann · 2023-05-18T14:01:51Z

src/commands/build.rs

    pub async fn run(self) -> Result<()> {
        let manifest_file = crate::manifest::resolve_file_path(&self.app_source)?;
-        spin_build::build(&manifest_file).await?;
+        spin_build::build(&manifest_file, &self.component_id).await?;


Would it make sense for component_id to be a Vec<String> (allowing the flag to be specified multiple times)? I'm thinking that would give feature parity with the zombie PR #903

It couldn't possibly do any harm, he said, cheerily placing his Jenga block.

itowlson · 2023-05-18T23:17:48Z

Updated to allow multiple use, resulting in a slight change to some cases:

ivan@hecate:~/testing/kvmadness$ spin build -c moarmadness
None of the components have a build command.
For information on specifying a build command, see https://developer.fermyon.com/spin/build#setting-up-for-spin-build.
ivan@hecate:~/testing/kvmadness$ spin build -c moarmadness -c fie
Error: Unknown component(s) fie
ivan@hecate:~/testing/kvmadness$ spin build -c moarmadness -c fie -c zounds
Error: Unknown component(s) fie, zounds

rylev · 2023-05-19T12:27:50Z

crates/build/src/lib.rs

+        let unknown_component_ids: Vec<_> = component_ids
+            .iter()
+            .filter(|id| !all_ids.contains(id))
+            .map(|s| s.as_str())
+            .collect();
+
+        if !unknown_component_ids.is_empty() {
+            bail!("Unknown component(s) {}", unknown_component_ids.join(", "));
+        }


nit: Might as well avoid the unnecessary allocation in the happy path?

Suggested change

let unknown_component_ids: Vec<_> = component_ids

.iter()

.filter(|id| !all_ids.contains(id))

.map(|s| s.as_str())

.collect();

if !unknown_component_ids.is_empty() {

bail!("Unknown component(s) {}", unknown_component_ids.join(", "));

}

let unknown_component_ids = component_ids

.iter()

.filter(|id| !all_ids.contains(id))

.peekable();

if unknown_component_ids.peek().is_some() {

bail!("Unknown component(s) {}", unknown_component_ids.collect::<Vec<_>>().join(", "));

}

TIL: peekable() - thank you!

"Avoid allocating an empty vector" doesn't feel like a huge priority when the happy path goes on to run an external process (a compiler, no less). And I have to admit I find is_empty more obvious than peek().is_some(), though maybe the latter is idiomatic to Rust devs?

But I can change it (to this or @lann's suggestion) if other folks prefer. We could also use @lann's mention of itertools::join to simplify the sad path in this one.

No need to block on my nits. I'll let you decide how to proceed from here. 😀

Yeah same for my suggestions, which are more along the lines of "behold the majesty of the yak shave".

lann · 2023-05-19T15:39:34Z

crates/build/src/lib.rs

+        let unknown_component_ids: Vec<_> = component_ids
+            .iter()
+            .filter(|id| !all_ids.contains(id))
+            .map(|s| s.as_str())
+            .collect();
+
+        if !unknown_component_ids.is_empty() {
+            bail!("Unknown component(s) {}", unknown_component_ids.join(", "));
+        }


Yet another option...

Suggested change

let unknown_component_ids: Vec<_> = component_ids

.iter()

.filter(|id| !all_ids.contains(id))

.map(|s| s.as_str())

.collect();

if !unknown_component_ids.is_empty() {

bail!("Unknown component(s) {}", unknown_component_ids.join(", "));

}

let unknown_component_ids = component_ids

.iter()

.filter(|id| !all_ids.contains(id))

.map(|s| s.as_str())

.collect::<Vec<_>>()

.join(", ");

ensure!(

unknown_component_ids.is_empty(),

"Unknown component(s) {unknown_component_ids}",

);

Sub-option: itertools (which other spin crates already use) has itertools::join

I thought about this but felt that "this set is empty" was more explanatory than "this string we are composing for UI purposes is empty."

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

itowlson requested review from fibonacci1729, lann and michelleN May 18, 2023 01:56

fibonacci1729 approved these changes May 18, 2023

View reviewed changes

itowlson force-pushed the build-one branch from d83ab7e to bab505d Compare May 18, 2023 02:21

lann reviewed May 18, 2023

View reviewed changes

itowlson force-pushed the build-one branch from bab505d to 805b8ec Compare May 18, 2023 22:21

itowlson requested a review from lann May 18, 2023 22:21

itowlson changed the title ~~Support building a single component~~ Support building only specific components May 19, 2023

rylev reviewed May 19, 2023

View reviewed changes

lann reviewed May 19, 2023

View reviewed changes

lann approved these changes May 22, 2023

View reviewed changes

Support building individual components (for spin doctor)

e10ab53

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

itowlson force-pushed the build-one branch from 805b8ec to e10ab53 Compare May 22, 2023 19:20

itowlson merged commit 4add263 into spinframework:main May 22, 2023

itowlson mentioned this pull request Sep 18, 2024

Add experimental spin up --component flag to run a subset of app components #2826

Merged

		eprintln!("Component '{id}' does not have a build command.");
		eprintln!("For information on specifying a build command, see https://developer.fermyon.com/spin/build#setting-up-for-spin-build.");

Conversation

itowlson commented May 18, 2023 • edited by lann Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lann commented May 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itowlson commented May 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

itowlson commented May 18, 2023 •

edited by lann

Loading