From 21da03da2d31ae49b9b5d9ac2e7c552c20ed1447 Mon Sep 17 00:00:00 2001 From: Marco Stronati Date: Mon, 16 Dec 2024 23:09:43 +0100 Subject: [PATCH] CLI bundle: add debug and kernel options. fixes #1447 (#1597) * cli: bundle add debug option * assembly: rename error EmptyKernel into NoExport * assembly: check library has at least one export * cli: bundle add kernel option * fixup: move tests * cli: bundle add `--output` option * fixup: comment * cli: changelog * fixup: test debug * cli: bundle include stdlib tested with: cargo run --features executable -- bundle ~/miden/miden-base/miden-lib/asm/kernels/transaction/lib --kernel ~/miden/miden-base/miden-lib/asm/kernels/transaction/api.masm * cli: bundle improve checks and errors * fixup test, add iterator for decorators in a node --- CHANGELOG.md | 1 + assembly/src/library/error.rs | 4 +- assembly/src/library/mod.rs | 11 ++- core/src/mast/mod.rs | 4 + miden/src/cli/bundle.rs | 76 ++++++++++++------ miden/tests/integration/cli/cli_test.rs | 79 +++++++++++++++++-- .../integration/cli/data/kernel_main.masm | 9 +++ miden/tests/integration/cli/data/lib/lib.masm | 3 + .../cli/data/lib_noexports/lib.masm | 3 + 9 files changed, 155 insertions(+), 35 deletions(-) create mode 100644 miden/tests/integration/cli/data/kernel_main.masm create mode 100644 miden/tests/integration/cli/data/lib/lib.masm create mode 100644 miden/tests/integration/cli/data/lib_noexports/lib.masm diff --git a/CHANGELOG.md b/CHANGELOG.md index 93797a7f6..32fb2da34 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - [BREAKING] Use `thiserror` 2.0 to derive errors and refactor them (#1588). #### Enhancements +- Added options `--kernel`, `--debug` and `--output` to `miden bundle` (#1447). - Added `miden_core::mast::MastForest::advice_map` to load it into the advice provider before the `MastForest` execution (#1574). ## 0.11.0 (2024-11-04) diff --git a/assembly/src/library/error.rs b/assembly/src/library/error.rs index e5158188e..9d3c58361 100644 --- a/assembly/src/library/error.rs +++ b/assembly/src/library/error.rs @@ -4,9 +4,9 @@ use crate::{ast::QualifiedProcedureName, diagnostics::Diagnostic}; #[derive(Debug, thiserror::Error, Diagnostic)] pub enum LibraryError { - #[error("kernel library must contain at least one exported procedure")] + #[error("library must contain at least one exported procedure")] #[diagnostic()] - EmptyKernel, + NoExport, #[error("invalid export in kernel library: {procedure_path}")] InvalidKernelExport { procedure_path: QualifiedProcedureName }, // Technically KernelError is the source error here, but since LibraryError is sometimes diff --git a/assembly/src/library/mod.rs b/assembly/src/library/mod.rs index 8f42223c2..38cadb3ae 100644 --- a/assembly/src/library/mod.rs +++ b/assembly/src/library/mod.rs @@ -72,12 +72,16 @@ impl Library { /// Constructs a new [`Library`] from the provided MAST forest and a set of exports. /// /// # Errors + /// Returns an error if the set of exports is empty. /// Returns an error if any of the specified exports do not have a corresponding procedure root /// in the provided MAST forest. pub fn new( mast_forest: Arc, exports: BTreeMap, ) -> Result { + if exports.is_empty() { + return Err(LibraryError::NoExport); + } for (fqn, &proc_body_id) in exports.iter() { if !mast_forest.is_procedure_root(proc_body_id) { return Err(LibraryError::NoProcedureRootForExport { procedure_path: fqn.clone() }); @@ -177,6 +181,9 @@ impl Deserializable for Library { let mast_forest = Arc::new(MastForest::read_from(source)?); let num_exports = source.read_usize()?; + if num_exports == 0 { + return Err(DeserializationError::InvalidValue(String::from("No exported procedures"))); + }; let mut exports = BTreeMap::new(); for _ in 0..num_exports { let proc_module = source.read()?; @@ -353,10 +360,6 @@ impl TryFrom for KernelLibrary { type Error = LibraryError; fn try_from(library: Library) -> Result { - if library.exports.is_empty() { - return Err(LibraryError::EmptyKernel); - } - let kernel_path = LibraryPath::from(LibraryNamespace::Kernel); let mut proc_digests = Vec::with_capacity(library.exports.len()); diff --git a/core/src/mast/mod.rs b/core/src/mast/mod.rs index 1d8af06ab..d71be872d 100644 --- a/core/src/mast/mod.rs +++ b/core/src/mast/mod.rs @@ -465,6 +465,10 @@ impl MastForest { &self.nodes } + pub fn decorators(&self) -> &[Decorator] { + &self.decorators + } + pub fn advice_map(&self) -> &AdviceMap { &self.advice_map } diff --git a/miden/src/cli/bundle.rs b/miden/src/cli/bundle.rs index 07ae7aa56..356762e3e 100644 --- a/miden/src/cli/bundle.rs +++ b/miden/src/cli/bundle.rs @@ -2,25 +2,37 @@ use std::path::PathBuf; use assembly::{ diagnostics::{IntoDiagnostic, Report}, - Assembler, Library, LibraryNamespace, + Assembler, KernelLibrary, Library, LibraryNamespace, }; use clap::Parser; +use stdlib::StdLibrary; #[derive(Debug, Clone, Parser)] #[clap( name = "Compile Library", - about = "Bundles .masm files into a single .masl library" + about = "Bundles .masm files into a single .masl library with access to the stdlib." )] pub struct BundleCmd { + /// Include debug symbols. + #[clap(short, long, action)] + debug: bool, /// Path to a directory containing the `.masm` files which are part of the library. #[clap(value_parser)] dir: PathBuf, - /// Defines the top-level namespace, e.g. `mylib`, otherwise the directory name is used. + /// Defines the top-level namespace, e.g. `mylib`, otherwise the directory name is used. For a + /// kernel library the namespace defaults to `kernel`. #[clap(short, long)] namespace: Option, /// Version of the library, defaults to `0.1.0`. #[clap(short, long, default_value = "0.1.0")] version: String, + /// Build a kernel library from module `kernel` and using the library `dir` as kernel + /// namespace. The `kernel` file should not be in the directory `dir`. + #[clap(short, long)] + kernel: Option, + /// Path of the output `.masl` file. + #[clap(short, long)] + output: Option, } impl BundleCmd { @@ -29,29 +41,49 @@ impl BundleCmd { println!("Build library"); println!("============================================================"); - let namespace = match &self.namespace { - Some(namespace) => namespace.to_string(), - None => self - .dir - .file_name() - .expect("dir must be a folder") - .to_string_lossy() - .into_owned(), - }; + let mut assembler = Assembler::default().with_debug_mode(self.debug); - let assembler = Assembler::default().with_debug_mode(true); - let library_namespace = - namespace.parse::().expect("invalid base namespace"); - let library = Library::from_dir(&self.dir, library_namespace, assembler)?; + if self.dir.is_file() { + return Err(Report::msg("`dir` must be a directory.")); + } + let dir = self.dir.file_name().ok_or("`dir` cannot end with `..`.").map_err(Report::msg)?; // write the masl output - let output_file = self - .dir - .join(self.namespace.as_deref().unwrap_or("out")) - .with_extension(Library::LIBRARY_EXTENSION); - library.write_to_file(output_file).into_diagnostic()?; + let output_file = match &self.output { + Some(output) => output, + None => { + let parent = + &self.dir.parent().ok_or("Invalid output path").map_err(Report::msg)?; + &parent.join("out").with_extension(Library::LIBRARY_EXTENSION) + }, + }; - println!("Built library {}", namespace); + match &self.kernel { + Some(kernel) => { + if !kernel.is_file() { + return Err(Report::msg("`kernel` must be a file")); + }; + assembler.add_library(StdLibrary::default())?; + let library = KernelLibrary::from_dir(kernel, Some(&self.dir), assembler)?; + library.write_to_file(output_file).into_diagnostic()?; + println!( + "Built kernel module {} with library {}", + kernel.display(), + &self.dir.display() + ); + }, + None => { + let namespace = match &self.namespace { + Some(namespace) => namespace.to_string(), + None => dir.to_string_lossy().into_owned(), + }; + let library_namespace = namespace.parse::()?; + assembler.add_library(StdLibrary::default())?; + let library = Library::from_dir(&self.dir, library_namespace, assembler)?; + library.write_to_file(output_file).into_diagnostic()?; + println!("Built library {}", namespace); + }, + } Ok(()) } diff --git a/miden/tests/integration/cli/cli_test.rs b/miden/tests/integration/cli/cli_test.rs index 8d657bd7a..b50801e98 100644 --- a/miden/tests/integration/cli/cli_test.rs +++ b/miden/tests/integration/cli/cli_test.rs @@ -1,12 +1,11 @@ +use std::{fs, path::Path}; + use assert_cmd::prelude::*; use predicates::prelude::*; extern crate escargot; -#[test] -// Tt test might be an overkill to test only that the 'run' cli command -// outputs steps and ms. -fn cli_run() -> Result<(), Box> { - let bin_under_test = escargot::CargoBuild::new() +fn bin_under_test() -> escargot::CargoRun { + escargot::CargoBuild::new() .bin("miden") .features("executable internal") .current_release() @@ -15,9 +14,14 @@ fn cli_run() -> Result<(), Box> { .unwrap_or_else(|err| { eprintln!("{err}"); panic!("failed to build `miden`"); - }); + }) +} - let mut cmd = bin_under_test.command(); +#[test] +// Tt test might be an overkill to test only that the 'run' cli command +// outputs steps and ms. +fn cli_run() -> Result<(), Box> { + let mut cmd = bin_under_test().command(); cmd.arg("run") .arg("-a") @@ -38,3 +42,64 @@ fn cli_run() -> Result<(), Box> { Ok(()) } + +use assembly::Library; +use vm_core::Decorator; + +#[test] +fn cli_bundle_debug() { + let mut cmd = bin_under_test().command(); + cmd.arg("bundle").arg("--debug").arg("./tests/integration/cli/data/lib"); + cmd.assert().success(); + + let lib = Library::deserialize_from_file("./tests/integration/cli/data/out.masl").unwrap(); + // If there are any AsmOp decorators in the forest, the bundle is in debug mode. + let found_one_asm_op = + lib.mast_forest().decorators().iter().any(|d| matches!(d, Decorator::AsmOp(_))); + assert!(found_one_asm_op); + fs::remove_file("./tests/integration/cli/data/out.masl").unwrap(); +} + +#[test] +fn cli_bundle_no_exports() { + let mut cmd = bin_under_test().command(); + cmd.arg("bundle").arg("./tests/integration/cli/data/lib_noexports"); + cmd.assert() + .failure() + .stderr(predicate::str::contains("library must contain at least one exported procedure")); +} + +#[test] +fn cli_bundle_kernel() { + let mut cmd = bin_under_test().command(); + cmd.arg("bundle") + .arg("./tests/integration/cli/data/lib") + .arg("--kernel") + .arg("./tests/integration/cli/data/kernel_main.masm"); + cmd.assert().success(); + fs::remove_file("./tests/integration/cli/data/out.masl").unwrap() +} + +/// A kernel can bundle with a library w/o exports. +#[test] +fn cli_bundle_kernel_noexports() { + let mut cmd = bin_under_test().command(); + cmd.arg("bundle") + .arg("./tests/integration/cli/data/lib_noexports") + .arg("--kernel") + .arg("./tests/integration/cli/data/kernel_main.masm"); + cmd.assert().success(); + fs::remove_file("./tests/integration/cli/data/out.masl").unwrap() +} + +#[test] +fn cli_bundle_output() { + let mut cmd = bin_under_test().command(); + cmd.arg("bundle") + .arg("./tests/integration/cli/data/lib") + .arg("--output") + .arg("test.masl"); + cmd.assert().success(); + assert!(Path::new("test.masl").exists()); + fs::remove_file("test.masl").unwrap() +} diff --git a/miden/tests/integration/cli/data/kernel_main.masm b/miden/tests/integration/cli/data/kernel_main.masm new file mode 100644 index 000000000..62d062b28 --- /dev/null +++ b/miden/tests/integration/cli/data/kernel_main.masm @@ -0,0 +1,9 @@ +proc.internal_proc + caller + drop + exec.::kernel::lib::lib_proc +end + +export.kernel_proc + exec.internal_proc +end diff --git a/miden/tests/integration/cli/data/lib/lib.masm b/miden/tests/integration/cli/data/lib/lib.masm new file mode 100644 index 000000000..386650e44 --- /dev/null +++ b/miden/tests/integration/cli/data/lib/lib.masm @@ -0,0 +1,3 @@ +export.lib_proc + swap +end diff --git a/miden/tests/integration/cli/data/lib_noexports/lib.masm b/miden/tests/integration/cli/data/lib_noexports/lib.masm new file mode 100644 index 000000000..442d645b0 --- /dev/null +++ b/miden/tests/integration/cli/data/lib_noexports/lib.masm @@ -0,0 +1,3 @@ +proc.lib_proc + swap +end