From 34f9a800c584805d572696bc6549802c40544c58 Mon Sep 17 00:00:00 2001 From: dcookspi <92065525+dcookspi@users.noreply.github.com> Date: Wed, 6 Nov 2024 17:47:23 -0800 Subject: [PATCH] Adds support to spk commands for requests files (#1130) Adds support to spk commands to take a path to yaml files of requests. Signed-off-by: David Gilligan-Cook --- crates/spk-cli/cmd-env/src/cmd_env.rs | 3 +- crates/spk-cli/cmd-explain/src/cmd_explain.rs | 3 +- crates/spk-cli/cmd-install/src/cmd_install.rs | 4 +- .../cmd-make-binary/src/cmd_make_binary.rs | 8 +- .../cmd-make-recipe/src/cmd_make_recipe.rs | 17 +- .../cmd-make-source/src/cmd_make_source.rs | 25 +- crates/spk-cli/cmd-render/src/cmd_render.rs | 3 +- crates/spk-cli/cmd-test/src/cmd_test.rs | 8 +- crates/spk-cli/common/src/flags.rs | 313 +++++++++++------- crates/spk-cli/common/src/flags_test.rs | 1 - crates/spk-cli/group1/src/cmd_bake.rs | 3 +- crates/spk-cli/group2/src/cmd_num_variants.rs | 10 +- crates/spk-cli/group4/src/cmd_view.rs | 16 +- crates/spk-schema/src/error_test.rs | 2 +- crates/spk-schema/src/lib.rs | 2 +- crates/spk-schema/src/spec.rs | 195 ++++++++--- crates/spk-schema/src/spec_test.rs | 7 +- crates/spk-schema/src/template.rs | 9 +- crates/spk-schema/src/v0/mod.rs | 2 + crates/spk-schema/src/v0/requirements.rs | 21 ++ crates/spk-schema/src/v0/spec.rs | 4 +- crates/spk-schema/src/v0/spec_test.rs | 7 +- cspell.json | 1 + 23 files changed, 468 insertions(+), 196 deletions(-) create mode 100644 crates/spk-schema/src/v0/requirements.rs diff --git a/crates/spk-cli/cmd-env/src/cmd_env.rs b/crates/spk-cli/cmd-env/src/cmd_env.rs index b9365a2c24..639ec7ea6d 100644 --- a/crates/spk-cli/cmd-env/src/cmd_env.rs +++ b/crates/spk-cli/cmd-env/src/cmd_env.rs @@ -77,10 +77,11 @@ impl Run for Env { let mut solver = self.solver.get_solver(&self.options).await?; - let requests = self + let (requests, extra_options) = self .requests .parse_requests(&self.requested, &self.options, solver.repositories()) .await?; + solver.update_options(extra_options); for request in requests { solver.add_request(request) } diff --git a/crates/spk-cli/cmd-explain/src/cmd_explain.rs b/crates/spk-cli/cmd-explain/src/cmd_explain.rs index 2a30966b1f..490911cb6f 100644 --- a/crates/spk-cli/cmd-explain/src/cmd_explain.rs +++ b/crates/spk-cli/cmd-explain/src/cmd_explain.rs @@ -70,10 +70,11 @@ impl Run for Explain { let mut solver = self.solver.get_solver(&self.options).await?; - let requests = self + let (requests, extra_options) = self .requests .parse_requests(&self.requested, &self.options, solver.repositories()) .await?; + solver.update_options(extra_options); for request in requests { solver.add_request(request) } diff --git a/crates/spk-cli/cmd-install/src/cmd_install.rs b/crates/spk-cli/cmd-install/src/cmd_install.rs index 6df5d39cfd..52d71b5ee7 100644 --- a/crates/spk-cli/cmd-install/src/cmd_install.rs +++ b/crates/spk-cli/cmd-install/src/cmd_install.rs @@ -50,11 +50,11 @@ impl Run for Install { current_env().map_err(|err| err.into()) )?; - let requests = self + let (requests, extra_options) = self .requests .parse_requests(&self.packages, &self.options, solver.repositories()) .await?; - + solver.update_options(extra_options); for solved in env.items() { solver.add_request(solved.request.clone().into()); } diff --git a/crates/spk-cli/cmd-make-binary/src/cmd_make_binary.rs b/crates/spk-cli/cmd-make-binary/src/cmd_make_binary.rs index 3dfc82e84a..8bbe9d6407 100644 --- a/crates/spk-cli/cmd-make-binary/src/cmd_make_binary.rs +++ b/crates/spk-cli/cmd-make-binary/src/cmd_make_binary.rs @@ -139,12 +139,18 @@ impl Run for MakeBinary { (!self.options.no_host).then(|| HOST_OPTIONS.get().unwrap_or_default()); for package in packages { - let (recipe, filename) = flags::find_package_recipe_from_template_or_repo( + let (spec_data, filename) = flags::find_package_recipe_from_template_or_repo( package.as_ref().map(|p| p.get_specifier()), &options, &repos, ) .await?; + let recipe = spec_data.into_recipe().wrap_err_with(|| { + format!( + "{filename} was expected to contain a recipe", + filename = filename.to_string_lossy() + ) + })?; let ident = recipe.ident(); tracing::info!("saving package recipe for {}", ident.format_ident()); diff --git a/crates/spk-cli/cmd-make-recipe/src/cmd_make_recipe.rs b/crates/spk-cli/cmd-make-recipe/src/cmd_make_recipe.rs index 3cd6fccd3a..b3cbce8f9d 100644 --- a/crates/spk-cli/cmd-make-recipe/src/cmd_make_recipe.rs +++ b/crates/spk-cli/cmd-make-recipe/src/cmd_make_recipe.rs @@ -8,8 +8,7 @@ use clap::Args; use miette::{Context, IntoDiagnostic, Result}; use spk_cli_common::{flags, CommandArgs, Run}; use spk_schema::foundation::format::FormatOptionMap; -use spk_schema::foundation::spec_ops::Named; -use spk_schema::{SpecTemplate, Template, TemplateExt}; +use spk_schema::{SpecFileData, SpecTemplate, Template, TemplateExt}; /// Render a package spec template into a recipe /// @@ -56,7 +55,11 @@ impl Run for MakeRecipe { } }; - tracing::info!("rendering template for {}", template.name()); + if let Some(name) = template.name() { + tracing::info!("rendering template for {name}"); + } else { + tracing::info!("rendering template without a name"); + } tracing::info!("using options {}", options.format_option_map()); let data = spk_schema::TemplateData::new(&options); tracing::debug!("full template data: {data:#?}"); @@ -74,10 +77,16 @@ impl Run for MakeRecipe { tracing::error!("This template did not render into a valid spec {err}"); Ok(1) } - Ok(_) => { + Ok(SpecFileData::Recipe(_)) => { tracing::info!("Successfully rendered a valid spec"); Ok(0) } + Ok(SpecFileData::Requests(_)) => { + tracing::error!( + "This template did not render into a valid recipe spec. It is a requests spec" + ); + Ok(2) + } } } } diff --git a/crates/spk-cli/cmd-make-source/src/cmd_make_source.rs b/crates/spk-cli/cmd-make-source/src/cmd_make_source.rs index a6e771b54e..6b7629cc61 100644 --- a/crates/spk-cli/cmd-make-source/src/cmd_make_source.rs +++ b/crates/spk-cli/cmd-make-source/src/cmd_make_source.rs @@ -10,7 +10,6 @@ use miette::{bail, Context, Result}; use spk_build::SourcePackageBuilder; use spk_cli_common::{flags, BuildArtifact, BuildResult, CommandArgs, Run}; use spk_schema::foundation::format::FormatIdent; -use spk_schema::foundation::spec_ops::Named; use spk_schema::ident::LocatedBuildIdent; use spk_schema::{Package, Recipe, SpecTemplate, Template, TemplateExt}; use spk_storage as storage; @@ -87,19 +86,29 @@ impl MakeSource { .parent() .map(ToOwned::to_owned) .unwrap_or_else(|| std::env::current_dir().unwrap_or_else(|_| PathBuf::from("."))); - - tracing::info!("rendering template for {}", template.name()); - let recipe = template.render(&options)?; + if let Some(name) = template.name() { + tracing::info!("rendering template for {name}"); + } else { + tracing::info!("rendering template without a name"); + } + let rendered_data = template.render(&options)?; + let recipe = rendered_data.into_recipe().wrap_err_with(|| { + format!( + "{filename} was expected to contain a recipe", + filename = template.file_path().to_string_lossy() + ) + })?; let ident = recipe.ident(); tracing::info!("saving package recipe for {}", ident.format_ident()); local.force_publish_recipe(&recipe).await?; tracing::info!("collecting sources for {}", ident.format_ident()); - let (out, _components) = SourcePackageBuilder::from_recipe(recipe) - .build_and_publish(root, &local) - .await - .wrap_err("Failed to collect sources")?; + let (out, _components) = + SourcePackageBuilder::from_recipe(Arc::unwrap_or_clone(recipe)) + .build_and_publish(root, &local) + .await + .wrap_err("Failed to collect sources")?; tracing::info!("created {}", out.ident().format_ident()); self.created_src.push( template.file_path().display().to_string(), diff --git a/crates/spk-cli/cmd-render/src/cmd_render.rs b/crates/spk-cli/cmd-render/src/cmd_render.rs index 65341de85e..3779f2882e 100644 --- a/crates/spk-cli/cmd-render/src/cmd_render.rs +++ b/crates/spk-cli/cmd-render/src/cmd_render.rs @@ -42,10 +42,11 @@ impl Run for Render { async fn run(&mut self) -> Result { let mut solver = self.solver.get_solver(&self.options).await?; - let requests = self + let (requests, extra_options) = self .requests .parse_requests(&self.packages, &self.options, solver.repositories()) .await?; + solver.update_options(extra_options); for name in requests { solver.add_request(name); } diff --git a/crates/spk-cli/cmd-test/src/cmd_test.rs b/crates/spk-cli/cmd-test/src/cmd_test.rs index 0bcf818f71..cb4192388a 100644 --- a/crates/spk-cli/cmd-test/src/cmd_test.rs +++ b/crates/spk-cli/cmd-test/src/cmd_test.rs @@ -100,9 +100,15 @@ impl Run for CmdTest { } }; - let (recipe, filename) = + let (spec_data, filename) = flags::find_package_recipe_from_template_or_repo(Some(&name), &options, &repos) .await?; + let recipe = spec_data.into_recipe().wrap_err_with(|| { + format!( + "{filename} was expected to contain a recipe", + filename = filename.to_string_lossy() + ) + })?; for stage in stages { tracing::info!("Testing {}@{stage}...", filename.display()); diff --git a/crates/spk-cli/common/src/flags.rs b/crates/spk-cli/common/src/flags.rs index 7cf91eb694..4f61712f87 100644 --- a/crates/spk-cli/common/src/flags.rs +++ b/crates/spk-cli/common/src/flags.rs @@ -21,7 +21,6 @@ use spk_schema::foundation::ident_build::Build; use spk_schema::foundation::ident_component::Component; use spk_schema::foundation::name::OptName; use spk_schema::foundation::option_map::OptionMap; -use spk_schema::foundation::spec_ops::Named; use spk_schema::foundation::version::CompatRule; use spk_schema::ident::{ parse_ident, @@ -33,7 +32,16 @@ use spk_schema::ident::{ VarRequest, }; use spk_schema::option_map::HOST_OPTIONS; -use spk_schema::{Recipe, SpecRecipe, SpecTemplate, Template, TemplateExt, TestStage, VariantExt}; +use spk_schema::{ + Recipe, + SpecFileData, + SpecRecipe, + SpecTemplate, + Template, + TemplateExt, + TestStage, + VariantExt, +}; #[cfg(feature = "statsd")] use spk_solve::{get_metrics_client, SPK_RUN_TIME_METRIC}; pub use variant::{Variant, VariantBuildStatus, VariantLocation}; @@ -246,8 +254,10 @@ pub struct Solver { impl Solver { pub async fn get_solver(&self, options: &Options) -> Result { let option_map = options.get_options()?; + let mut solver = solve::Solver::default(); solver.update_options(option_map); + for (name, repo) in self.repos.get_repos_for_non_destructive_operation().await? { tracing::debug!(repo=%name, "using repository"); solver.add_repository(repo); @@ -263,9 +273,6 @@ impl Solver { self.check_impossible_builds || self.check_impossible_all, ); - for r in options.get_var_requests()? { - solver.add_request(r.into()); - } Ok(solver) } } @@ -289,10 +296,6 @@ pub struct Options { #[clap(long = "opt", short)] pub options: Vec, - /// Specify build/resolve options from a json or yaml file (see --opt/-o) - #[clap(long, value_hint = ValueHint::FilePath)] - pub options_file: Vec, - /// Do not add the default options for the current host system #[clap(long)] pub no_host: bool, @@ -307,16 +310,6 @@ impl Options { .wrap_err("Failed to compute options for current host")?, }; - for filename in self.options_file.iter() { - let reader = std::fs::File::open(filename) - .into_diagnostic() - .wrap_err_with(|| format!("Failed to open: {filename:?}"))?; - let options: OptionMap = serde_yaml::from_reader(reader) - .into_diagnostic() - .wrap_err_with(|| format!("Failed to parse as option mapping: {filename:?}"))?; - opts.extend(options); - } - for pair in self.options.iter() { let pair = pair.trim(); if pair.starts_with('{') { @@ -387,8 +380,14 @@ impl Requests { let path = std::path::Path::new(package); if path.is_file() { - let (_, template) = find_package_template(Some(&package))?.must_be_found(); - let recipe = template.render(options)?; + let (filename, template) = find_package_template(Some(&package))?.must_be_found(); + let rendered_data = template.render(options)?; + let recipe = rendered_data.into_recipe().wrap_err_with(|| { + format!( + "{filename} was expected to contain a recipe", + filename = filename.to_string_lossy() + ) + })?; idents.push(recipe.ident().to_any_ident(None)); } else { idents.push(parse_ident(package)?) @@ -398,57 +397,125 @@ impl Requests { Ok(idents) } - /// Parse and build a request from the given string and these flags + /// Parse and build a request, and any extra options, from the + /// given string and these flags. If the request expands into + /// multiple requests, such as from a request file, this will + /// return the last request. Any options returned are filtered to + /// exclude any (override) options given in the options parameter. pub async fn parse_request>( &self, request: R, options: &Options, repos: &[Arc], - ) -> Result { - Ok(self + ) -> Result<(Request, OptionMap)> { + let (mut requests, extra_options) = self .parse_requests([request.as_ref()], options, repos) - .await? - .pop() - .unwrap()) + .await?; + let last_request = requests.pop().unwrap(); + Ok((last_request, extra_options)) } - /// Parse and build requests from the given strings and these flags. + /// Parse and build requests, and any extra options, from the + /// given strings and these flags. Any options returned are + /// filtered to exclude any (override) options given in the + /// options parameter. pub async fn parse_requests( &self, requests: I, options: &Options, repos: &[Arc], - ) -> Result> + ) -> Result<(Vec, OptionMap)> where I: IntoIterator, S: AsRef, { let mut out = Vec::::new(); - let options = options.get_options()?; + let override_options = options.get_options()?; + let mut templating_options = override_options.clone(); + let mut extra_options = OptionMap::default(); + + // From the positional REQUESTS arg for r in requests.into_iter() { - let r = r.as_ref(); - if r.contains('@') { - let (recipe, _, stage, build_variant) = parse_stage_specifier(r, &options, repos) - .await - .wrap_err_with(|| format!("parsing {r} as a filename with stage specifier"))?; + let r: &str = r.as_ref(); + + // Is it a filepath to a package requests yaml file? + if r.ends_with(".spk.yaml") { + let (spec, filename) = + find_package_recipe_from_template_or_repo(Some(&r), &templating_options, repos) + .await + .wrap_err_with(|| format!("finding requests file {r}"))?; + let requests_from_file = spec.into_requests().wrap_err_with(|| { + format!( + "{filename} was expected to contain a list of requests", + filename = filename.to_string_lossy() + ) + })?; + + out.extend(requests_from_file.requirements); + + for (name, value) in requests_from_file.options { + // Command line override options take precedence. + // Only when there is no command line override for + // this option name is it used + if override_options.get(&name).is_none() { + // For template values in later files and specs + templating_options.insert(OptName::new(&name)?.into(), value.clone()); + // For later use by commands, usually when + // setting up a solver + extra_options.insert(OptName::new(&name)?.into(), value); + } + } + continue; + } - match stage { - TestStage::Sources => { - if build_variant.is_some() { - bail!("Source stage does not accept a build variant specifier") - } + let reqs = self + .parse_cli_or_pkg_file_request(r, &templating_options, repos) + .await?; + out.extend(reqs); + } - let ident = recipe.ident().to_any_ident(Some(Build::Source)); - out.push( - PkgRequest::from_ident_exact(ident, RequestedBy::CommandLine).into(), - ); + if out.is_empty() { + Err(Error::String( + "Needs at least one request: Missing required argument ... ".to_string(), + ) + .into()) + } else { + Ok((out, extra_options)) + } + } + + async fn parse_cli_or_pkg_file_request( + &self, + request: &str, + options: &OptionMap, + repos: &[Arc], + ) -> Result> { + // Parses a command line request into one or more requests. + // 'file@stage' strings can expand into more than one request. + let mut out = Vec::::new(); + + if request.contains('@') { + let (recipe, _, stage, build_variant) = parse_stage_specifier(request, options, repos) + .await + .wrap_err_with(|| { + format!("parsing {request} as a filename with stage specifier") + })?; + + match stage { + TestStage::Sources => { + if build_variant.is_some() { + bail!("Source stage does not accept a build variant specifier") } - TestStage::Build => { - let requirements = match build_variant { - Some(VariantIndex(index)) => { - let default_variants = recipe.default_variants(&options); - let variant = + let ident = recipe.ident().to_any_ident(Some(Build::Source)); + out.push(PkgRequest::from_ident_exact(ident, RequestedBy::CommandLine).into()); + } + + TestStage::Build => { + let requirements = match build_variant { + Some(VariantIndex(index)) => { + let default_variants = recipe.default_variants(options); + let variant = default_variants .iter() .skip(index) @@ -460,71 +527,72 @@ impl Requests { recipe.ident().format_ident() ))? .with_overrides(options.clone()); - recipe.get_build_requirements(&variant)? - } - None => recipe.get_build_requirements(&options)?, - }; - out.extend(requirements.into_owned()); - } - - TestStage::Install => { - if build_variant.is_some() { - bail!("Install stage does not accept a build variant specifier") + recipe.get_build_requirements(&variant)? } + None => recipe.get_build_requirements(&options)?, + }; + out.extend(requirements.into_owned()); + } - out.push( - PkgRequest::from_ident_exact( - recipe.ident().to_any_ident(None), - RequestedBy::CommandLine, - ) - .into(), - ) + TestStage::Install => { + if build_variant.is_some() { + bail!("Install stage does not accept a build variant specifier") } - } - continue; - } - let value: serde_yaml::Value = serde_yaml::from_str(r) - .into_diagnostic() - .wrap_err("Request was not a valid yaml value")?; - let mut request_data = match value { - v @ serde_yaml::Value::String(_) => { - let mut mapping = serde_yaml::Mapping::with_capacity(1); - mapping.insert("pkg".into(), v); - mapping - } - serde_yaml::Value::Mapping(m) => m, - _ => { - bail!( - "Invalid request, expected either a string or a mapping, got: {:?}", - value + + out.push( + PkgRequest::from_ident_exact( + recipe.ident().to_any_ident(None), + RequestedBy::CommandLine, + ) + .into(), ) } - }; + } + return Ok(out); + } - let prerelease_policy_key = "prereleasePolicy".into(); - if self.pre && !request_data.contains_key(&prerelease_policy_key) { - request_data.insert(prerelease_policy_key, "IncludeAll".into()); + // This is request without a '@' stage specifier + let value: serde_yaml::Value = serde_yaml::from_str(request) + .into_diagnostic() + .wrap_err("Request was not a valid yaml value")?; + let mut request_data = match value { + v @ serde_yaml::Value::String(_) => { + let mut mapping = serde_yaml::Mapping::with_capacity(1); + mapping.insert("pkg".into(), v); + mapping } + serde_yaml::Value::Mapping(m) => m, + _ => { + bail!( + "Invalid request, expected either a string or a mapping, got: {:?}", + value + ) + } + }; - let mut req = serde_yaml::from_value::(request_data.into()) - .into_diagnostic() - .wrap_err(format!("Failed to parse request {r}"))? - .into_pkg() - .ok_or_else(|| miette!("Expected a package request, got None"))?; - req.add_requester(RequestedBy::CommandLine); + let prerelease_policy_key = "prereleasePolicy".into(); + if self.pre && !request_data.contains_key(&prerelease_policy_key) { + request_data.insert(prerelease_policy_key, "IncludeAll".into()); + } - if req.pkg.components.is_empty() { - if req.pkg.is_source() { - req.pkg.components.insert(Component::Source); - } else { - req.pkg.components.insert(Component::default_for_run()); - } - } - if req.required_compat.is_none() { - req.required_compat = Some(CompatRule::API); + let mut req = serde_yaml::from_value::(request_data.into()) + .into_diagnostic() + .wrap_err_with(|| format!("Failed to parse request {request}"))? + .into_pkg() + .ok_or_else(|| miette!("Expected a package request, got None"))?; + req.add_requester(RequestedBy::CommandLine); + + if req.pkg.components.is_empty() { + if req.pkg.is_source() { + req.pkg.components.insert(Component::Source); + } else { + req.pkg.components.insert(Component::default_for_run()); } - out.push(req.into()); } + if req.required_compat.is_none() { + req.required_compat = Some(CompatRule::API); + } + out.push(req.into()); Ok(out) } @@ -584,7 +652,14 @@ pub async fn parse_stage_specifier( .await .wrap_err_with(|| format!("finding package recipe for {package}"))?; - Ok((spec, filename, stage, build_variant)) + let recipe = spec.into_recipe().wrap_err_with(|| { + format!( + "{filename} was expected to contain a recipe", + filename = filename.to_string_lossy() + ) + })?; + + Ok((recipe, filename, stage, build_variant)) } /// The result of the [`find_package_template`] function. @@ -718,17 +793,20 @@ where return Err(err.into()); } }; - if template.name().as_str() == package.as_ref() { - return Ok(Found { - path, - template: Arc::new(template), - }); + if let Some(name) = template.name() { + if name.as_str() == package.as_ref() { + return Ok(Found { + path, + template: Arc::new(template), + }); + } } } Ok(NotFound(package.as_ref().to_owned())) } +// TODO: rename this because it is more than package recipe spec now? /// Find a package recipe either from a template file in the current /// directory, or published version of the requested package, if any. /// @@ -741,7 +819,7 @@ pub async fn find_package_recipe_from_template_or_repo( package_name: Option<&S>, options: &OptionMap, repos: &[Arc], -) -> Result<(Arc, std::path::PathBuf)> +) -> Result<(SpecFileData, std::path::PathBuf)> where S: AsRef, { @@ -757,8 +835,14 @@ where ) })? { FindPackageTemplateResult::Found { path, template } => { - let recipe = template.render(options)?; - Ok((Arc::new(recipe), path)) + let found = template.render(options).wrap_err_with(|| { + format!( + "{filename} was expected to contain a valid spk yaml data file", + filename = path.to_string_lossy() + ) + })?; + tracing::debug!("Rendered template from the data in {path:?}"); + Ok((found, path)) } FindPackageTemplateResult::MultipleTemplateFiles(files) => { // must_be_found() will exit the program when called on MultipleTemplateFiles @@ -792,7 +876,10 @@ where "Using recipe found for {}", recipe.ident().format_ident(), ); - return Ok((recipe, std::path::PathBuf::from(&name.as_ref()))); + return Ok(( + SpecFileData::Recipe(recipe), + std::path::PathBuf::from(&name.as_ref()), + )); } Err(spk_storage::Error::PackageNotFound(_)) => continue, diff --git a/crates/spk-cli/common/src/flags_test.rs b/crates/spk-cli/common/src/flags_test.rs index 01e2f86831..8be7f54132 100644 --- a/crates/spk-cli/common/src/flags_test.rs +++ b/crates/spk-cli/common/src/flags_test.rs @@ -24,7 +24,6 @@ use spk_schema::foundation::option_map::OptionMap; fn test_option_flags_parsing(#[case] args: &[&str], #[case] expected: &[(&str, &str)]) { let options = super::Options { no_host: true, - options_file: Default::default(), options: args.iter().map(ToString::to_string).collect(), }; let actual = options.get_options().unwrap(); diff --git a/crates/spk-cli/group1/src/cmd_bake.rs b/crates/spk-cli/group1/src/cmd_bake.rs index 17b80bcfef..5d489960d6 100644 --- a/crates/spk-cli/group1/src/cmd_bake.rs +++ b/crates/spk-cli/group1/src/cmd_bake.rs @@ -228,10 +228,11 @@ impl Bake { // with it. let mut solver = self.solver.get_solver(&self.options).await?; - let requests = self + let (requests, extra_options) = self .requests .parse_requests(&self.requested, &self.options, solver.repositories()) .await?; + solver.update_options(extra_options); for request in requests { solver.add_request(request) } diff --git a/crates/spk-cli/group2/src/cmd_num_variants.rs b/crates/spk-cli/group2/src/cmd_num_variants.rs index 13a729a1f1..c472145899 100644 --- a/crates/spk-cli/group2/src/cmd_num_variants.rs +++ b/crates/spk-cli/group2/src/cmd_num_variants.rs @@ -5,7 +5,7 @@ use std::sync::Arc; use clap::Args; -use miette::Result; +use miette::{Result, WrapErr}; use spk_cli_common::{flags, CommandArgs, Run}; use spk_schema::Recipe; @@ -34,12 +34,18 @@ impl Run for NumVariants { .map(|(_, r)| Arc::new(r)) .collect::>(); - let (recipe, _) = flags::find_package_recipe_from_template_or_repo( + let (spec_data, filename) = flags::find_package_recipe_from_template_or_repo( self.package.as_ref(), &options, &repos, ) .await?; + let recipe = spec_data.into_recipe().wrap_err_with(|| { + format!( + "{filename} was expected to contain a recipe", + filename = filename.to_string_lossy() + ) + })?; println!("{}", recipe.default_variants(&options).len()); diff --git a/crates/spk-cli/group4/src/cmd_view.rs b/crates/spk-cli/group4/src/cmd_view.rs index 079563351c..76f52143b0 100644 --- a/crates/spk-cli/group4/src/cmd_view.rs +++ b/crates/spk-cli/group4/src/cmd_view.rs @@ -226,10 +226,16 @@ impl View { options: &OptionMap, show_variants_with_tests: bool, ) -> Result { - let (_, template) = flags::find_package_template(self.package.as_ref()) + let (filename, template) = flags::find_package_template(self.package.as_ref()) .wrap_err("find package template")? .must_be_found(); - let recipe = template.render(options)?; + let rendered_data = template.render(options)?; + let recipe = rendered_data.into_recipe().wrap_err_with(|| { + format!( + "{filename} was expected to contain a recipe", + filename = filename.to_string_lossy() + ) + })?; let default_variants = recipe.default_variants(options); match &self.format { @@ -467,7 +473,7 @@ impl View { let solver = self.solver.get_solver(&self.options).await?; let repos = solver.repositories(); - let parsed_request = match self + let (parsed_request, _extra_options) = match self .requests .parse_request(&package, &self.options, repos) .await @@ -607,7 +613,9 @@ impl View { async fn print_package_info_from_solve(&self, package: &String) -> Result { let mut solver = self.solver.get_solver(&self.options).await?; - let request = match self + // _extra_option are unused here because getting package info + // from a solve is basically deprecated and should be removed soon. + let (request, _extra_options) = match self .requests .parse_request(&package, &self.options, solver.repositories()) .await diff --git a/crates/spk-schema/src/error_test.rs b/crates/spk-schema/src/error_test.rs index 5a0059b2c7..27dd22e270 100644 --- a/crates/spk-schema/src/error_test.rs +++ b/crates/spk-schema/src/error_test.rs @@ -27,7 +27,7 @@ fn test_yaml_short() { let expected = r#" 1 | short and wrong - | ^ invalid type: string "short and wrong", expected struct YamlMapping + | ^ invalid type: string "short and wrong", expected struct DataApiVersionMapping "#; let message = err.to_string(); diff --git a/crates/spk-schema/src/lib.rs b/crates/spk-schema/src/lib.rs index cd1cc89a23..cdafe2710c 100644 --- a/crates/spk-schema/src/lib.rs +++ b/crates/spk-schema/src/lib.rs @@ -52,7 +52,7 @@ pub use recipe::{BuildEnv, Recipe}; pub use requirements_list::RequirementsList; pub use serde_json; pub use source_spec::{GitSource, LocalSource, ScriptSource, SourceSpec, TarSource}; -pub use spec::{Spec, SpecRecipe, SpecTemplate, SpecVariant}; +pub use spec::{ApiVersion, Spec, SpecFileData, SpecRecipe, SpecTemplate, SpecVariant}; pub use spk_schema_foundation::option_map::{self, OptionMap}; pub use spk_schema_foundation::{ self as foundation, diff --git a/crates/spk-schema/src/spec.rs b/crates/spk-schema/src/spec.rs index 112c022d0a..82cccedc77 100644 --- a/crates/spk-schema/src/spec.rs +++ b/crates/spk-schema/src/spec.rs @@ -6,6 +6,7 @@ use std::borrow::Cow; use std::io::Read; use std::path::Path; use std::str::FromStr; +use std::sync::Arc; use enum_dispatch::enum_dispatch; use format_serde_error::SerdeError; @@ -136,7 +137,7 @@ macro_rules! spec { /// A generic, structured data object that can be turned into a recipe /// when provided with the necessary option values pub struct SpecTemplate { - name: PkgNameBuf, + name: Option, file_path: std::path::PathBuf, template: String, } @@ -148,20 +149,18 @@ impl SpecTemplate { } } -impl Named for SpecTemplate { - fn name(&self) -> &PkgName { - &self.name +impl SpecTemplate { + pub fn name(&self) -> Option<&PkgNameBuf> { + self.name.as_ref() } } impl Template for SpecTemplate { - type Output = SpecRecipe; - fn file_path(&self) -> &Path { &self.file_path } - fn render(&self, options: &OptionMap) -> Result { + fn render(&self, options: &OptionMap) -> Result { let data = super::TemplateData::new(options); let rendered = spk_schema_tera::render_template( self.file_path.to_string_lossy(), @@ -169,7 +168,9 @@ impl Template for SpecTemplate { &data, ) .map_err(Error::InvalidTemplate)?; - Ok(SpecRecipe::from_yaml(rendered)?) + + let file_data = SpecFileData::from_yaml(rendered)?; + Ok(file_data) } } @@ -177,8 +178,10 @@ impl TemplateExt for SpecTemplate { fn from_file(path: &Path) -> Result { let file_path = dunce::canonicalize(path).map_err(|err| Error::InvalidPath(path.to_owned(), err))?; + let file = std::fs::File::open(&file_path) .map_err(|err| Error::FileOpenError(file_path.to_owned(), err))?; + let mut template = String::new(); std::io::BufReader::new(file) .read_to_string(&mut template) @@ -202,33 +205,48 @@ impl TemplateExt for SpecTemplate { tracing::warn!( "Spec file is missing the 'api' field, this may be an error in the future" ); - tracing::warn!(" > for specs in the original spk format, add 'api: v0/package'"); + tracing::warn!( + " > for package specs in the original spk format, add a 'api: v0/package' line" + ); } let name_field = match api { - Some(serde_yaml::Value::String(api)) if api == "v0/platform" => "platform", - _ => "pkg", + Some(serde_yaml::Value::String(api)) => { + let field = api.split("/").nth(1).unwrap_or("pkg"); + if field == "package" { + "pkg" + } else { + field + } + } + Some(_) => "pkg", + None => "pkg", }; - let pkg = template_value - .get(serde_yaml::Value::String(name_field.to_string())) - .ok_or_else(|| { + let name = if name_field == "requirements" { + // This is Spec data and does not have a name, e.g. Requests(V0::Requirements) + None + } else { + // Read the name from the name field, check it is a valid + // string, and turn it into a PkgNameBuf + let pkg = template_value + .get(serde_yaml::Value::String(name_field.to_string())) + .ok_or_else(|| { + crate::Error::String(format!( + "Missing '{name_field}' field in spec file: {file_path:?}" + )) + })?; + + let pkg = pkg.as_str().ok_or_else(|| { crate::Error::String(format!( - "Missing {name_field} field in spec file: {file_path:?}" + "Invalid value for '{name_field}' field: expected string, got {pkg:?} in {file_path:?}" )) })?; - let pkg = pkg.as_str().ok_or_else(|| { - crate::Error::String(format!( - "Invalid value for '{name_field}' field: expected string, got {pkg:?} in {file_path:?}" - )) - })?; - - let name = PkgNameBuf::from_str( // it should never be possible for split to return 0 results // but this trick avoids the use of unwrap - pkg.split('/').next().unwrap_or(pkg), - )?; + Some(PkgNameBuf::from_str(pkg.split('/').next().unwrap_or(pkg))?) + }; Ok(Self { file_path, @@ -424,18 +442,15 @@ impl FromYaml for SpecRecipe { // the 'api' field does not exist in a spec. To do this properly // and still be able to maintain source location data for // yaml errors, we need to deserialize twice: once to get the - // api version, and a second time to deserialize that version + // api version, and a second time to deserialize that version. + // deserializing into a value and then using from_value + // instead of using from_str twice will lose useful context + // info if the parsing errors. // the name of this struct appears in error messages when the // root of the yaml doc is not a mapping, so we use something - // fairly generic, eg: 'expected struct YamlMapping' - #[derive(Deserialize)] - struct YamlMapping { - #[serde(default = "ApiVersion::default")] - api: ApiVersion, - } - - let with_version = match serde_yaml::from_str::(&yaml) { + // fairly generic, eg: 'expected struct DataApiVersionMapping' + let with_version = match serde_yaml::from_str::(&yaml) { // we cannot simply use map_err because we need the compiler // to understand that we only pass ownership of 'yaml' if // the function is returning @@ -456,8 +471,100 @@ impl FromYaml for SpecRecipe { .map_err(|err| SerdeError::new(yaml, SerdeYamlError(err)))?; Ok(Self::V0Platform(inner)) } + ApiVersion::V0Requirements => { + // Reading a list of requests/requirements file is not + // supported here. But it might be in future. + unimplemented!() + } + } + } +} + +// Used during the initial parsing to determine what kind of data is in a file +#[derive(Deserialize)] +struct DataApiVersionMapping { + #[serde(default = "ApiVersion::default")] + api: ApiVersion, +} + +/// Enum for the kinds of data in a spk yaml file +#[derive(Debug)] +pub enum SpecFileData { + /// A package or platform recipe + Recipe(Arc), + /// A list of requests + Requests(v0::Requirements), +} + +impl SpecFileData { + /// Return a SpecRecipe from this SpecFileData. Errors if it is + /// not SpecRecipe + pub fn into_recipe(self) -> Result> { + match self { + SpecFileData::Recipe(r) => Ok(r.to_owned()), + SpecFileData::Requests(_) => { + Err(Error::String( + "A package or platform recipe spec is required in this context. This is requests data." + .to_string(), + ) + ) + } + } + } + + /// Return a list of requests/requirements from this SpecFileData. + /// Errors if it is not a list of requests + pub fn into_requests(self) -> Result { + match self { + SpecFileData::Recipe(_) => { + Err(Error::String( + "Requests data is required in this context. This is a package or platform recipe spec." + .to_string(), + ) + ) + } + SpecFileData::Requests(r) => Ok(r.to_owned()), } } + + pub fn from_yaml>(yaml: S) -> Result { + let yaml = yaml.into(); + + let value: serde_yaml::Value = + serde_yaml::from_str(&yaml).map_err(Error::SpecEncodingError)?; + + // First work out what kind of data this is, based on the + // DataApiVersionMapping value. + let with_version = match serde_yaml::from_value::(value.clone()) { + // we cannot simply use map_err because we need the compiler + // to understand that we only pass ownership of 'yaml' if + // the function is returning + Err(err) => { + return Err(SerdeError::new(yaml, SerdeYamlError(err)).into()); + } + Ok(m) => m, + }; + + // Create the appropriate object from the parsed value + let spec = match with_version.api { + ApiVersion::V0Package => { + let inner = serde_yaml::from_value(value) + .map_err(|err| SerdeError::new(yaml, SerdeYamlError(err)))?; + SpecFileData::Recipe(Arc::new(SpecRecipe::V0Package(inner))) + } + ApiVersion::V0Platform => { + let inner = serde_yaml::from_value(value) + .map_err(|err| SerdeError::new(yaml, SerdeYamlError(err)))?; + SpecFileData::Recipe(Arc::new(SpecRecipe::V0Platform(inner))) + } + ApiVersion::V0Requirements => { + let requests: v0::Requirements = serde_yaml::from_value(value) + .map_err(|err| SerdeError::new(yaml, SerdeYamlError(err)))?; + SpecFileData::Requests(requests) + } + }; + Ok(spec) + } } #[derive(Debug, Clone, Hash, Eq, PartialEq)] @@ -699,18 +806,15 @@ impl FromYaml for Spec { // the 'api' field does not exist in a spec. To do this properly // and still be able to maintain source location data for // yaml errors, we need to deserialize twice: once to get the - // api version, and a second time to deserialize that version + // api version, and a second time to deserialize that version. + // deserializing into a value and then using from_value + // instead of using from_str twice will lose useful context + // info if the parsing errors. // the name of this struct appears in error messages when the // root of the yaml doc is not a mapping, so we use something - // fairly generic, eg: 'expected struct YamlMapping' - #[derive(Deserialize)] - struct YamlMapping { - #[serde(default = "ApiVersion::default")] - api: ApiVersion, - } - - let with_version = match serde_yaml::from_str::(&yaml) { + // fairly generic, eg: 'expected struct DataApiVersionMapping' + let with_version = match serde_yaml::from_str::(&yaml) { // we cannot simply use map_err because we need the compiler // to understand that we only pass ownership of 'yaml' if // the function is returning @@ -731,6 +835,11 @@ impl FromYaml for Spec { .map_err(|err| SerdeError::new(yaml, SerdeYamlError(err)))?; Ok(Self::V0Package(inner)) } + ApiVersion::V0Requirements => { + // Reading a list of requests/requirement file is not + // supported here. But it might be in future. + unimplemented!() + } } } } @@ -747,6 +856,8 @@ pub enum ApiVersion { V0Package, #[serde(rename = "v0/platform")] V0Platform, + #[serde(rename = "v0/requirements")] + V0Requirements, } impl Default for ApiVersion { diff --git a/crates/spk-schema/src/spec_test.rs b/crates/spk-schema/src/spec_test.rs index 24649e6378..fc4cb67ae5 100644 --- a/crates/spk-schema/src/spec_test.rs +++ b/crates/spk-schema/src/spec_test.rs @@ -321,7 +321,7 @@ sources: - git: https://downloads.testing/my-package/v{{ opt.typo }} "#; let tpl = SpecTemplate { - name: PkgName::new("my-package").unwrap().to_owned(), + name: Some(PkgName::new("my-package").unwrap().to_owned()), file_path: "my-package.spk.yaml".into(), template: SPEC.to_string(), }; @@ -345,13 +345,14 @@ fn test_template_namespace_options() { format_serde_error::never_color(); static SPEC: &str = r#"pkg: mypackage/{{ opt.namespace.version }}"#; let tpl = SpecTemplate { - name: PkgName::new("my-package").unwrap().to_owned(), + name: Some(PkgName::new("my-package").unwrap().to_owned()), file_path: "my-package.spk.yaml".into(), template: SPEC.to_string(), }; let options = option_map! {"namespace.version" => "1.0.0"}; - let recipe = tpl + let rendered_data = tpl .render(&options) .expect("template should render with sub-object access"); + let recipe = rendered_data.into_recipe().unwrap(); assert_eq!(recipe.version().to_string(), "1.0.0"); } diff --git a/crates/spk-schema/src/template.rs b/crates/spk-schema/src/template.rs index a6b1648d39..953af71be9 100644 --- a/crates/spk-schema/src/template.rs +++ b/crates/spk-schema/src/template.rs @@ -6,19 +6,16 @@ use std::collections::HashMap; use std::path::Path; use crate::foundation::option_map::OptionMap; -use crate::foundation::spec_ops::Named; -use crate::Result; +use crate::{Result, SpecFileData}; /// Can be rendered into a recipe. #[enum_dispatch::enum_dispatch] -pub trait Template: Named + Sized { - type Output: super::Recipe; - +pub trait Template: Sized { /// Identify the location of this template on disk fn file_path(&self) -> &Path; /// Render this template with the provided values. - fn render(&self, options: &OptionMap) -> Result; + fn render(&self, options: &OptionMap) -> Result; } pub trait TemplateExt: Template { diff --git a/crates/spk-schema/src/v0/mod.rs b/crates/spk-schema/src/v0/mod.rs index ba52ff8e45..0f23977a76 100644 --- a/crates/spk-schema/src/v0/mod.rs +++ b/crates/spk-schema/src/v0/mod.rs @@ -3,12 +3,14 @@ // https://github.com/spkenv/spk mod platform; +mod requirements; mod spec; mod test_spec; mod variant; mod variant_spec; pub use platform::Platform; +pub use requirements::Requirements; pub use spec::Spec; pub use test_spec::TestSpec; pub use variant::Variant; diff --git a/crates/spk-schema/src/v0/requirements.rs b/crates/spk-schema/src/v0/requirements.rs new file mode 100644 index 0000000000..ebeaa0eb45 --- /dev/null +++ b/crates/spk-schema/src/v0/requirements.rs @@ -0,0 +1,21 @@ +// Copyright (c) Contributors to the SPK project. +// SPDX-License-Identifier: Apache-2.0 +// https://github.com/spkenv/spk + +use std::collections::BTreeMap; + +use serde::{Deserialize, Serialize}; + +use crate::RequirementsList; + +/// For a list of requirements parsed from the requests file +#[derive(Debug, Clone, Hash, PartialEq, Eq, Ord, PartialOrd, Deserialize, Serialize)] +pub struct Requirements { + /// A list of var or pkg requests + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub requirements: RequirementsList, + + /// Additional options for templates and solver's initial options + #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] + pub options: BTreeMap, +} diff --git a/crates/spk-schema/src/v0/spec.rs b/crates/spk-schema/src/v0/spec.rs index 95482bf960..498daffc38 100644 --- a/crates/spk-schema/src/v0/spec.rs +++ b/crates/spk-schema/src/v0/spec.rs @@ -604,7 +604,9 @@ impl Recipe for Spec { Err(err) => return Err(Error::String(format!("Failed to load spk config: {err}"))), }; - updated.meta.update_metadata(&config.metadata)?; + if let Err(err) = updated.meta.update_metadata(&config.metadata) { + tracing::warn!("Failed to collect extra package metadata: {err}"); + } let mut missing_build_requirements = HashMap::new(); let mut missing_runtime_requirements: HashMap)> = diff --git a/crates/spk-schema/src/v0/spec_test.rs b/crates/spk-schema/src/v0/spec_test.rs index 1e6f26dd92..02f28fbc30 100644 --- a/crates/spk-schema/src/v0/spec_test.rs +++ b/crates/spk-schema/src/v0/spec_test.rs @@ -40,13 +40,16 @@ fn test_sources_relative_to_spec_file(tmpdir: tempfile::TempDir) { file.write_all(b"{pkg: test-pkg}").unwrap(); drop(file); - let crate::Spec::V0Package(spec) = SpecTemplate::from_file(&spec_file) + let spec = SpecTemplate::from_file(&spec_file) .unwrap() .render(&OptionMap::default()) + .unwrap(); + let crate::Spec::V0Package(recipe) = spec + .into_recipe() .unwrap() .generate_source_build(&spec_dir) .unwrap(); - if let Some(super::SourceSpec::Local(local)) = spec.sources.first() { + if let Some(super::SourceSpec::Local(local)) = recipe.sources.first() { assert_eq!(local.path, spec_dir); } else { panic!("expected spec to have one local source spec"); diff --git a/cspell.json b/cspell.json index 7f888a1663..630cb53035 100644 --- a/cspell.json +++ b/cspell.json @@ -131,6 +131,7 @@ "depver", "Deque", "derserializer", + "deserializing", "DESTDIR", "devel", "devs",