From 8a13cff3d2481c03119d7e6dd16118e74711ab37 Mon Sep 17 00:00:00 2001 From: Jeff Garzik Date: Sun, 8 Feb 2026 07:40:53 +0000 Subject: [PATCH 1/2] make: Fix intermittent dash_r_with_file test failure and inference rule handling Separate inference rules from regular rules to prevent CWD scanning from picking up files created by parallel tests. Add two-pass makefile classification so .SUFFIXES is available during inference rule detection. Add run_for_target() to compute input/output from target name directly. Fix test isolation: rename precious target to avoid text.txt conflict with async_events, use unique .sfx/.xfo suffixes for suffixes_basic test, restore hijacked silent test and missing dash_p_with_mk test, fix orphaned code blocks and escaped assertion strings in integration tests. Co-Authored-By: Claude Opus 4.6 --- make/src/lib.rs | 140 +++++++++++++++--- make/src/rule.rs | 71 +++++++-- make/tests/integration.rs | 124 ++++++++++++---- .../inference_rules/dash_r_suffix.mk | 7 + .../inference_rules/not_default_target.mk | 7 + .../inference_rules/target_no_commands.mk | 7 + .../precious/basic_precious.mk | 4 +- .../suffixes/suffixes_basic.mk | 8 +- 8 files changed, 300 insertions(+), 68 deletions(-) create mode 100644 make/tests/makefiles/inference_rules/dash_r_suffix.mk create mode 100644 make/tests/makefiles/inference_rules/not_default_target.mk create mode 100644 make/tests/makefiles/inference_rules/target_no_commands.mk diff --git a/make/src/lib.rs b/make/src/lib.rs index 74abb027c..5ea37219b 100644 --- a/make/src/lib.rs +++ b/make/src/lib.rs @@ -39,7 +39,12 @@ const DEFAULT_SHELL: &str = "/bin/sh"; /// The only way to create a Make is from a Makefile and a Config. pub struct Make { macros: Vec, + /// Target rules (non-special, non-inference). + /// Invariant: inference rules are never stored here, so `first_target()` + /// always returns a valid default target per POSIX. rules: Vec, + /// Inference rules (e.g. `.c.o:`, `.txt.out:`). + inference_rules: Vec, default_rule: Option, // .DEFAULT pub config: Config, } @@ -58,10 +63,52 @@ impl Make { } pub fn first_target(&self) -> Result<&Target, ErrorCode> { - let rule = self.rules.first().ok_or(NoTarget { target: None })?; + // Per POSIX: "the first target that make encounters that is not a special + // target or an inference rule shall be used." + // If there are no non-special, non-inference targets, fall back to the + // first inference rule (which will scan CWD for matching files). + let rule = self + .rules + .first() + .or_else(|| self.inference_rules.first()) + .ok_or(NoTarget { target: None })?; rule.targets().next().ok_or(NoTarget { target: None }) } + /// Finds a matching inference rule for the given target name. + /// + /// Per POSIX: the suffix of the target (.s1) is compared to .SUFFIXES. + /// If found, inference rules are searched for the first .s2.s1 rule whose + /// prerequisite file ($*.s2) exists. + fn find_inference_rule(&self, name: &str) -> Option<&Rule> { + let suffixes = self.config.rules.get(".SUFFIXES")?; + + // Find the target's suffix (.s1) + let target_suffix = suffixes + .iter() + .filter(|s| name.ends_with(s.as_str())) + .max_by_key(|s| s.len())?; + + let stem = &name[..name.len() - target_suffix.len()]; + + // Search inference rules for .s2.s1 where $*.s2 exists + for rule in &self.inference_rules { + let Some(rule_target) = rule.targets().next() else { + continue; + }; + if let Target::Inference { from, to, .. } = rule_target { + let expected_suffix = format!(".{}", to); + if expected_suffix == *target_suffix { + let prereq_path = format!("{}.{}", stem, from); + if std::path::Path::new(&prereq_path).exists() { + return Some(rule); + } + } + } + } + None + } + /// Builds the target with the given name. /// /// # Returns @@ -69,14 +116,31 @@ impl Make { /// - Ok(false) if the target was already up to date. /// - Err(_) if any errors occur. pub fn build_target(&self, name: impl AsRef) -> Result { + // Search both regular rules and inference rules let rule = match self.rule_by_target_name(&name) { Some(rule) => rule, - None => match &self.default_rule { + None => match self + .inference_rules + .iter() + .find(|rule| rule.targets().any(|t| t.as_ref() == name.as_ref())) + { Some(rule) => rule, None => { - return Err(NoTarget { - target: Some(name.as_ref().to_string()), - }) + // Per POSIX: "If a target exists and there is neither a target rule + // nor an inference rule for the target, the target shall be considered + // up-to-date." + if get_modified_time(&name).is_some() { + return Ok(false); + } + // No rule and file doesn't exist - try .DEFAULT or fail + match &self.default_rule { + Some(rule) => rule, + None => { + return Err(NoTarget { + target: Some(name.as_ref().to_string()), + }) + } + } } }, }; @@ -111,6 +175,18 @@ impl Make { for prerequisite in &newer_prerequisites { self.build_target(prerequisite)?; } + + // Per POSIX: "When no target rule with commands is found to update a + // target, the inference rules shall be checked." If the matched target + // rule has no recipes, look for a matching inference rule and run it + // for this specific target instead. + if rule.recipes().count() == 0 { + if let Some(inference_rule) = self.find_inference_rule(target.as_ref()) { + inference_rule.run_for_target(&self.config, &self.macros, target, up_to_date)?; + return Ok(true); + } + } + rule.run(&self.config, &self.macros, target, up_to_date)?; Ok(true) @@ -184,32 +260,60 @@ impl TryFrom<(Makefile, Config)> for Make { type Error = ErrorCode; fn try_from((makefile, config): (Makefile, Config)) -> Result { - let mut rules = vec![]; - let mut special_rules = vec![]; - let mut inference_rules = vec![]; - - for rule in makefile.rules() { - let rule = Rule::from(rule); + // Two-pass classification: .SUFFIXES must be processed before inference + // rule classification so that user-defined suffixes (especially with -r) + // are available when determining whether a rule like `.txt.out:` is an + // inference rule. + + let mut suffixes_rules = vec![]; + let mut remaining_parsed_rules = vec![]; + + // Pass 1: Separate .SUFFIXES rules from everything else and process + // them immediately so config.rules[".SUFFIXES"] is populated. + for parsed_rule in makefile.rules() { + let rule = Rule::from(parsed_rule); let Some(target) = rule.targets().next() else { return Err(NoTarget { target: None }); }; - - if SpecialTarget::try_from(target.clone()).is_ok() { - special_rules.push(rule); - } else if InferenceTarget::try_from((target.clone(), config.clone())).is_ok() { - inference_rules.push(rule); + if let Ok(SpecialTarget::Suffixes) = SpecialTarget::try_from(target.clone()) { + suffixes_rules.push(rule); } else { - rules.push(rule); + remaining_parsed_rules.push(rule); } } + // Build the Make struct early so we can process .SUFFIXES via the + // normal special_target::process path (which writes to make.config). let mut make = Self { - rules, + rules: vec![], + inference_rules: vec![], macros: makefile.variable_definitions().collect(), default_rule: None, config, }; + for rule in suffixes_rules { + special_target::process(rule, &mut make)?; + } + + // Pass 2: Classify remaining rules. Now make.config.rules[".SUFFIXES"] + // contains both built-in (unless -r) and user-defined suffixes. + let mut special_rules = vec![]; + + for rule in remaining_parsed_rules { + let Some(target) = rule.targets().next() else { + return Err(NoTarget { target: None }); + }; + + if SpecialTarget::try_from(target.clone()).is_ok() { + special_rules.push(rule); + } else if InferenceTarget::try_from((target.clone(), make.config.clone())).is_ok() { + make.inference_rules.push(rule); + } else { + make.rules.push(rule); + } + } + for rule in special_rules { special_target::process(rule, &mut make)?; } diff --git a/make/src/rule.rs b/make/src/rule.rs index fe25cb045..6dd69b8fd 100644 --- a/make/src/rule.rs +++ b/make/src/rule.rs @@ -66,6 +66,37 @@ impl Rule { self.recipes.iter() } + /// Runs an inference rule for a specific target (not a CWD scan). + /// + /// This is used when POSIX requires applying an inference rule to a specific + /// target that has no commands of its own. The internal macros ($<, $*, etc.) + /// are substituted based on the target name and the inference rule's suffixes. + pub fn run_for_target( + &self, + global_config: &GlobalConfig, + macros: &[VariableDefinition], + target: &Target, + up_to_date: bool, + ) -> Result<(), ErrorCode> { + // For an inference rule applied to a specific target, compute the + // input/output pair from the target name and the rule's suffixes. + let files = if let Some(Target::Inference { from, to, .. }) = self.targets().next() { + let target_name = target.as_ref(); + let expected_suffix = format!(".{}", to); + if let Some(stem) = target_name.strip_suffix(&expected_suffix) { + let input = PathBuf::from(format!("{}.{}", stem, from)); + let output = PathBuf::from(target_name); + vec![(input, output)] + } else { + vec![(PathBuf::from(""), PathBuf::from(""))] + } + } else { + vec![(PathBuf::from(""), PathBuf::from(""))] + }; + + self.run_with_files(global_config, macros, target, up_to_date, files) + } + /// Runs the rule with the global config and macros passed in. /// /// Returns `Ok` on success and `Err` on any errors while running the rule. @@ -75,6 +106,32 @@ impl Rule { macros: &[VariableDefinition], target: &Target, up_to_date: bool, + ) -> Result<(), ErrorCode> { + let files = match target { + Target::Inference { from, to, .. } => find_files_with_extension(from)? + .into_iter() + .map(|input| { + let mut output = input.clone(); + output.set_extension(to); + (input, output) + }) + .collect::>(), + _ => { + vec![(PathBuf::from(""), PathBuf::from(""))] + } + }; + + self.run_with_files(global_config, macros, target, up_to_date, files) + } + + /// Internal helper: runs the rule's recipes for the given input/output file pairs. + fn run_with_files( + &self, + global_config: &GlobalConfig, + macros: &[VariableDefinition], + target: &Target, + up_to_date: bool, + files: Vec<(PathBuf, PathBuf)>, ) -> Result<(), ErrorCode> { let GlobalConfig { ignore: global_ignore, @@ -97,20 +154,6 @@ impl Rule { phony: _, } = self.config; - let files = match target { - Target::Inference { from, to, .. } => find_files_with_extension(from)? - .into_iter() - .map(|input| { - let mut output = input.clone(); - output.set_extension(to); - (input, output) - }) - .collect::>(), - _ => { - vec![(PathBuf::from(""), PathBuf::from(""))] - } - }; - for inout in files { for recipe in self.recipes() { let RecipeConfig { diff --git a/make/tests/integration.rs b/make/tests/integration.rs index 23e9d5183..b44e319c1 100644 --- a/make/tests/integration.rs +++ b/make/tests/integration.rs @@ -7,7 +7,6 @@ // SPDX-License-Identifier: MIT // -use std::env; use std::fs::{remove_file, File}; use std::io::Write; use std::process::{Child, Command, Stdio}; @@ -160,17 +159,6 @@ mod arguments { 0, ) } - - #[test] - fn dash_r() { - run_test_helper( - &["-r"], - "", - "make: no makefile\n", - ErrorCode::NoMakefile.into(), - ) - } - #[test] fn dash_r_with_file() { run_test_helper_with_setup_and_destruct( @@ -182,12 +170,22 @@ mod arguments { File::create("testfile.txt").expect("failed to create file"); }, || { - remove_file("testfile.txt").expect("failed to remove file"); - remove_file("testfile.out").expect("failed to remove file"); + let _ = remove_file("testfile.txt"); + let _ = remove_file("testfile.out"); }, ); } + #[test] + fn dash_r() { + run_test_helper( + &["-r"], + "", + "make: no makefile\n", + ErrorCode::NoMakefile.into(), + ) + } + #[test] fn dash_i() { run_test_helper( @@ -449,6 +447,9 @@ mod target_behavior { #[test] fn async_events() { + // Clean up any leftover files from previous test runs + let _ = remove_file("text.txt"); + let args = [ "-f", "tests/makefiles/target_behavior/async_events/signal.mk", @@ -472,6 +473,10 @@ mod target_behavior { assert_eq!(stderr, "make: Interrupt\nmake: Deleting file 'text.txt'\n"); assert_eq!(output.status.code(), Some(130)); + + // The makefile creates text.txt and the signal handler should delete it, + // but clean up anyway in case test fails + let _ = remove_file("text.txt"); } } @@ -624,6 +629,11 @@ mod special_targets { #[test] fn precious() { + // Clean up any leftover files from previous test runs + let _ = remove_file("precious_text.txt"); + let _ = remove_file("preciousdir/some.txt"); + let _ = remove_dir("preciousdir"); + let args = [ "-f", "tests/makefiles/special_targets/precious/basic_precious.mk", @@ -653,6 +663,7 @@ mod special_targets { assert_eq!(output.status.code(), Some(130)); + let _ = remove_file("precious_text.txt"); remove_file("preciousdir/some.txt").unwrap(); remove_dir("preciousdir").unwrap(); } @@ -664,33 +675,23 @@ mod special_targets { "-f", "tests/makefiles/special_targets/suffixes/suffixes_basic.mk", ], - "Converting copied.txt to copied.out\n", + "Converting suffixes_test.sfx to suffixes_test.xfo\n", "", 0, - create_txt, + create_file, remove_files, ); - fn create_txt() { - let dir = env::current_dir().unwrap(); - for file in dir.read_dir().unwrap().map(Result::unwrap) { - if !file.file_type().map(|x| x.is_file()).unwrap_or(false) { - continue; - } - if file.path().extension().map(|x| x == "txt").unwrap_or(false) { - remove_file(file.path()).unwrap(); - } - } - - File::create("copied.txt") + fn create_file() { + File::create("suffixes_test.sfx") .unwrap() .write_all(b"some content") .unwrap(); } fn remove_files() { - remove_file("copied.txt").unwrap(); - remove_file("copied.out").unwrap(); + let _ = remove_file("suffixes_test.sfx"); + let _ = remove_file("suffixes_test.xfo"); } } @@ -785,3 +786,66 @@ mod special_targets { } } } + +mod inference_rules { + use super::*; + + #[test] + fn dash_r_suffix_classification() { + // Test that -r flag + user-defined .SUFFIXES + inference rule works correctly. + // This directly tests the original bug: inference rules must be classified + // correctly even when -r clears built-in suffixes. + run_test_helper_with_setup_and_destruct( + &["-rf", "tests/makefiles/inference_rules/dash_r_suffix.mk"], + "Converting dash_r_test.txt to dash_r_test.out\n", + "", + 0, + || { + File::create("dash_r_test.txt").expect("failed to create file"); + }, + || { + let _ = remove_file("dash_r_test.txt"); + let _ = remove_file("dash_r_test.out"); + }, + ); + } + + #[test] + fn target_no_commands_uses_inference() { + // Test that a target rule with no commands triggers inference rule lookup. + // Per POSIX: "When no target rule with commands is found to update a + // target, the inference rules shall be checked." + run_test_helper_with_setup_and_destruct( + &[ + "-sf", + "tests/makefiles/inference_rules/target_no_commands.mk", + ], + "Inference applied: inference_test.src -> inference_test.dst\n", + "", + 0, + || { + File::create("inference_test.src").expect("failed to create file"); + }, + || { + let _ = remove_file("inference_test.src"); + let _ = remove_file("inference_test.dst"); + }, + ); + } + + #[test] + fn inference_rule_not_default_target() { + // Test that inference rules are never selected as the default target. + // Per POSIX: "the first target that make encounters that is not a + // special target or an inference rule shall be used." + run_test_helper( + &[ + "-sf", + "tests/makefiles/inference_rules/not_default_target.mk", + ], + "CORRECT: real target ran\n", + "", + 0, + ); + } +} diff --git a/make/tests/makefiles/inference_rules/dash_r_suffix.mk b/make/tests/makefiles/inference_rules/dash_r_suffix.mk new file mode 100644 index 000000000..d5f7e5247 --- /dev/null +++ b/make/tests/makefiles/inference_rules/dash_r_suffix.mk @@ -0,0 +1,7 @@ +.SUFFIXES: .txt .out + +.txt.out: + @echo "Converting $< to $@" + @cp $< $@ + +dash_r_test.out: dash_r_test.txt diff --git a/make/tests/makefiles/inference_rules/not_default_target.mk b/make/tests/makefiles/inference_rules/not_default_target.mk new file mode 100644 index 000000000..78e9f7aa4 --- /dev/null +++ b/make/tests/makefiles/inference_rules/not_default_target.mk @@ -0,0 +1,7 @@ +.SUFFIXES: .a .b + +.a.b: + @echo "WRONG: inference rule ran as default" + +real_target: + @echo "CORRECT: real target ran" diff --git a/make/tests/makefiles/inference_rules/target_no_commands.mk b/make/tests/makefiles/inference_rules/target_no_commands.mk new file mode 100644 index 000000000..b4c824ab8 --- /dev/null +++ b/make/tests/makefiles/inference_rules/target_no_commands.mk @@ -0,0 +1,7 @@ +.SUFFIXES: .src .dst + +.src.dst: + @echo "Inference applied: $< -> $@" + @cp $< $@ + +inference_test.dst: inference_test.src diff --git a/make/tests/makefiles/special_targets/precious/basic_precious.mk b/make/tests/makefiles/special_targets/precious/basic_precious.mk index 39a7da28b..7a8b5b0b5 100644 --- a/make/tests/makefiles/special_targets/precious/basic_precious.mk +++ b/make/tests/makefiles/special_targets/precious/basic_precious.mk @@ -1,5 +1,5 @@ -.PRECIOUS: -text.txt: +.PRECIOUS: +precious_text.txt: echo hello mkdir preciousdir touch preciousdir/some.txt diff --git a/make/tests/makefiles/special_targets/suffixes/suffixes_basic.mk b/make/tests/makefiles/special_targets/suffixes/suffixes_basic.mk index ca34c1450..ebe359de5 100644 --- a/make/tests/makefiles/special_targets/suffixes/suffixes_basic.mk +++ b/make/tests/makefiles/special_targets/suffixes/suffixes_basic.mk @@ -1,7 +1,7 @@ -.SUFFIXES: .txt .out +.SUFFIXES: .sfx .xfo -.txt.out: - @echo "Converting copied.txt to copied.out" - @cp copied.txt copied.out +.sfx.xfo: + @echo "Converting suffixes_test.sfx to suffixes_test.xfo" + @cp suffixes_test.sfx suffixes_test.xfo From f4ebb6c60dd1ef698e761b2ddedd885f695f0b7f Mon Sep 17 00:00:00 2001 From: Jeff Garzik Date: Sun, 8 Feb 2026 11:58:40 +0000 Subject: [PATCH 2/2] make: Apply special target modifiers to inference rules too The additive() and global() methods only iterated over make.rules, so .SILENT, .IGNORE, .PRECIOUS, and .PHONY modifiers were not applied to inference rules stored in make.inference_rules. Chain both vecs. Co-Authored-By: Claude Opus 4.6 --- make/src/special_target.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/make/src/special_target.rs b/make/src/special_target.rs index ed4a4bfbd..cec4a0bf0 100644 --- a/make/src/special_target.rs +++ b/make/src/special_target.rs @@ -200,6 +200,7 @@ impl Processor<'_> { self.make .rules .iter_mut() + .chain(self.make.inference_rules.iter_mut()) .filter(|r| r.targets().any(|t| t.as_ref() == prerequisite.as_ref())) .for_each(f.clone()); } @@ -209,7 +210,11 @@ impl Processor<'_> { /// specified. fn global(&mut self, f: impl FnMut(&mut Rule) + Clone) { if self.rule.prerequisites().count() == 0 { - self.make.rules.iter_mut().for_each(f); + self.make + .rules + .iter_mut() + .chain(self.make.inference_rules.iter_mut()) + .for_each(f); } } }