fix(build): make fresh parallel LPeg builds reproducible - #33
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🔇 Additional comments (6)
📝 SummarySummary by CodeRabbit
WalkthroughThe pull request vendors Lua 5.1 headers and LuaJIT configuration data under ChangesLua 5.1 header integration
Priority: ⬇️ Low Change: Bug fix 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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 |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR successfully addresses the parallel build race condition by replacing dynamic header directory scanning with pinned Lua 5.1 headers from the locked LuaJIT source package. The solution is cleaner and more reproducible.
Critical Issues
Two issues need attention before merge:
-
Missing header file validation in build.rs - Add explicit checks that headers exist before compilation to provide clear error messages instead of cryptic linker failures.
-
SHA-256 hashes not verified - The documented hashes in README.md should be enforced during build to prevent supply chain attacks and detect file corruption.
Verification Needed
The PR claims testing passed on Linux x86-64. Ensure cross-platform builds (especially Windows mentioned in the PR description) are verified before merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
|
/q review |
|
@codex review |
There was a problem hiding this comment.
This PR effectively resolves the parallel build reproducibility issue by replacing fragile directory-scanning logic with vendored Lua 5.1 C API headers. The implementation is clean, well-documented with provenance and SHA-256 hashes, and maintains proper licensing attribution.
The key improvements are:
- Eliminates race conditions from scanning sibling build directories
- Makes builds independent of Cargo's target-directory layout and build order
- Adds explicit build input tracking with cargo:rerun-if-changed directives
- Preserves upstream copyright and documents exact source package versions
The verification tests confirm this fixes the reported build failure while maintaining LPeg functionality. The approach is sound and ready to merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Stack
First layer of the stabilization stack. Later terminal and startup fixes build on this branch.
Reproduced failure
A fresh
cargo build --workspace --releasefailed inox-lua/build.rsbecause it searched sibling Cargo output directories for LuaJIT headers whilemlua-syswas still building. Choosing the newest directory could also select unrelated cached artifacts.Change
Compile vendored LPeg against three unmodified Lua 5.1 C API headers from the exact locked
luajit-src 210.7.2+b925b3epackage. Preserve the upstream copyright, record source provenance and SHA-256 hashes, and remove output-directory scanning. The binary still links the single LuaJIT runtime built bymlua-sys.Executed verification
Linux x86-64, Rust 1.98.0. Every Cargo command unsets
CARGO_BUILD_BUILD_DIRandRUSTC_WRAPPER.no mlua-sys vendored LuaJIT headers found ...; build mlua first.CARGO_TARGET_DIR.cargo test --release -p ox-lua --test stdlib lpeg: 2 passed, including the upstream LPeg 1.1.0 suite.git diff --check: passed.This fixes an additional clean-build blocker relevant to #28. It does not claim that the remaining Windows runtime code compiles or that the full Neovim compatibility gates pass.