From 25c47ed0bc0a7cadf9e8dc092629a21371803afc Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 18 Aug 2025 11:02:13 -0700 Subject: [PATCH 1/2] Add tests for with_renderer and with_preprocessor collisions This adds tests with with_renderer and with_preprocessor are used with extensions that have the same name. --- tests/testsuite/book_test.rs | 3 ++- tests/testsuite/preprocessor.rs | 24 ++++++++++++++++++++++++ tests/testsuite/renderer.rs | 25 +++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 1 deletion(-) diff --git a/tests/testsuite/book_test.rs b/tests/testsuite/book_test.rs index 2f5e9d6334..3cf6b0a45e 100644 --- a/tests/testsuite/book_test.rs +++ b/tests/testsuite/book_test.rs @@ -22,7 +22,8 @@ enum StatusCode { pub struct BookTest { /// The temp directory where the test should perform its work. pub dir: PathBuf, - assert: snapbox::Assert, + /// Snapshot assertion support. + pub assert: snapbox::Assert, /// This indicates whether or not the book has been built. built: bool, } diff --git a/tests/testsuite/preprocessor.rs b/tests/testsuite/preprocessor.rs index 9fa1df1cbd..81b7b4e767 100644 --- a/tests/testsuite/preprocessor.rs +++ b/tests/testsuite/preprocessor.rs @@ -180,3 +180,27 @@ fn missing_optional_not_fatal() { "#]]); }); } + +// with_preprocessor of an existing name. +#[test] +fn with_preprocessor_same_name() { + let mut test = BookTest::init(|_| {}); + test.change_file( + "book.toml", + "[preprocessor.dummy]\n\ + command = 'mdbook-preprocessor-does-not-exist'\n", + ); + let spy: Arc> = Default::default(); + let mut book = test.load_book(); + book.with_preprocessor(Spy(Arc::clone(&spy))); + let err = book.build().unwrap_err(); + test.assert.eq( + format!("{err:?}"), + str![[r#" +Unable to run the preprocessor `dummy` + +Caused by: + [NOT_FOUND] +"#]], + ); +} diff --git a/tests/testsuite/renderer.rs b/tests/testsuite/renderer.rs index a04ff0e3ec..4f84529723 100644 --- a/tests/testsuite/renderer.rs +++ b/tests/testsuite/renderer.rs @@ -241,3 +241,28 @@ fn relative_command_path() { }) .check_file("book/output", "test"); } + +// with_renderer of an existing name. +#[test] +fn with_renderer_same_name() { + let mut test = BookTest::init(|_| {}); + test.change_file( + "book.toml", + "[output.dummy]\n\ + command = 'mdbook-renderer-does-not-exist'\n", + ); + let spy: Arc> = Default::default(); + let mut book = test.load_book(); + book.with_renderer(Spy(Arc::clone(&spy))); + let err = book.build().unwrap_err(); + test.assert.eq( + format!("{err:?}"), + str![[r#" +Rendering failed + +Caused by: + 0: Unable to run the backend `dummy` + 1: [NOT_FOUND] +"#]], + ); +} From 338a9b424ecb670b5ef47abad10657779f4b116e Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 18 Aug 2025 11:14:29 -0700 Subject: [PATCH 2/2] Change with_renderer/with_preprocessor to overwrite This changes `with_renderer` and `with_preprocessor` to replace any extensions of the same name instead of just appending to the list. This is necessary for rust-lang's build process, because we replace the preprocessors with local ones. Previously, mdbook would just print an error, but continue working. With the change that preprocessors are no longer optional by default, it is now required that we have a way to replace the existing entries. --- Cargo.lock | 1 + Cargo.toml | 1 + crates/mdbook-driver/Cargo.toml | 1 + crates/mdbook-driver/src/mdbook.rs | 52 ++++++++++++++---------- crates/mdbook-driver/src/mdbook/tests.rs | 29 ++++--------- tests/testsuite/preprocessor.rs | 15 +++---- tests/testsuite/renderer.rs | 15 ++----- 7 files changed, 51 insertions(+), 63 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fe63883f1c..2b1daadad5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1335,6 +1335,7 @@ name = "mdbook-driver" version = "0.5.0-alpha.1" dependencies = [ "anyhow", + "indexmap", "log", "mdbook-core", "mdbook-html", diff --git a/Cargo.toml b/Cargo.toml index c085d69984..dbdfb44232 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -37,6 +37,7 @@ font-awesome-as-a-crate = "0.3.0" futures-util = "0.3.31" handlebars = "6.3.2" hex = "0.4.3" +indexmap = "2.10.0" ignore = "0.4.23" log = "0.4.27" mdbook-core = { path = "crates/mdbook-core" } diff --git a/crates/mdbook-driver/Cargo.toml b/crates/mdbook-driver/Cargo.toml index f7edc05392..9f7936a4ad 100644 --- a/crates/mdbook-driver/Cargo.toml +++ b/crates/mdbook-driver/Cargo.toml @@ -9,6 +9,7 @@ rust-version.workspace = true [dependencies] anyhow.workspace = true +indexmap.workspace = true log.workspace = true mdbook-core.workspace = true mdbook-html.workspace = true diff --git a/crates/mdbook-driver/src/mdbook.rs b/crates/mdbook-driver/src/mdbook.rs index 4edc27c63a..d173502ba4 100644 --- a/crates/mdbook-driver/src/mdbook.rs +++ b/crates/mdbook-driver/src/mdbook.rs @@ -5,6 +5,7 @@ use crate::builtin_renderers::{CmdRenderer, MarkdownRenderer}; use crate::init::BookBuilder; use crate::load::{load_book, load_book_from_disk}; use anyhow::{Context, Error, Result, bail}; +use indexmap::IndexMap; use log::{debug, error, info, log_enabled, trace, warn}; use mdbook_core::book::{Book, BookItem, BookItems}; use mdbook_core::config::{Config, RustEdition}; @@ -28,14 +29,18 @@ mod tests; pub struct MDBook { /// The book's root directory. pub root: PathBuf, + /// The configuration used to tweak now a book is built. pub config: Config, + /// A representation of the book's contents in memory. pub book: Book, - renderers: Vec>, - /// List of pre-processors to be run on the book. - preprocessors: Vec>, + /// Renderers to execute. + renderers: IndexMap>, + + /// Pre-processors to be run on the book. + preprocessors: IndexMap>, } impl MDBook { @@ -156,7 +161,7 @@ impl MDBook { pub fn build(&self) -> Result<()> { info!("Book building has started"); - for renderer in &self.renderers { + for renderer in self.renderers.values() { self.execute_build_process(&**renderer)?; } @@ -171,7 +176,7 @@ impl MDBook { renderer.name().to_string(), ); let mut preprocessed_book = self.book.clone(); - for preprocessor in &self.preprocessors { + for preprocessor in self.preprocessors.values() { if preprocessor_should_run(&**preprocessor, renderer, &self.config)? { debug!("Running the {} preprocessor.", preprocessor.name()); preprocessed_book = preprocessor.run(&preprocess_ctx, preprocessed_book)?; @@ -207,13 +212,15 @@ impl MDBook { /// The only requirement is that your renderer implement the [`Renderer`] /// trait. pub fn with_renderer(&mut self, renderer: R) -> &mut Self { - self.renderers.push(Box::new(renderer)); + self.renderers + .insert(renderer.name().to_string(), Box::new(renderer)); self } /// Register a [`Preprocessor`] to be used when rendering the book. pub fn with_preprocessor(&mut self, preprocessor: P) -> &mut Self { - self.preprocessors.push(Box::new(preprocessor)); + self.preprocessors + .insert(preprocessor.name().to_string(), Box::new(preprocessor)); self } @@ -258,10 +265,9 @@ impl MDBook { // Index Preprocessor is disabled so that chapter paths // continue to point to the actual markdown files. - self.preprocessors = determine_preprocessors(&self.config, &self.root)? - .into_iter() - .filter(|pre| pre.name() != IndexPreprocessor::NAME) - .collect(); + self.preprocessors = determine_preprocessors(&self.config, &self.root)?; + self.preprocessors + .shift_remove_entry(IndexPreprocessor::NAME); let (book, _) = self.preprocess_book(&TestRenderer)?; let color_output = std::io::stderr().is_terminal(); @@ -399,24 +405,25 @@ struct OutputConfig { } /// Look at the `Config` and try to figure out what renderers to use. -fn determine_renderers(config: &Config) -> Result>> { - let mut renderers = Vec::new(); +fn determine_renderers(config: &Config) -> Result>> { + let mut renderers = IndexMap::new(); let outputs = config.outputs::()?; renderers.extend(outputs.into_iter().map(|(key, table)| { - if key == "html" { + let renderer = if key == "html" { Box::new(HtmlHandlebars::new()) as Box } else if key == "markdown" { Box::new(MarkdownRenderer::new()) as Box } else { let command = table.command.unwrap_or_else(|| format!("mdbook-{key}")); - Box::new(CmdRenderer::new(key, command)) - } + Box::new(CmdRenderer::new(key.clone(), command)) + }; + (key, renderer) })); // if we couldn't find anything, add the HTML renderer as a default if renderers.is_empty() { - renderers.push(Box::new(HtmlHandlebars::new())); + renderers.insert("html".to_string(), Box::new(HtmlHandlebars::new())); } Ok(renderers) @@ -442,7 +449,10 @@ struct PreprocessorConfig { } /// Look at the `MDBook` and try to figure out what preprocessors to run. -fn determine_preprocessors(config: &Config, root: &Path) -> Result>> { +fn determine_preprocessors( + config: &Config, + root: &Path, +) -> Result>> { // Collect the names of all preprocessors intended to be run, and the order // in which they should be run. let mut preprocessor_names = TopologicalSort::::new(); @@ -490,7 +500,7 @@ fn determine_preprocessors(config: &Config, root: &Path) -> Result Result= index(after) { eprintln!("Preprocessor order:"); - for preprocessor in &preprocessors { - eprintln!(" {}", preprocessor.name()); + for preprocessor in preprocessors.keys() { + eprintln!(" {}", preprocessor); } panic!("{before} should come before {after}"); } @@ -193,11 +186,8 @@ fn dependencies_dont_register_undefined_preprocessors() { let preprocessors = determine_preprocessors(&cfg, Path::new("")).unwrap(); - assert!( - !preprocessors - .iter() - .any(|preprocessor| preprocessor.name() == "random") - ); + // Does not contain "random" + assert_eq!(preprocessors.keys().collect::>(), ["index", "links"]); } #[test] @@ -214,11 +204,8 @@ fn dependencies_dont_register_builtin_preprocessors_if_disabled() { let preprocessors = determine_preprocessors(&cfg, Path::new("")).unwrap(); - assert!( - !preprocessors - .iter() - .any(|preprocessor| preprocessor.name() == "links") - ); + // Does not contain "links" + assert_eq!(preprocessors.keys().collect::>(), ["random"]); } #[test] diff --git a/tests/testsuite/preprocessor.rs b/tests/testsuite/preprocessor.rs index 81b7b4e767..de3e5efe38 100644 --- a/tests/testsuite/preprocessor.rs +++ b/tests/testsuite/preprocessor.rs @@ -193,14 +193,9 @@ fn with_preprocessor_same_name() { let spy: Arc> = Default::default(); let mut book = test.load_book(); book.with_preprocessor(Spy(Arc::clone(&spy))); - let err = book.build().unwrap_err(); - test.assert.eq( - format!("{err:?}"), - str![[r#" -Unable to run the preprocessor `dummy` - -Caused by: - [NOT_FOUND] -"#]], - ); + // Unfortunately this is unable to capture the output when using the API. + book.build().unwrap(); + let inner = spy.lock().unwrap(); + assert_eq!(inner.run_count, 1); + assert_eq!(inner.rendered_with, ["html"]); } diff --git a/tests/testsuite/renderer.rs b/tests/testsuite/renderer.rs index 4f84529723..456fcd7e56 100644 --- a/tests/testsuite/renderer.rs +++ b/tests/testsuite/renderer.rs @@ -254,15 +254,8 @@ fn with_renderer_same_name() { let spy: Arc> = Default::default(); let mut book = test.load_book(); book.with_renderer(Spy(Arc::clone(&spy))); - let err = book.build().unwrap_err(); - test.assert.eq( - format!("{err:?}"), - str![[r#" -Rendering failed - -Caused by: - 0: Unable to run the backend `dummy` - 1: [NOT_FOUND] -"#]], - ); + // Unfortunately this is unable to capture the output when using the API. + book.build().unwrap(); + let inner = spy.lock().unwrap(); + assert_eq!(inner.run_count, 1); }