From fbbcf325b8bbe72f924da6a7cdb128d973ef0026 Mon Sep 17 00:00:00 2001 From: Melody Horn Date: Mon, 11 Nov 2024 14:46:19 -0700 Subject: fix conditional assignment when original is inherited --- src/makefile/input.rs | 27 ++++++++++++++------------- src/makefile/macro.rs | 14 ++------------ src/makefile/macro_scope.rs | 9 +++++++-- src/makefile/mod.rs | 2 +- tests/conditional_assignment_inheritance.rs | 28 ++++++++++++++++++++++++++++ tests/update_logic.rs | 14 +++----------- tests/utils/mod.rs | 13 +++++++++++++ 7 files changed, 68 insertions(+), 39 deletions(-) create mode 100644 tests/conditional_assignment_inheritance.rs create mode 100644 tests/utils/mod.rs diff --git a/src/makefile/input.rs b/src/makefile/input.rs index 40720c2..6333a66 100644 --- a/src/makefile/input.rs +++ b/src/makefile/input.rs @@ -212,7 +212,8 @@ impl<'a, 'parent> MakefileReader<'a, 'parent, BufReader> { ) -> Result { let mut macros = MacroSet::new(); #[cfg(feature = "full")] - if let Some(mut old_makefile_list) = macros.pop("MAKEFILE_LIST") { + if let Some(old_makefile_list) = stack.get("MAKEFILE_LIST") { + let mut old_makefile_list = old_makefile_list.into_owned(); old_makefile_list.text.extend(TokenString::text(format!( " {}", path.as_ref().to_string_lossy() @@ -454,7 +455,7 @@ impl<'a, 'parent, R: BufRead> MakefileReader<'a, 'parent, R> { let action = line .action( self.conditional_stack.last(), - |name| self.macros.is_defined(name), + |name| self.stack.with_scope(&self.macros).is_defined(name), |t| self.expand_macros_deferred_eval(t, &mut deferred_eval_context), ) .wrap_err_with(|| { @@ -820,17 +821,16 @@ impl<'a, 'parent, R: BufRead> MakefileReader<'a, 'parent, R> { value }; - let skipped = match self.macros.get(name) { + let skipped = match self + .stack + .with_scope(&self.macros) + .get(name) + .map(|x| x.source.clone()) + { // We always let command line or MAKEFLAGS macros override macros from the file. - Some(Macro { - source: ItemSource::CommandLineOrMakeflags, - .. - }) => true, + Some(ItemSource::CommandLineOrMakeflags) => true, // We let environment variables override macros from the file only if the command-line argument to do that was given - Some(Macro { - source: ItemSource::Environment, - .. - }) => self.args.environment_overrides, + Some(ItemSource::Environment) => self.args.environment_overrides, Some(_) => skip_if_defined, None => false, }; @@ -846,8 +846,9 @@ impl<'a, 'parent, R: BufRead> MakefileReader<'a, 'parent, R> { &value ); - let value = match self.macros.pop(name) { - Some(mut old_value) if append => { + let value = match self.stack.with_scope(&self.macros).get(name) { + Some(old_value) if append => { + let mut old_value = old_value.into_owned(); #[cfg(feature = "full")] let value = if old_value.eagerly_expanded { TokenString::text(self.expand_macros(&value).wrap_err_with(|| { diff --git a/src/makefile/macro.rs b/src/makefile/macro.rs index 301173a..b2a1510 100644 --- a/src/makefile/macro.rs +++ b/src/makefile/macro.rs @@ -119,7 +119,8 @@ impl Set { } } - pub fn get(&self, name: &str) -> Option<&Macro> { + /// To properly process inherited macros, use [MacroScopeStack::get]. + pub fn get_non_recursive(&self, name: &str) -> Option<&Macro> { self.data.get(name) } @@ -127,17 +128,6 @@ impl Set { self.data.insert(name, r#macro); } - #[cfg(feature = "full")] - pub fn is_defined(&self, name: &str) -> bool { - self.get(name).map_or(false, |x| !x.text.is_empty()) - } - - // `remove` is fine, but I think for "remove-and-return" `pop` is better - pub fn pop(&mut self, name: &str) -> Option { - // TODO figure out a better way to handle inheritance - self.data.remove(name) - } - pub fn extend( &mut self, other: HashMap, diff --git a/src/makefile/macro_scope.rs b/src/makefile/macro_scope.rs index bee603e..ba9b6cf 100644 --- a/src/makefile/macro_scope.rs +++ b/src/makefile/macro_scope.rs @@ -26,7 +26,7 @@ pub trait MacroScope { impl MacroScope for MacroSet { fn get(&self, name: &str) -> Option> { - self.get(name).map(Cow::Borrowed) + self.get_non_recursive(name).map(Cow::Borrowed) } } @@ -82,7 +82,7 @@ impl<'a> MacroScopeStack<'a> { } } - fn get(&self, name: &str) -> Option> { + pub fn get(&self, name: &str) -> Option> { for scope in &self.scopes { if let Some(r#macro) = scope.get(name) { return Some(r#macro); @@ -91,6 +91,11 @@ impl<'a> MacroScopeStack<'a> { None } + #[cfg(feature = "full")] + pub fn is_defined(&self, name: &str) -> bool { + self.get(name).map_or(false, |x| !x.text.is_empty()) + } + pub fn expand<#[cfg(feature = "full")] R: BufRead>( &self, text: &TokenString, diff --git a/src/makefile/mod.rs b/src/makefile/mod.rs index ddfac6f..9b2f626 100644 --- a/src/makefile/mod.rs +++ b/src/makefile/mod.rs @@ -194,7 +194,7 @@ impl<'a> Makefile<'a> { let follow_gnu = cfg!(feature = "full"); - let vpath_options = match self.macros.get("VPATH") { + let vpath_options = match self.macros.get_non_recursive("VPATH") { Some(Macro { text, .. }) if follow_gnu => { let vpath = self.expand_macros(text, None)?; env::split_paths(&vpath).collect() diff --git a/tests/conditional_assignment_inheritance.rs b/tests/conditional_assignment_inheritance.rs new file mode 100644 index 0000000..cb7e806 --- /dev/null +++ b/tests/conditional_assignment_inheritance.rs @@ -0,0 +1,28 @@ +mod utils; + +use std::fs; +use utils::{make, R}; + +#[test] +#[cfg(feature = "full")] +fn conditional_assignment_inheritance_test() -> R { + let dir = tempfile::tempdir()?; + + let file_a = " +EGG = bug +include file_b.mk +check: +\t@echo $(EGG) +"; + fs::write(dir.path().join("Makefile"), file_a)?; + let file_b = " +EGG ?= nope +"; + fs::write(dir.path().join("file_b.mk"), file_b)?; + + let result = make(&dir)?; + assert!(result.status.success()); + assert_eq!(String::from_utf8(result.stdout)?.trim(), "bug"); + + Ok(()) +} diff --git a/tests/update_logic.rs b/tests/update_logic.rs index 9e37b19..0c97ad5 100644 --- a/tests/update_logic.rs +++ b/tests/update_logic.rs @@ -1,19 +1,11 @@ use std::fs; -use std::path::Path; -use std::process::{Command, Output}; -use eyre::{Result, WrapErr}; +use eyre::WrapErr; use std::thread::sleep; use std::time::Duration; -type R = Result<()>; - -fn make(dir: impl AsRef) -> Result { - Command::new(env!("CARGO_BIN_EXE_makers")) - .current_dir(dir) - .output() - .wrap_err("") -} +mod utils; +use utils::{make, R}; #[test] fn basic_test() -> R { diff --git a/tests/utils/mod.rs b/tests/utils/mod.rs new file mode 100644 index 0000000..8b834c1 --- /dev/null +++ b/tests/utils/mod.rs @@ -0,0 +1,13 @@ +use std::path::Path; +use std::process::{Command, Output}; + +use eyre::Context; + +pub type R = eyre::Result<()>; + +pub fn make(dir: impl AsRef) -> eyre::Result { + Command::new(env!("CARGO_BIN_EXE_makers")) + .current_dir(dir) + .output() + .wrap_err("") +} -- cgit v1.2.3