fix: Use Xcode's libclang on macOS#3557
Conversation
larseggert
requested review from
KershawChang,
martinthomson and
mxinden
as code owners
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3557 +/- ##
==========================================
- Coverage 94.57% 94.47% -0.11%
==========================================
Files 128 132 +4
Lines 39736 40066 +330
Branches 39736 40066 +330
==========================================
+ Hits 37581 37853 +272
- Misses 1317 1365 +48
- Partials 838 848 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good fix. The core change — moving setup_clang() from setup_standalone() to main() — is correct: build_bindings() always invokes bindgen regardless of the setup path (gecko, standalone, or pkg-config), so ensuring libclang is discoverable should be unconditional. The macOS-specific logic to pin Xcode's libclang is well-motivated by clang-sys's version-sorting behavior that can pick up a Homebrew LLVM lacking the macOS SDK include paths.
Two minor notes posted inline (both non-blocking).
There was a problem hiding this comment.
Pull request overview
This PR updates neqo-crypto’s build script to make bindgen/libclang discovery more reliable on macOS by preferring the libclang shipped with Xcode (avoiding Homebrew LLVM mismatches), and centralizes clang setup so it runs for all build modes.
Changes:
- Add macOS-specific logic to set
LIBCLANG_PATHbased onxcode-select --print-path. - Move
setup_clang()invocation tomain()so it runs for all configuration paths (gecko, NSS_DIR, pkg-config). - Narrow the Gecko/mozbuild-derived libclang setup to Windows-only logic.
There was a problem hiding this comment.
Good change — the core fix (detecting and preferring Xcode's libclang over Homebrew's on macOS) addresses a real build breakage, and the restructuring into per-platform branches is clearer than the old code.
Two architectural observations:
-
setup_clang()now runs for all build paths (gecko, standalone, pkg-config) instead of only standalone. This is correct — the old function was a no-op on non-Windows anyway (early return forOS != "windows"), and now it actually provides value on macOS regardless of which NSS discovery path is used. -
Several pre-existing Cargo directive bugs survive in the Windows branch: missing
cargo:prefixes onrerun-if-env-changed(line 103) andwarning(line 126) mean those directives are silently ignored by Cargo. Since you're restructuring this function, it would be worth fixing them in the same pass — see inline comments. Copilot's earlier review also noted theMOZBUILD_STATE_PATHone.
As Copilot noted, xcode-select --print-path may return the Command Line Tools path (/Library/Developer/CommandLineTools) rather than a full Xcode installation, in which case the Toolchains/XcodeDefault.xctoolchain/usr/lib subpath won't exist. A fallback to <path>/usr/lib would improve robustness for CLT-only setups.
There was a problem hiding this comment.
Good change. Moving setup_clang() from setup_standalone() to main() is the right call — bindgen runs for all three setup paths (gecko, standalone, pkg-config), so libclang discovery should be unconditional. The macOS logic to prefer Xcode's libclang over Homebrew's is well-motivated by clang-sys's version-sorting behavior, and the CLT fallback (usr/lib in addition to the Xcode toolchain path) handles both full Xcode and Command Line Tools installs.
The pre-existing Cargo directive bugs (cargo: prefix, rustc-env syntax) have been fixed in subsequent commits. One minor new observation posted inline: the cargo:rustc-env=LIBCLANG_PATH directive is now syntactically correct (was broken before), which means it now actively leaks a build-time-only variable into the compiled crate where it's unused.
There was a problem hiding this comment.
Clean, well-scoped fix. Moving setup_clang() from setup_standalone() to main() is the right call — build_bindings() runs unconditionally regardless of the NSS setup path (gecko, standalone, pkg-config), so libclang discovery should be unconditional too. Running it for gecko builds is benign since LIBCLANG_PATH is typically already set by mach (early return on line 75).
The macOS logic correctly addresses clang-sys's version-sorting behavior that can prefer a Homebrew LLVM lacking SDK include paths. The cargo: prefix fixes and rustc-env removal are good cleanups done along the way.
One inline comment posted (CLT fallback, also noted by Copilot).
There was a problem hiding this comment.
Clean, well-scoped fix. Moving setup_clang() from setup_standalone() to main() is correct — build_bindings() invokes bindgen unconditionally regardless of which NSS setup path is taken, so libclang discovery should be unconditional too. The macOS logic to prefer Xcode's libclang over Homebrew's addresses a real clang-sys version-sorting pitfall, and the candidate array with CLT fallback handles both full Xcode and Command Line Tools installs.
Prior review feedback (missing cargo: prefixes, rustc-env removal, CLT fallback, xcode-select error diagnostics) has been well-incorporated across the follow-up commits. One minor remaining item posted inline: the new macOS warning messages should use println!("cargo:warning=...") instead of eprintln! to be reliably surfaced by Cargo.
|
Should be rebased once #3558 has landed. |
|
@Not-Nik this will need to move to nss-rs |
Instead of Homebrew's, for example.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: Lars Eggert <lars@eggert.org>
larseggert
force-pushed
the
fix-macos-llvm-homebrew
branch
from
a263357 to
8af26c2
Compare
There was a problem hiding this comment.
Good, well-scoped fix. Moving setup_clang() from setup_standalone() to main() is correct — build_bindings() invokes bindgen unconditionally, so libclang discovery should be too. The per-platform branching is cleaner than the old inverted OS != "windows" guard, and the candidates array handles both full Xcode and Command Line Tools installs.
Prior review feedback (cargo: prefixes, rustc-env removal, CLT fallback, error diagnostics) has been well-incorporated. One remaining issue: the macOS warning paths use eprintln! while the Windows branch uses println!("cargo:warning=...") — see inline comments.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: Lars Eggert <lars@eggert.org>
There was a problem hiding this comment.
Good, well-scoped fix. The core change — lifting setup_clang() from setup_standalone() to main() — is correct: build_bindings() invokes bindgen unconditionally regardless of the NSS discovery path, so libclang setup should be unconditional too. Running it for gecko builds is benign since LIBCLANG_PATH is typically already set by mach (early return on line 75).
The macOS logic to prefer Xcode's libclang over Homebrew's is well-motivated by clang-sys's version-sorting behavior, and the candidates array with CLT fallback (usr/lib in addition to the Xcode toolchain path) handles both full Xcode and Command Line Tools installs. The removal of the pre-existing broken cargo:rustc-env=LIBCLANG_PATH directive is a good cleanup.
One inline comment posted about the CI detection guard.
There was a problem hiding this comment.
Clean, well-motivated fix. Moving setup_clang() from setup_standalone() to main() is correct — build_bindings() invokes bindgen unconditionally regardless of the NSS discovery path, so libclang setup should be too. The per-platform branching with macOS Xcode detection and CLT fallback via the candidates array is a clear improvement over the old inverted OS != "windows" guard.
All prior review feedback has been well-incorporated across the follow-up commits: correct cargo: prefixes, println!("cargo:warning=...") instead of eprintln!, CLT fallback, env::var("CI") for CI detection, and removal of the unused cargo:rustc-env directive.
One minor inline comment about a missing cargo:rerun-if-env-changed for the CI variable.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: Lars Eggert <lars@eggert.org>
There was a problem hiding this comment.
Good, well-scoped fix. The changes look correct and all prior review feedback has been incorporated.
Architecture: Moving setup_clang() from setup_standalone() to main() is the right call — build_bindings() invokes bindgen unconditionally regardless of the NSS discovery path (gecko, standalone, pkg-config), so libclang setup should be unconditional too. For gecko builds this is benign since LIBCLANG_PATH is already set by mach (early return at line 76).
macOS logic: The Xcode detection via xcode-select --print-path with the two-entry candidates array correctly handles both full Xcode installs (Toolchains/XcodeDefault.xctoolchain/usr/lib) and Command Line Tools-only setups (usr/lib). This addresses the clang-sys version-sorting behavior that can pick up a Homebrew LLVM lacking macOS SDK include paths.
Cleanups: Switching from GITHUB_WORKFLOW == "CI" to env::var("CI") for CI detection is more robust (standard env var across all CI systems). The cargo:rerun-if-env-changed directives are correctly registered for all consulted env vars. The removal of the broken cargo:rustc-env=LIBCLANG_PATH directive is a good cleanup — that variable is only needed during build script execution, not at crate compile time.
Benchmark resultsNo significant performance differences relative to f5eab61. All resultstransfer/1-conn/1-100mb-resp (aka. Download)/mtu-1504: No change in performance detected. time: [204.26 ms 204.69 ms 205.22 ms]
thrpt: [487.29 MiB/s 488.56 MiB/s 489.58 MiB/s]
change:
time: [-0.5729% -0.2266% +0.1111] (p = 0.21 > 0.05)
thrpt: [-0.1110% +0.2271% +0.5762]
No change in performance detected.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) low mild
3 (3.00%) high severetransfer/1-conn/10_000-parallel-1b-resp (aka. RPS)/mtu-1504: No change in performance detected. time: [285.66 ms 287.63 ms 289.64 ms]
thrpt: [34.526 Kelem/s 34.767 Kelem/s 35.007 Kelem/s]
change:
time: [-1.4470% -0.5229% +0.4273] (p = 0.29 > 0.05)
thrpt: [-0.4255% +0.5256% +1.4682]
No change in performance detected.transfer/1-conn/1-1b-resp (aka. HPS)/mtu-1504: No change in performance detected. time: [38.570 ms 38.699 ms 38.849 ms]
thrpt: [25.740 B/s 25.840 B/s 25.927 B/s]
change:
time: [-1.3363% -0.6654% -0.0089] (p = 0.05 > 0.05)
thrpt: [+0.0089% +0.6699% +1.3544]
No change in performance detected.
Found 7 outliers among 100 measurements (7.00%)
3 (3.00%) high mild
4 (4.00%) high severetransfer/1-conn/1-100mb-req (aka. Upload)/mtu-1504: No change in performance detected. time: [203.89 ms 204.34 ms 204.89 ms]
thrpt: [488.06 MiB/s 489.38 MiB/s 490.47 MiB/s]
change:
time: [-0.1651% +0.1218% +0.4334] (p = 0.45 > 0.05)
thrpt: [-0.4315% -0.1216% +0.1654]
No change in performance detected.
Found 4 outliers among 100 measurements (4.00%)
2 (2.00%) high mild
2 (2.00%) high severestreams/walltime/1-streams/each-1000-bytes: Change within noise threshold. time: [585.96 µs 588.19 µs 590.74 µs]
change: [-1.2273% -0.6451% -0.0677] (p = 0.03 < 0.05)
Change within noise threshold.
Found 11 outliers among 100 measurements (11.00%)
1 (1.00%) high mild
10 (10.00%) high severestreams/walltime/1000-streams/each-1-bytes: Change within noise threshold. time: [12.429 ms 12.456 ms 12.491 ms]
change: [+0.6200% +0.8988% +1.2246] (p = 0.00 < 0.05)
Change within noise threshold.
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) high mild
1 (1.00%) high severestreams/walltime/1000-streams/each-1000-bytes: Change within noise threshold. time: [44.657 ms 44.706 ms 44.759 ms]
change: [+0.3528% +0.5194% +0.6934] (p = 0.00 < 0.05)
Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severetransfer/walltime/pacing-false/varying-seeds: Change within noise threshold. time: [82.135 ms 82.181 ms 82.228 ms]
change: [+1.2826% +1.4014% +1.5072] (p = 0.00 < 0.05)
Change within noise threshold.transfer/walltime/pacing-true/varying-seeds: Change within noise threshold. time: [82.036 ms 82.094 ms 82.157 ms]
change: [-0.4906% -0.3875% -0.2836] (p = 0.00 < 0.05)
Change within noise threshold.
Found 4 outliers among 100 measurements (4.00%)
2 (2.00%) high mild
2 (2.00%) high severetransfer/walltime/pacing-false/same-seed: Change within noise threshold. time: [80.941 ms 81.023 ms 81.117 ms]
change: [+0.3123% +0.4347% +0.5759] (p = 0.00 < 0.05)
Change within noise threshold.
Found 6 outliers among 100 measurements (6.00%)
2 (2.00%) high mild
4 (4.00%) high severetransfer/walltime/pacing-true/same-seed: No change in performance detected. time: [82.503 ms 82.606 ms 82.760 ms]
change: [-0.1844% -0.0291% +0.1659] (p = 0.78 > 0.05)
No change in performance detected.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severeDownload data for |
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
|
Client/server transfer resultsPerformance differences relative to f5eab61. No significant performance differences. Table above only shows statistically significant changes. See all results below. All resultsTransfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Download data for |
Instead of Homebrew's, for example.