Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 31 additions & 6 deletions crates/build/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,49 @@ mod manifest;

use anyhow::{anyhow, bail, Context, Result};
use spin_loader::local::parent_dir;
use std::path::{Path, PathBuf};
use std::{
collections::HashSet,
path::{Path, PathBuf},
};
use subprocess::{Exec, Redirection};

use crate::manifest::{BuildAppInfoAnyVersion, RawComponentManifest};

/// If present, run the build command of each component.
pub async fn build(manifest_file: &Path) -> Result<()> {
pub async fn build(manifest_file: &Path, component_ids: &[String]) -> Result<()> {
let manifest_text = tokio::fs::read_to_string(manifest_file)
.await
.with_context(|| format!("Cannot read manifest file from {}", manifest_file.display()))?;
let app = toml::from_str(&manifest_text).map(BuildAppInfoAnyVersion::into_v1)?;
let app_dir = parent_dir(manifest_file)?;

if app.components.iter().all(|c| c.build.is_none()) {
println!("No build command found!");
let components_to_build = if component_ids.is_empty() {
app.components
} else {
let all_ids: HashSet<_> = app.components.iter().map(|c| &c.id).collect();
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(", "));
}
Comment on lines +29 to +37
Copy link
Collaborator

Choose a reason for hiding this comment

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

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(", "));
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Comment on lines +29 to +37
Copy link
Collaborator

Choose a reason for hiding this comment

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

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}",
);

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


app.components
.into_iter()
.filter(|c| component_ids.contains(&c.id))
.collect()
};

if components_to_build.iter().all(|c| c.build.is_none()) {
println!("None of the components have a build command.");
println!("For information on specifying a build command, see https://developer.fermyon.com/spin/build#setting-up-for-spin-build.");
return Ok(());
}
app.components

components_to_build
.into_iter()
.map(|c| build_component(c, &app_dir))
.collect::<Result<Vec<_>, _>>()?;
Expand Down Expand Up @@ -101,6 +126,6 @@ mod tests {
#[tokio::test]
async fn can_load_even_if_trigger_invalid() {
let bad_trigger_file = test_data_root().join("bad_trigger.toml");
build(&bad_trigger_file).await.unwrap();
build(&bad_trigger_file, &[]).await.unwrap();
}
}
8 changes: 4 additions & 4 deletions src/commands/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ pub struct BuildCommand {
)]
pub app_source: PathBuf,

/// Component ID to build. The default is all components.
#[clap(short = 'c', long)]
pub component_id: Option<String>,
/// Component ID to build. This can be specified multiple times. The default is all components.
#[clap(short = 'c', long, multiple = true)]
pub component_id: Vec<String>,

/// Run the application after building.
#[clap(name = BUILD_UP_OPT, short = 'u', long = "up")]
Expand All @@ -38,7 +38,7 @@ pub struct BuildCommand {
impl BuildCommand {
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?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


if self.up {
let mut cmd = UpCommand::parse_from(
Expand Down