passthrough cc env/args using native cc features#158
Conversation
|
Welcome @PDXKimani! It looks like this is your first PR to tikv/jemallocator 🎉 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates ChangesBuild Script Enhancement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Signed-off-by: Dylan Abraham <dylan@momentohq.com>
PDXKimani
force-pushed
the
cc-env-passthrough
branch
from
a1b43f0 to
fabd9f3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jemalloc-sys/build.rs`:
- Around line 158-160: The fallback for LDFLAGS incorrectly uses the CFLAGS
value (ldflags is set to cflags.clone()), which can pass compiler-only flags to
the linker; change the fallback in the read_and_watch_env("LDFLAGS")
unwrap_or_else to return an empty OsString instead of cflags.clone() so ldflags
only contains explicit linker flags; update the expression referencing
read_and_watch_env, ldflags, and cflags accordingly.
- Around line 157-169: The build script calls cc::Tool methods cc_env() and
cflags_env() which require cc >= 1.0.26, so update the build-dependency entry
for cc in Cargo.toml to require version 1.0.26 or later (e.g., change the
build-dependency from "cc = \"1.0.13\"" to "cc = \"^1.0.26\"") so the cc_env()
and cflags_env() methods used in build.rs are available at compile time.
| let cflags = compiler.cflags_env(); | ||
| let ldflags = read_and_watch_env("LDFLAGS") | ||
| .map(OsString::from) | ||
| .unwrap_or_else(|_| cflags.clone()); | ||
|
|
||
| // Use cc_env() to get the full CC value including any wrapper (e.g. sccache). | ||
| // cc_env() returns empty when no wrapper is configured, so fall back to path(). | ||
| let cc = compiler.cc_env(); | ||
| let cc = if cc.is_empty() { | ||
| compiler.path().as_os_str().to_owned() | ||
| } else { | ||
| cc | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
cc crate Rust cflags_env cc_env methods version
💡 Result:
In the Rust cc crate, cc_env and cflags_env are methods on cc::Tool (not on cc::Build):
Tool::cc_env() -> OsString: returns a string formatted like theCCenvironment variable (primarily useful whenCCis a wrapper likesccache cc), or an empty string if no such wrapper/CCenv was involved. [1]Tool::cflags_env() -> OsString: returns the tool’s computed compiler arguments formatted like theCFLAGSenvironment variable (note: it’s the tool’s internal args, not necessarily the user’sCFLAGS). [1]
Typical usage:
let tool = cc::Build::new().get_compiler();
let cc = tool.cc_env();
let cflags = tool.cflags_env();What version added these methods?
They were added after cc 1.0.0 and are present by cc 1.0.26:
- Absent in 1.0.0 (
Toolhas nocc_env/cflags_env). [2] - Present in 1.0.26 (
pub fn cc_envandpub fn cflags_envexist). [3]
So, if you need these APIs, require cc >= 1.0.26.
Sources:
[1] cc::Tool docs (latest) showing cc_env / cflags_env and their semantics. (docs.rs)
[2] cc-rs tag upstream/1.0.0 src/lib.rs (no cc_env / cflags_env). (chromium.googlesource.com)
[3] cc-rs tag upstream/1.0.26 src/lib.rs (includes cc_env / cflags_env). (chromium.googlesource.com)
Citations:
- 1: https://docs.rs/cc/latest/cc/struct.Tool.html?utm_source=openai
- 2: https://chromium.googlesource.com/external/github.com/alexcrichton/cc-rs/%2B/54c0d23086077bbc20303cc07299a4b757b42d72/src/lib.rs
- 3: https://chromium.googlesource.com/external/github.com/alexcrichton/cc-rs/%2B/refs/tags/upstream/1.0.26/src/lib.rs
🏁 Script executed:
find . -name "Cargo.toml" -type f | xargs grep -l "cc\s*=" | head -5Repository: tikv/jemallocator
Length of output: 110
🏁 Script executed:
find . -name "Cargo.toml" -type f -path "*/jemalloc-sys/*" -exec cat {} \;Repository: tikv/jemallocator
Length of output: 1635
Update cc dependency to require version 1.0.26 or later.
The code uses cc_env() and cflags_env() methods on cc::Tool, which were not available in cc 1.0.13 (the minimum version specified in Cargo.toml). These methods were added in cc 1.0.26. Update the build-dependency to cc = "^1.0.26" to ensure the methods are available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@jemalloc-sys/build.rs` around lines 157 - 169, The build script calls
cc::Tool methods cc_env() and cflags_env() which require cc >= 1.0.26, so update
the build-dependency entry for cc in Cargo.toml to require version 1.0.26 or
later (e.g., change the build-dependency from "cc = \"1.0.13\"" to "cc =
\"^1.0.26\"") so the cc_env() and cflags_env() methods used in build.rs are
available at compile time.
There was a problem hiding this comment.
These both exist in https://docs.rs/cc/1.0.13/cc/struct.Tool.html
There was a problem hiding this comment.
@PDXKimani, you are correct — I apologize for the incorrect review comment. The cc_env() and cflags_env() methods on cc::Tool are present even in cc 1.0.10, and per the docs you linked, they are definitely available in cc 1.0.13 as well. No version bump is needed. My earlier concern was mistaken.
✏️ Learnings added
Learnt from: PDXKimani
Repo: tikv/jemallocator PR: 158
File: jemalloc-sys/build.rs:157-169
Timestamp: 2026-03-17T18:53:45.020Z
Learning: The cc crate's Tool::cc_env() and Tool::cflags_env() methods are available since at least cc 1.0.10 (and are present in cc 1.0.13). Do not flag their usage as requiring a version bump from 1.0.13 or later in jemallocator/jemalloc-sys/build.rs.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
jemalloc-sys/build.rs (2)
157-169:⚠️ Potential issue | 🟠 MajorBump the
ccminimum version before usingTool::cc_env/Tool::cflags_env.Line 157 and Line 164 use APIs that are not guaranteed by a
cc = "1.0.13"floor. This can fail in minimal-version or older-lockfile builds; raise the build-dependency floor accordingly (e.g.,>=1.0.26).#!/bin/bash set -euo pipefail # Verify current cc build-dependency constraints in the repo fd Cargo.toml | while read -r file; do if rg -n '^\s*\[build-dependencies\]|^\s*cc\s*=' "$file" >/dev/null; then echo "== $file ==" rg -n -A3 -B2 '^\s*\[build-dependencies\]|^\s*cc\s*=' "$file" echo fi done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jemalloc-sys/build.rs` around lines 157 - 169, The build script uses newer Tool APIs (Tool::cc_env and Tool::cflags_env via compiler.cc_env() and compiler.cflags_env()) that require a newer cc crate than 1.0.13; update the build-dependencies entry for cc in Cargo.toml to a minimum version that includes these methods (e.g., >=1.0.26) so builds with minimal-version or older lockfiles succeed, then regenerate/update the lockfile (cargo update) to ensure the newer cc is used during builds.
158-160:⚠️ Potential issue | 🟠 MajorDon’t default
LDFLAGStoCFLAGS.Line 160 currently falls back to
cflags.clone(), which can inject compile-only flags into linker flags and break configure/link probes.Suggested fix
let ldflags = read_and_watch_env("LDFLAGS") .map(OsString::from) - .unwrap_or_else(|_| cflags.clone()); + .unwrap_or_default();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jemalloc-sys/build.rs` around lines 158 - 160, The code incorrectly falls back to using cflags for linker flags; change the ldflags assignment so read_and_watch_env("LDFLAGS") does NOT default to cflags.clone() — instead use an empty OsString (e.g., unwrap_or_else(|_| OsString::new())) or another linker-appropriate default; update the variable created by the call to read_and_watch_env to avoid injecting compile-only flags into linker flags (refer to read_and_watch_env, ldflags, and cflags).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@jemalloc-sys/build.rs`:
- Around line 157-169: The build script uses newer Tool APIs (Tool::cc_env and
Tool::cflags_env via compiler.cc_env() and compiler.cflags_env()) that require a
newer cc crate than 1.0.13; update the build-dependencies entry for cc in
Cargo.toml to a minimum version that includes these methods (e.g., >=1.0.26) so
builds with minimal-version or older lockfiles succeed, then regenerate/update
the lockfile (cargo update) to ensure the newer cc is used during builds.
- Around line 158-160: The code incorrectly falls back to using cflags for
linker flags; change the ldflags assignment so read_and_watch_env("LDFLAGS")
does NOT default to cflags.clone() — instead use an empty OsString (e.g.,
unwrap_or_else(|_| OsString::new())) or another linker-appropriate default;
update the variable created by the call to read_and_watch_env to avoid injecting
compile-only flags into linker flags (refer to read_and_watch_env, ldflags, and
cflags).
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Dylan Abraham <dylan@momentohq.com>
PDXKimani
force-pushed
the
cc-env-passthrough
branch
from
d0bf012 to
fbf26aa
Compare
kvcache
left a comment
kvcache
left a comment
There was a problem hiding this comment.
This is a cool update, and it improves our build times. Looking forward to input from maintainers!
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jemalloc-sys/build.rs (1)
157-160:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve explicit
CPPFLAGSoverrides.
configureconsumesCPPFLAGSseparately fromCFLAGS, but Line 210 always replaces any caller-providedCPPFLAGSwith thecc-derivedcflags. That drops preprocessor-only-I/-Doverrides during configure probes, which undercuts the env passthrough goal of this change.Suggested fix
let cflags = compiler.cflags_env(); let ldflags = read_and_watch_env("LDFLAGS") .map(OsString::from) .unwrap_or_default(); +let cppflags = read_and_watch_env("CPPFLAGS") + .map(OsString::from) + .unwrap_or_else(|_| cflags.clone()); // Use cc_env() to get the full CC value including any wrapper (e.g. sccache). // cc_env() returns empty when no wrapper is configured, so fall back to path(). let cc = compiler.cc_env(); @@ info!("CC={:?}", cc); info!("CFLAGS={:?}", cflags); info!("LDFLAGS={:?}", ldflags); +info!("CPPFLAGS={:?}", cppflags); @@ .env("CC", &cc) .env("CFLAGS", &cflags) .env("LDFLAGS", &ldflags) - .env("CPPFLAGS", &cflags) + .env("CPPFLAGS", &cppflags)Also applies to: 207-210
🤖 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 157 - 160, The build script currently overwrites caller-provided CPPFLAGS by assigning the cc-derived cflags to the CPPFLAGS slot; preserve explicit CPPFLAGS by reading read_and_watch_env("CPPFLAGS") into a new variable (e.g., cppflags) and only default to cflags when CPPFLAGS is not set (e.g., cppflags = read_and_watch_env("CPPFLAGS").map(OsString::from).unwrap_or_else(|| cflags.clone())); keep cflags and ldflags as-is, use cppflags for configure invocations, and apply the same change to the other occurrence mentioned (the block around the other 207-210 lines) so preprocessor-only -I/-D overrides are passed through.
🤖 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.
Outside diff comments:
In `@jemalloc-sys/build.rs`:
- Around line 157-160: The build script currently overwrites caller-provided
CPPFLAGS by assigning the cc-derived cflags to the CPPFLAGS slot; preserve
explicit CPPFLAGS by reading read_and_watch_env("CPPFLAGS") into a new variable
(e.g., cppflags) and only default to cflags when CPPFLAGS is not set (e.g.,
cppflags = read_and_watch_env("CPPFLAGS").map(OsString::from).unwrap_or_else(||
cflags.clone())); keep cflags and ldflags as-is, use cppflags for configure
invocations, and apply the same change to the other occurrence mentioned (the
block around the other 207-210 lines) so preprocessor-only -I/-D overrides are
passed through.
Signed-off-by: Jay Lee <busyjaylee@gmail.com>
3 participants
Use the cc-native
cflags_env()andcc_env()to generate the build command.cflags_env()is just a cc-native equivalent of what was being done manually with the args builder.cc_env()will capture whateverCCwas set to instead of dropping it and just setting the compiler path.Feature-wise, this means we'll pull through configured CC wrappers (e.g. ccache/sccache) and use them for the build.
Summary by CodeRabbit