Improve MAKEFLAGS merging#168
Conversation
|
Welcome @orlp! It looks like this is your first PR to tikv/jemallocator 🎉 |
|
Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits. 📝 Please follow instructions in the contributing guide to update your commits with the DCO Full details of the Developer Certificate of Origin can be found at developercertificate.org. The list of commits missing DCO signoff:
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
📝 WalkthroughWalkthroughThe build script now correctly merges ChangesMAKEFLAGS Parsing and Merging
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jemalloc-sys/build.rs`:
- Around line 446-451: The current parallelism detection uses f.contains("-j")
on the entire MAKEFLAGS (in the makeflags -> already_parallel logic) which can
false-positive on arguments after the "--" separator; change the check to only
inspect the options portion before the first "--" (split MAKEFLAGS on "--" and
use the first segment) and then search that options segment for "-j" or the long
form like "--jobserver-"; update the condition that sets already_parallel (and
keep cmd.env("MAKEFLAGS", &f) unchanged) to use this filtered segment when
deciding whether to treat make as already parallelized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| let already_parallel = if let Some(f) = makeflags { | ||
| cmd.env("MAKEFLAGS", &f); | ||
| f.contains("-j") // Matches both -j and --jobserver-*. | ||
| } else { | ||
| false | ||
| }; |
There was a problem hiding this comment.
Parallelism detection can produce false positives.
The substring check f.contains("-j") searches the entire MAKEFLAGS string, including post-dash arguments. If a target or file after -- contains the substring -j (e.g., MAKEFLAGS="-- -junk-target"), the check incorrectly returns true, causing the code to skip adding -j when parallelism is not actually configured.
🔍 Proposed fix to check only the options portion
- let already_parallel = if let Some(f) = makeflags {
+ let already_parallel = if let Some(ref f) = makeflags {
cmd.env("MAKEFLAGS", &f);
- f.contains("-j") // Matches both -j and --jobserver-*.
+ let (options, _) = parse_makeflags(f);
+ options.iter().any(|opt| opt.starts_with("-j") || opt.starts_with("--jobserver"))
} else {
false
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jemalloc-sys/build.rs` around lines 446 - 451, The current parallelism
detection uses f.contains("-j") on the entire MAKEFLAGS (in the makeflags ->
already_parallel logic) which can false-positive on arguments after the "--"
separator; change the check to only inspect the options portion before the first
"--" (split MAKEFLAGS on "--" and use the first segment) and then search that
options segment for "-j" or the long form like "--jobserver-"; update the
condition that sets already_parallel (and keep cmd.env("MAKEFLAGS", &f)
unchanged) to use this filtered segment when deciding whether to treat make as
already parallelized.
There was a problem hiding this comment.
I'm aware but can't be arsed to do something 'proper' - I think this already takes up more resources than is deserved.
There was a problem hiding this comment.
@orlp Fair enough — the scenario where a post--- argument happens to contain -j is admittedly quite contrived in practice. Happy to leave it as-is.
(ʕ•ᴥ•ʔ)ᵒᵏ
1 participant
Fixes #167.
Summary by CodeRabbit