From 9b67d825fea1c7dc6dc6f4348255cda1fa825f1d Mon Sep 17 00:00:00 2001 From: Olivia Banks Date: Tue, 4 Nov 2025 10:16:59 -0700 Subject: [PATCH 1/5] [cherrypick] fix variable expansion issue #145 --- benches/parse.rs | 7 +++-- src/eval.rs | 6 +++++ src/load.rs | 59 ++++++++++++++++++++++++++----------------- src/parse.rs | 6 +++++ tests/e2e/bindings.rs | 13 +++++----- 5 files changed, 59 insertions(+), 32 deletions(-) diff --git a/benches/parse.rs b/benches/parse.rs index 2886685d..625a0aa2 100644 --- a/benches/parse.rs +++ b/benches/parse.rs @@ -1,5 +1,6 @@ use divan::Bencher; -use std::{io::Write, path::PathBuf, str::FromStr}; +use std::io::Write; +use std::path::PathBuf; fn generate_build_ninja(statement_count: usize) -> Vec { let mut buf: Vec = Vec::new(); @@ -55,8 +56,10 @@ fn load_synthetic(bencher: Bencher) { input.push(0); bencher.bench_local(|| { let mut loader = n2::load::Loader::new(); + let mut parser = n2::parse::Parser::new(&input); + loader - .parse(PathBuf::from_str("build.ninja").unwrap(), &input) + .parse_with_parser(&mut parser, PathBuf::from(""), &[]) .unwrap(); }); } diff --git a/src/eval.rs b/src/eval.rs index 221ee63d..79ef0397 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -128,10 +128,16 @@ impl<'text> Vars<'text> { pub fn insert(&mut self, key: &'text str, val: String) { self.0.insert(key, val); } + pub fn get(&self, key: &str) -> Option<&String> { self.0.get(key) } + + pub fn get_all(&self) -> &FxHashMap<&'text str, String> { + &self.0 + } } + impl<'a> Env for Vars<'a> { fn get_var(&self, var: &str) -> Option>> { Some(EvalString::new(vec![EvalPart::Literal( diff --git a/src/load.rs b/src/load.rs index 4fc3845e..b5272a35 100644 --- a/src/load.rs +++ b/src/load.rs @@ -173,29 +173,23 @@ impl Loader { self.graph.add_build(build) } - fn read_file(&mut self, id: FileId) -> anyhow::Result<()> { + pub fn read_file_by_id(&self, id: FileId) -> anyhow::Result<(PathBuf, Vec)> { let path = self.graph.file(id).path().to_path_buf(); - let bytes = match trace::scope("read file", || scanner::read_file_with_nul(&path)) { - Ok(b) => b, + + match trace::scope("read file", || scanner::read_file_with_nul(&path)) { + Ok(b) => Ok((path, b)), Err(e) => bail!("read {}: {}", path.display(), e), - }; - self.parse(path, &bytes) + } } - fn evaluate_and_read_file( + pub fn parse_with_parser( &mut self, - file: EvalString<&str>, + parser: &mut parse::Parser, + path: PathBuf, envs: &[&dyn eval::Env], ) -> anyhow::Result<()> { - let evaluated = self.evaluate_path(file, envs); - self.read_file(evaluated) - } - - pub fn parse(&mut self, path: PathBuf, bytes: &[u8]) -> anyhow::Result<()> { let filename = std::rc::Rc::new(path); - let mut parser = parse::Parser::new(&bytes); - loop { let stmt = match parser .read() @@ -204,18 +198,26 @@ impl Loader { None => break, Some(s) => s, }; + match stmt { - Statement::Include(id) => trace::scope("include", || { - self.evaluate_and_read_file(id, &[&parser.vars]) - })?, - // TODO: implement scoping for subninja - Statement::Subninja(id) => trace::scope("subninja", || { - self.evaluate_and_read_file(id, &[&parser.vars]) - })?, + // TODO: Support new scope for 'subninja' statements. + // This will require, at the very least, merging the + // variables of the sub-parser back into the parent parser. + Statement::Include(in_path) | Statement::Subninja(in_path) => { + let id = self.evaluate_path(in_path, &[&parser.vars]); + let (path, bytes) = self.read_file_by_id(id)?; + let bytes = std::rc::Rc::new(bytes); + let mut sub_parser = parse::Parser::new(&bytes); + + sub_parser.inherit(&parser); + self.parse_with_parser(&mut sub_parser, path, envs)?; + } + Statement::Default(defaults) => { let evaluated = self.evaluate_paths(defaults, &[&parser.vars]); self.default.extend(evaluated); } + Statement::Rule(rule) => { let mut vars: SmallMap> = SmallMap::default(); for (name, val) in rule.vars.into_iter() { @@ -226,12 +228,15 @@ impl Loader { } self.rules.insert(rule.name.to_owned(), vars); } + Statement::Build(build) => self.add_build(filename.clone(), &parser.vars, build)?, + Statement::Pool(pool) => { self.pools.insert(pool.name.to_string(), pool.depth); } }; } + self.builddir = parser.vars.get("builddir").cloned(); Ok(()) } @@ -254,8 +259,12 @@ pub fn read(build_filename: &str) -> anyhow::Result { .graph .files .id_from_canonical(to_owned_canon_path(build_filename)); - loader.read_file(id) + let (path, bytes) = loader.read_file_by_id(id)?; + let mut parser = parse::Parser::new(&bytes); + + loader.parse_with_parser(&mut parser, path, &[]) })?; + let mut hashes = graph::Hashes::default(); let db = trace::scope("db::open", || { let mut db_path = PathBuf::from(".n2_db"); @@ -268,6 +277,7 @@ pub fn read(build_filename: &str) -> anyhow::Result { db::open(&db_path, &mut loader.graph, &mut hashes) }) .map_err(|err| anyhow!("load .n2_db: {}", err))?; + Ok(State { graph: loader.graph, db, @@ -282,8 +292,11 @@ pub fn read(build_filename: &str) -> anyhow::Result { pub fn parse(name: &str, mut content: Vec) -> anyhow::Result { content.push(0); let mut loader = Loader::new(); + trace::scope("loader.read_file", || { - loader.parse(PathBuf::from(name), &content) + let mut parser = parse::Parser::new(&content); + loader.parse_with_parser(&mut parser, PathBuf::from(name), &[]) })?; + Ok(loader.graph) } diff --git a/src/parse.rs b/src/parse.rs index 6779bc3a..7e160ffe 100644 --- a/src/parse.rs +++ b/src/parse.rs @@ -66,6 +66,12 @@ impl<'text> Parser<'text> { } } + pub fn inherit<'b>(&mut self, from: &'b Self) { + for (k, v) in from.vars.get_all() { + self.vars.insert(k, v.clone()); + } + } + pub fn format_parse_error(&self, filename: &Path, err: ParseError) -> String { self.scanner.format_parse_error(filename, err) } diff --git a/tests/e2e/bindings.rs b/tests/e2e/bindings.rs index eebfd954..c46d9ec5 100644 --- a/tests/e2e/bindings.rs +++ b/tests/e2e/bindings.rs @@ -153,12 +153,13 @@ build foo: write_file #[test] fn across_files() -> anyhow::Result<()> { let space = TestSpace::new()?; + space.write("world.txt", "")?; space.write( "build.ninja", &[ ECHO_RULE, " -var = hello +ext = txt include other.ninja ", ] @@ -167,15 +168,13 @@ include other.ninja space.write( "other.ninja", " -build out: echo $var/world +build hello: echo world.$ext + text = what a beautiful day ", )?; - let out = space.run(&mut n2_command(vec!["out"]))?; - assert_output_contains(&out, "input /world missing"); + let out = space.run_expect(&mut n2_command(vec!["hello"]))?; + assert_output_contains(&out, "what a beautiful day"); - // TODO: should instead be something like: - // let out = space.run_expect(&mut n2_command(vec!["out"]))?; - // assert_output_contains(&out, "echo hello/world"); Ok(()) } From 13a5fe4ec18d76bd75dc3ceddb406b4553127dd3 Mon Sep 17 00:00:00 2001 From: Olivia Banks Date: Tue, 4 Nov 2025 10:37:23 -0700 Subject: [PATCH 2/5] [loader] change lie in TODO comment --- src/load.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/load.rs b/src/load.rs index b5272a35..9fa26646 100644 --- a/src/load.rs +++ b/src/load.rs @@ -200,7 +200,7 @@ impl Loader { }; match stmt { - // TODO: Support new scope for 'subninja' statements. + // TODO: Support new scope for 'include' statements. // This will require, at the very least, merging the // variables of the sub-parser back into the parent parser. Statement::Include(in_path) | Statement::Subninja(in_path) => { From a3c4b02a019d6f78d89b185cffa4ddd3d0fc5f8e Mon Sep 17 00:00:00 2001 From: Olivia Banks Date: Mon, 10 Nov 2025 10:05:28 -0700 Subject: [PATCH 3/5] [revert] 13a5fe4 --- src/load.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/load.rs b/src/load.rs index 9fa26646..b5272a35 100644 --- a/src/load.rs +++ b/src/load.rs @@ -200,7 +200,7 @@ impl Loader { }; match stmt { - // TODO: Support new scope for 'include' statements. + // TODO: Support new scope for 'subninja' statements. // This will require, at the very least, merging the // variables of the sub-parser back into the parent parser. Statement::Include(in_path) | Statement::Subninja(in_path) => { From ae4b32af5a75f750f2b7d451821dde76fe70f75d Mon Sep 17 00:00:00 2001 From: Olivia Banks Date: Mon, 10 Nov 2025 10:06:32 -0700 Subject: [PATCH 4/5] [todo] improve future inherit function --- src/parse.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/parse.rs b/src/parse.rs index 7e160ffe..71a3ea17 100644 --- a/src/parse.rs +++ b/src/parse.rs @@ -67,6 +67,7 @@ impl<'text> Parser<'text> { } pub fn inherit<'b>(&mut self, from: &'b Self) { + // TODO: should use from parser as a scope rather than copying. for (k, v) in from.vars.get_all() { self.vars.insert(k, v.clone()); } From 5f00e887fc06425d9f280e564c10e9cbdd03e097 Mon Sep 17 00:00:00 2001 From: Olivia Banks Date: Mon, 10 Nov 2025 10:07:03 -0700 Subject: [PATCH 5/5] [todo] remove for subninja scope support Removed TODO comments regarding subninja statement scope support. --- src/load.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/load.rs b/src/load.rs index b5272a35..c0892e31 100644 --- a/src/load.rs +++ b/src/load.rs @@ -200,9 +200,6 @@ impl Loader { }; match stmt { - // TODO: Support new scope for 'subninja' statements. - // This will require, at the very least, merging the - // variables of the sub-parser back into the parent parser. Statement::Include(in_path) | Statement::Subninja(in_path) => { let id = self.evaluate_path(in_path, &[&parser.vars]); let (path, bytes) = self.read_file_by_id(id)?;