-
Notifications
You must be signed in to change notification settings - Fork 289
Build profiles #3246
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.
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
base: main
Are you sure you want to change the base?
Build profiles #3246
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -15,14 +15,18 @@ use subprocess::{Exec, Redirection}; | |||||||||||||||||||||||
|
||||||||||||||||||||||||
use crate::manifest::component_build_configs; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
const LAST_BUILD_PROFILE_FILE: &str = "last-build.txt"; | ||||||||||||||||||||||||
const LAST_BUILD_ANON_VALUE: &str = "<anonymous>"; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
/// If present, run the build command of each component. | ||||||||||||||||||||||||
pub async fn build( | ||||||||||||||||||||||||
manifest_file: &Path, | ||||||||||||||||||||||||
profile: Option<&str>, | ||||||||||||||||||||||||
component_ids: &[String], | ||||||||||||||||||||||||
target_checks: TargetChecking, | ||||||||||||||||||||||||
cache_root: Option<PathBuf>, | ||||||||||||||||||||||||
) -> Result<()> { | ||||||||||||||||||||||||
let build_info = component_build_configs(manifest_file) | ||||||||||||||||||||||||
let build_info = component_build_configs(manifest_file, profile) | ||||||||||||||||||||||||
.await | ||||||||||||||||||||||||
.with_context(|| { | ||||||||||||||||||||||||
format!( | ||||||||||||||||||||||||
|
@@ -53,6 +57,8 @@ pub async fn build( | |||||||||||||||||||||||
// If the build failed, exit with an error at this point. | ||||||||||||||||||||||||
build_result?; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
save_last_build_profile(&app_dir, profile); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
let Some(manifest) = build_info.manifest() else { | ||||||||||||||||||||||||
// We can't proceed to checking (because that needs a full healthy manifest), and we've | ||||||||||||||||||||||||
// already emitted any necessary warning, so quit. | ||||||||||||||||||||||||
|
@@ -89,8 +95,19 @@ pub async fn build( | |||||||||||||||||||||||
/// Run all component build commands, using the default options (build all | ||||||||||||||||||||||||
/// components, perform target checking). We run a "default build" in several | ||||||||||||||||||||||||
/// places and this centralises the logic of what such a "default build" means. | ||||||||||||||||||||||||
pub async fn build_default(manifest_file: &Path, cache_root: Option<PathBuf>) -> Result<()> { | ||||||||||||||||||||||||
build(manifest_file, &[], TargetChecking::Check, cache_root).await | ||||||||||||||||||||||||
pub async fn build_default( | ||||||||||||||||||||||||
manifest_file: &Path, | ||||||||||||||||||||||||
profile: Option<&str>, | ||||||||||||||||||||||||
cache_root: Option<PathBuf>, | ||||||||||||||||||||||||
) -> Result<()> { | ||||||||||||||||||||||||
build( | ||||||||||||||||||||||||
manifest_file, | ||||||||||||||||||||||||
profile, | ||||||||||||||||||||||||
&[], | ||||||||||||||||||||||||
TargetChecking::Check, | ||||||||||||||||||||||||
cache_root, | ||||||||||||||||||||||||
) | ||||||||||||||||||||||||
.await | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
fn build_components( | ||||||||||||||||||||||||
|
@@ -215,6 +232,50 @@ fn construct_workdir(app_dir: &Path, workdir: Option<impl AsRef<Path>>) -> Resul | |||||||||||||||||||||||
Ok(cwd) | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
/// Saves the build profile to the "last build profile" file. | ||||||||||||||||||||||||
/// Errors are ignored as they should not block building. | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps propagate errors here and log at the call site? |
||||||||||||||||||||||||
pub fn save_last_build_profile(app_dir: &Path, profile: Option<&str>) { | ||||||||||||||||||||||||
let app_stash_dir = app_dir.join(".spin"); | ||||||||||||||||||||||||
_ = std::fs::create_dir_all(&app_stash_dir); | ||||||||||||||||||||||||
let last_build_profile_file = app_stash_dir.join(LAST_BUILD_PROFILE_FILE); | ||||||||||||||||||||||||
Comment on lines
+237
to
+240
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small nicety: do nothing in the common case:
Suggested change
|
||||||||||||||||||||||||
_ = std::fs::write( | ||||||||||||||||||||||||
&last_build_profile_file, | ||||||||||||||||||||||||
profile.unwrap_or(LAST_BUILD_ANON_VALUE), | ||||||||||||||||||||||||
); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
/// Reads the last build profile from the "last build profile" file. | ||||||||||||||||||||||||
/// Errors are ignored. | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto propagate/log. |
||||||||||||||||||||||||
pub fn read_last_build_profile(app_dir: &Path) -> Option<String> { | ||||||||||||||||||||||||
let app_stash_dir = app_dir.join(".spin"); | ||||||||||||||||||||||||
let last_build_profile_file = app_stash_dir.join(LAST_BUILD_PROFILE_FILE); | ||||||||||||||||||||||||
let last_build_str = std::fs::read_to_string(&last_build_profile_file).ok()?; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
if last_build_str == LAST_BUILD_ANON_VALUE { | ||||||||||||||||||||||||
None | ||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||
Some(last_build_str) | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
Comment on lines
+254
to
+258
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not inarguably better, but another option:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it's too much Go programming but I find this a bit obscure and would prefer to keep spelling it out. Appreciate the education though - just give me a couple of years to internalise it! |
||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
/// Prints a warning to stderr if the given profile is not the same | ||||||||||||||||||||||||
/// as the most recent build in the given application directory. | ||||||||||||||||||||||||
pub fn warn_if_not_latest_build(manifest_path: &Path, profile: Option<&str>) { | ||||||||||||||||||||||||
let Some(app_dir) = manifest_path.parent() else { | ||||||||||||||||||||||||
return; | ||||||||||||||||||||||||
}; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
let latest_build = read_last_build_profile(app_dir); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
if profile != latest_build.as_deref() { | ||||||||||||||||||||||||
let profile_opt = match profile { | ||||||||||||||||||||||||
Some(p) => format!(" --profile {p}"), | ||||||||||||||||||||||||
None => "".to_string(), | ||||||||||||||||||||||||
}; | ||||||||||||||||||||||||
terminal::warn!("You built a different profile more recently than the one you are running. If the app appears to be behaving like an older version then run `spin up --build{profile_opt}`."); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
/// Specifies target environment checking behaviour | ||||||||||||||||||||||||
pub enum TargetChecking { | ||||||||||||||||||||||||
/// The build should check that all components are compatible with all target environments. | ||||||||||||||||||||||||
|
@@ -242,23 +303,23 @@ 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, &[], TargetChecking::Skip, None) | ||||||||||||||||||||||||
build(&bad_trigger_file, None, &[], TargetChecking::Skip, None) | ||||||||||||||||||||||||
.await | ||||||||||||||||||||||||
.unwrap(); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
#[tokio::test] | ||||||||||||||||||||||||
async fn succeeds_if_target_env_matches() { | ||||||||||||||||||||||||
let manifest_path = test_data_root().join("good_target_env.toml"); | ||||||||||||||||||||||||
build(&manifest_path, &[], TargetChecking::Check, None) | ||||||||||||||||||||||||
build(&manifest_path, None, &[], TargetChecking::Check, None) | ||||||||||||||||||||||||
.await | ||||||||||||||||||||||||
.unwrap(); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
#[tokio::test] | ||||||||||||||||||||||||
async fn fails_if_target_env_does_not_match() { | ||||||||||||||||||||||||
let manifest_path = test_data_root().join("bad_target_env.toml"); | ||||||||||||||||||||||||
let err = build(&manifest_path, &[], TargetChecking::Check, None) | ||||||||||||||||||||||||
let err = build(&manifest_path, None, &[], TargetChecking::Check, None) | ||||||||||||||||||||||||
.await | ||||||||||||||||||||||||
.expect_err("should have failed") | ||||||||||||||||||||||||
.to_string(); | ||||||||||||||||||||||||
|
@@ -273,7 +334,8 @@ mod tests { | |||||||||||||||||||||||
#[tokio::test] | ||||||||||||||||||||||||
async fn has_meaningful_error_if_target_env_does_not_match() { | ||||||||||||||||||||||||
let manifest_file = test_data_root().join("bad_target_env.toml"); | ||||||||||||||||||||||||
let manifest = spin_manifest::manifest_from_file(&manifest_file).unwrap(); | ||||||||||||||||||||||||
let mut manifest = spin_manifest::manifest_from_file(&manifest_file).unwrap(); | ||||||||||||||||||||||||
spin_manifest::normalize::normalize_manifest(&mut manifest, None); | ||||||||||||||||||||||||
let application = spin_environments::ApplicationToValidate::new( | ||||||||||||||||||||||||
manifest.clone(), | ||||||||||||||||||||||||
manifest_file.parent().unwrap(), | ||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -8,9 +8,10 @@ use crate::schema::v2::{AppManifest, ComponentSpec, KebabId}; | |||||||||||||||||||||||||||||||
/// - Inline components in trigger configs are moved into top-level | ||||||||||||||||||||||||||||||||
/// components and replaced with a reference. | ||||||||||||||||||||||||||||||||
/// - Any triggers without an ID are assigned a generated ID. | ||||||||||||||||||||||||||||||||
pub fn normalize_manifest(manifest: &mut AppManifest) { | ||||||||||||||||||||||||||||||||
pub fn normalize_manifest(manifest: &mut AppManifest, profile: Option<&str>) { | ||||||||||||||||||||||||||||||||
normalize_trigger_ids(manifest); | ||||||||||||||||||||||||||||||||
normalize_inline_components(manifest); | ||||||||||||||||||||||||||||||||
apply_profile_overrides(manifest, profile); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
fn normalize_inline_components(manifest: &mut AppManifest) { | ||||||||||||||||||||||||||||||||
|
@@ -103,3 +104,45 @@ fn normalize_trigger_ids(manifest: &mut AppManifest) { | |||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
fn apply_profile_overrides(manifest: &mut AppManifest, profile: Option<&str>) { | ||||||||||||||||||||||||||||||||
let Some(profile) = profile else { | ||||||||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
for (_, component) in &mut manifest.components { | ||||||||||||||||||||||||||||||||
let Some(overrides) = component.profile.get(profile) else { | ||||||||||||||||||||||||||||||||
continue; | ||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||
lann marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
if let Some(profile_build) = overrides.build.as_ref() { | ||||||||||||||||||||||||||||||||
match component.build.as_mut() { | ||||||||||||||||||||||||||||||||
None => { | ||||||||||||||||||||||||||||||||
component.build = Some(crate::schema::v2::ComponentBuildConfig { | ||||||||||||||||||||||||||||||||
command: profile_build.command.clone(), | ||||||||||||||||||||||||||||||||
workdir: None, | ||||||||||||||||||||||||||||||||
watch: vec![], | ||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
Some(build) => { | ||||||||||||||||||||||||||||||||
build.command = profile_build.command.clone(); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
if let Some(source) = overrides.source.as_ref() { | ||||||||||||||||||||||||||||||||
component.source = source.clone(); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
for (name, value) in &overrides.environment { | ||||||||||||||||||||||||||||||||
component.environment.insert(name.clone(), value.clone()); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
for (reference, value) in &overrides.dependencies.inner { | ||||||||||||||||||||||||||||||||
component | ||||||||||||||||||||||||||||||||
.dependencies | ||||||||||||||||||||||||||||||||
.inner | ||||||||||||||||||||||||||||||||
.insert(reference.clone(), value.clone()); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
Comment on lines
+137
to
+146
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think these ought to work 🤷
Suggested change
(and really in this context it would be completely fine to just |
||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚲🏠