Skip to content

Improve MAKEFLAGS merging #168

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
orlp wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Improve MAKEFLAGS merging #168

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 53 additions & 10 deletions jemalloc-sys/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,21 +393,64 @@ fn main() {
}
}

/// Parses a MAKEFLAGS string into a vec of option arguments, and vec of
/// post-option arguments (those after --).
fn parse_makeflags(flags: &str) -> (Vec<String>, Vec<String>) {
let mut options = Vec::new();
let mut post_options = Vec::new();
let mut first = true;
let mut options_ended = false;
for grp in flags.trim().split_ascii_whitespace() {
if first && !grp.starts_with('-') {
// The first group of MAKEFLAGS is interpreted as prepended with -,
// if it wasn't already.
options.push(format!("-{grp}"));
} else if options_ended {
post_options.push(grp.to_string())
} else if grp == "--" {
options_ended = true;
} else {
options.push(grp.to_string())
}

first = false;
}
(options, post_options)
}

fn merge_makeflags(a: &str, b: &str) -> String {
let (mut opts, mut post_opts) = parse_makeflags(a);
let (b_opts, b_post_opts) = parse_makeflags(b);
opts.extend(b_opts);
post_opts.extend(b_post_opts);
if !post_opts.is_empty() {
opts.push("--".to_string());
opts.extend(post_opts);
}
opts.join(" ")
}

fn make_command(make_cmd: &str, build_dir: &Path, num_jobs: &str) -> Command {
let mut cmd = Command::new(make_cmd);
cmd.current_dir(build_dir);

if let Ok(makeflags) = std::env::var("CARGO_MAKEFLAGS") {
let makeflags = if let Ok(orig_makeflags) = std::env::var("MAKEFLAGS") {
// Prepend Cargo makeflags before externally configured makeflags
// Adding Cargo makeflags at the end was causing issues, see
// https://github.com/tikv/jemallocator/issues/92#issuecomment-3536269176.
format!("{makeflags} {orig_makeflags}")
} else {
makeflags
};
cmd.env("MAKEFLAGS", makeflags);
// Pass through MAKEFLAGS.
let makeflags = match (std::env::var("CARGO_MAKEFLAGS"), std::env::var("MAKEFLAGS")) {
// Merging makeflags is non-trivial, see:
// - https://github.com/tikv/jemallocator/issues/92#issuecomment-3536269176
// - https://github.com/tikv/jemallocator/issues/167#issuecomment-4678875839
(Ok(a), Ok(b)) => Some(merge_makeflags(&a, &b)),
(Ok(f), Err(_)) | (Err(_), Ok(f)) => Some(f),
_ => None
};
let already_parallel = if let Some(f) = makeflags {
cmd.env("MAKEFLAGS", &f);
f.contains("-j") // Matches both -j and --jobserver-*.
} else {
false
};
Comment on lines +446 to +451

@coderabbitai coderabbitai Bot Jun 11, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoff

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.

@orlp orlp Jun 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aware but can't be arsed to do something 'proper' - I think this already takes up more resources than is deserved.

@coderabbitai coderabbitai Bot Jun 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

(ʕ•ᴥ•ʔ)ᵒᵏ


if !already_parallel {
cmd.arg("-j").arg(num_jobs);
}
cmd
Expand Down