build(unified): compile ProcessBindingConstants.cpp standalone - #29582
Conversation
ProcessBindingConstants.cpp uses 251 #ifdef checks to decide which fs.constants / os.constants to expose per-platform. When bundled in the same unified TU after NodeFSStatBinding.cpp (which #defines S_IFBLK / S_IFSOCK on Windows for its own S_IS*() helpers), those macros leak forward and fs.constants gains S_IFBLK/S_IFSOCK on Windows, breaking Node.js parity and test/js/node/fs/fs.test.ts. Same class of issue as ProcessBindingUV.cpp which is already in noUnify for its errno/EAI_* macro dependencies.
|
Updated 5:05 AM PT - Apr 22nd, 2026
❌ @robobun, your commit cdd40d2 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29582That installs a local version of the PR into your bun-29582 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughModified the build configuration to add Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward addition to the noUnify list following the established ProcessBindingUV.cpp pattern.
Extended reasoning...
Overview
Adds a single entry (src/bun.js/bindings/ProcessBindingConstants.cpp) to the noUnify array in scripts/build/unified.ts, with an explanatory comment. This forces the file to compile as a standalone TU rather than being bundled with siblings in the unified-sources build.
Security risks
None. This is a build-configuration list change with no effect on runtime logic, auth, crypto, or user-facing surface area beyond restoring pre-#29545 behavior for fs.constants on Windows.
Level of scrutiny
Low. The change is mechanical and follows the exact pattern of the entry directly above it (ProcessBindingUV.cpp, excluded for the identical macro-leakage reason). I verified the root-cause claims: NodeFSStatBinding.cpp:55-72 does #define S_IFBLK/S_IFSOCK under #ifndef guards, and ProcessBindingConstants.cpp:659/668 uses #ifdef S_IFBLK/#ifdef S_IFSOCK to gate exposure. Both files live in the same directory and N sorts before P, so the macro leak is real.
Other factors
The PR description is thorough and the fix is the conservative, correct one — ProcessBindingConstants.cpp has 251 #ifdef checks and is fundamentally sensitive to ambient macro state, so isolating it is preferable to #undefing in NodeFSStatBinding.cpp. No bugs were found by the bug hunter. No outstanding reviewer comments.
Main now compiles ProcessBindingConstants.cpp standalone (noUnify), which is the more robust fix — that file has 251 #ifdef checks and needs isolation from ambient macro state regardless of which upstream file leaks. The push/pop wrapper here is no longer needed.
Main landed #29582 which excludes ProcessBindingConstants.cpp from the unified-source bundle, so the push_macro/pop_macro bracket in NodeFSStatBinding.cpp added here is no longer needed to keep fs.constants correct on Windows.
…sh#29582) ## Problem Since oven-sh#29545 (unified C++ sources), `test/js/node/fs/fs.test.ts` fails on Windows: ``` error: expect(received).toEqual(expected) { ... + "S_IFBLK": 24576, ... + "S_IFSOCK": 49152, ... } ✗ fs.constants ``` Node.js does not expose `S_IFBLK` / `S_IFSOCK` in `fs.constants` on Windows, and neither did Bun before oven-sh#29545. ## Cause `ProcessBindingConstants.cpp` decides which constants to expose via `#ifdef`: ```cpp #ifdef S_IFBLK object->putDirect(vm, ..., jsNumber(S_IFBLK)); #endif ``` `NodeFSStatBinding.cpp` `#define`s `S_IFBLK` / `S_IFSOCK` on Windows for its own `S_ISBLK()` / `S_ISSOCK()` helpers. Both files live in `src/bun.js/bindings/`, and with `N` sorted before `P`, `NodeFSStatBinding.cpp` is `#include`d first in the same unified TU — its macros leak into `ProcessBindingConstants.cpp`'s `#ifdef` checks. `ProcessBindingConstants.cpp` has **251** `#ifdef`/`#ifndef` checks; it is fundamentally sensitive to ambient macro state and should compile in isolation, same as `ProcessBindingUV.cpp` (already in `noUnify` for the identical reason with errno/EAI_* macros). ## Fix Add `ProcessBindingConstants.cpp` to `noUnify` in `scripts/build/unified.ts`. ## Verification - `fs.constants` test failure reproduces on Windows in every build based on `main` ≥ `2cf54bf6dd` (e.g. [#47110](https://buildkite.com/bun/bun/builds/47110), windows-2019-x64 / x64-baseline / win11-aarch64) - Not seen on `main` CI yet only because build #47033 failed at an unrelated aarch64-musl build-cpp step before Windows tests ran - Linux/macOS unaffected (`S_IFBLK`/`S_IFSOCK` come from `<sys/stat.h>` there regardless) Co-authored-by: robobun <robobun@users.noreply.github.com>
…sh#29582) ## Problem Since oven-sh#29545 (unified C++ sources), `test/js/node/fs/fs.test.ts` fails on Windows: ``` error: expect(received).toEqual(expected) { ... + "S_IFBLK": 24576, ... + "S_IFSOCK": 49152, ... } ✗ fs.constants ``` Node.js does not expose `S_IFBLK` / `S_IFSOCK` in `fs.constants` on Windows, and neither did Bun before oven-sh#29545. ## Cause `ProcessBindingConstants.cpp` decides which constants to expose via `#ifdef`: ```cpp #ifdef S_IFBLK object->putDirect(vm, ..., jsNumber(S_IFBLK)); #endif ``` `NodeFSStatBinding.cpp` `#define`s `S_IFBLK` / `S_IFSOCK` on Windows for its own `S_ISBLK()` / `S_ISSOCK()` helpers. Both files live in `src/bun.js/bindings/`, and with `N` sorted before `P`, `NodeFSStatBinding.cpp` is `#include`d first in the same unified TU — its macros leak into `ProcessBindingConstants.cpp`'s `#ifdef` checks. `ProcessBindingConstants.cpp` has **251** `#ifdef`/`#ifndef` checks; it is fundamentally sensitive to ambient macro state and should compile in isolation, same as `ProcessBindingUV.cpp` (already in `noUnify` for the identical reason with errno/EAI_* macros). ## Fix Add `ProcessBindingConstants.cpp` to `noUnify` in `scripts/build/unified.ts`. ## Verification - `fs.constants` test failure reproduces on Windows in every build based on `main` ≥ `2cf54bf6dd` (e.g. [#47110](https://buildkite.com/bun/bun/builds/47110), windows-2019-x64 / x64-baseline / win11-aarch64) - Not seen on `main` CI yet only because build #47033 failed at an unrelated aarch64-musl build-cpp step before Windows tests ran - Linux/macOS unaffected (`S_IFBLK`/`S_IFSOCK` come from `<sys/stat.h>` there regardless) Co-authored-by: robobun <robobun@users.noreply.github.com>
2 participants
Problem
Since #29545 (unified C++ sources),
test/js/node/fs/fs.test.tsfails on Windows:Node.js does not expose
S_IFBLK/S_IFSOCKinfs.constantson Windows, and neither did Bun before #29545.Cause
ProcessBindingConstants.cppdecides which constants to expose via#ifdef:NodeFSStatBinding.cpp#definesS_IFBLK/S_IFSOCKon Windows for its ownS_ISBLK()/S_ISSOCK()helpers. Both files live insrc/bun.js/bindings/, and withNsorted beforeP,NodeFSStatBinding.cppis#included first in the same unified TU — its macros leak intoProcessBindingConstants.cpp's#ifdefchecks.ProcessBindingConstants.cpphas 251#ifdef/#ifndefchecks; it is fundamentally sensitive to ambient macro state and should compile in isolation, same asProcessBindingUV.cpp(already innoUnifyfor the identical reason with errno/EAI_* macros).Fix
Add
ProcessBindingConstants.cpptonoUnifyinscripts/build/unified.ts.Verification
fs.constantstest failure reproduces on Windows in every build based onmain≥2cf54bf6dd(e.g. #47110, windows-2019-x64 / x64-baseline / win11-aarch64)mainCI yet only because build #47033 failed at an unrelated aarch64-musl build-cpp step before Windows tests ranS_IFBLK/S_IFSOCKcome from<sys/stat.h>there regardless)