Conversation
…le 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 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR improves make’s inference-rule handling and test isolation to address intermittent failures under parallel test execution, especially around -r behavior and suffix-based inference rules.
Changes:
- Separate inference rules from regular target rules, with a two-pass makefile classification so
.SUFFIXESis processed before inference-rule detection. - Add targeted inference execution (
Rule::run_for_target) and inference lookup for “target rule with no commands” cases. - Update and expand integration tests/makefiles to avoid cross-test filesystem collisions and to cover the fixed inference behaviors.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| make/src/lib.rs | Introduces inference_rules, two-pass rule classification, and inference-rule lookup/execution flow changes. |
| make/src/rule.rs | Adds run_for_target() and refactors rule execution to share a run_with_files() helper. |
| make/tests/integration.rs | Fixes test cleanup/isolation, restores/adjusts argument tests, and adds new inference-rule regression tests. |
| make/tests/makefiles/special_targets/suffixes/suffixes_basic.mk | Uses unique suffixes/filenames to avoid CWD collisions from inference scans. |
| make/tests/makefiles/special_targets/precious/basic_precious.mk | Renames the target to avoid conflict with other tests’ text.txt. |
| make/tests/makefiles/inference_rules/target_no_commands.mk | New fixture: explicit target with no commands should trigger inference lookup. |
| make/tests/makefiles/inference_rules/not_default_target.mk | New fixture: inference rules must not become the default target when a real target exists. |
| make/tests/makefiles/inference_rules/dash_r_suffix.mk | New fixture: -r + user .SUFFIXES must still classify and apply inference rules correctly. |
| // 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) |
There was a problem hiding this comment.
run_for_target() falls back to running the rule with empty input/output paths when the rule isn't an inference rule or when the target name doesn't match the rule's to suffix. That can cause recipes to execute with $</$* substituted as empty strings (and potentially run cp/rm against unintended paths) instead of failing fast. Consider returning an error (e.g., ErrorCode::NoTarget { target: Some(target_name.to_string()) }) or Ok(()) without running recipes when the suffix doesn't match, and treat a non-inference rule here as a programmer error.
| // 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) | |
| let target_name = target.as_ref(); | |
| // For an inference rule applied to a specific target, compute the | |
| // input/output pair from the target name and the rule's suffixes. | |
| if let Some(Target::Inference { from, to, .. }) = self.targets().next() { | |
| 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); | |
| let files = vec![(input, output)]; | |
| return self.run_with_files(global_config, macros, target, up_to_date, files); | |
| } | |
| // Suffix does not match this inference rule; do not run any recipes. | |
| return Ok(()); | |
| } | |
| // `run_for_target` should only be used with inference rules; treat other | |
| // usages as a programmer error and report a missing target. | |
| debug_assert!( | |
| false, | |
| "run_for_target called on non-inference rule for target {target_name}" | |
| ); | |
| Err(NoTarget { | |
| target: Some(target_name.to_string()), | |
| }) |
| /// Invariant: inference rules are never stored here, so `first_target()` | ||
| /// always returns a valid default target per POSIX. |
There was a problem hiding this comment.
The struct-level invariant comment says first_target() "always returns a valid default target per POSIX", but first_target() explicitly falls back to self.inference_rules.first() when there are no regular targets, which returns an inference rule target (and will scan the CWD). The comment should be updated to match the actual behavior (or the fallback behavior adjusted) so future changes don’t rely on an incorrect invariant.
| /// Invariant: inference rules are never stored here, so `first_target()` | |
| /// always returns a valid default target per POSIX. | |
| /// Invariant: inference rules are never stored here; they are kept | |
| /// separately in `inference_rules`. When there are no regular targets, | |
| /// `first_target()` may fall back to an inference rule instead. |
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 <noreply@anthropic.com>
…le 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.