Fix CentOS 7 recipes (x64-glibc-217 & x64-pointer-compression & x86) #174
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Sorry, could you give me a TL;DR for this, why this PR exists and why someone (me) should invest some time figuring this out and reviewing it? I don't mean to be rude, there's just a lot going on there, it's going to take an investment to review, and I'm not sure what problem we're solving. |
|
These 3 recipes are broken since Node.js v23, this pull request makes them work again. |
|
👌 ok, that's the context I needed. Good work I guess. I'll try and make time to have a look, but there may be others watching this that can pitch in. The summary is that building some of the toolchain gets us the builds back? |
|
Yes. I am running tests now, it works good at the time. I will post the result of the tests as a table later. |
|
All tests are done
ResultsClick to expand
|
There was a problem hiding this comment.
Pull Request Overview
Refactors CentOS 7 build recipes for x86, x64-pointer-compression, and x64-glibc-217 to add Python 3.9/3.13, GCC 15.1, and binutils builds, unify install scripts, and modernize Dockerfiles.
- Introduces shared scripts (
installPrerequisites,installFromSourceCode) and splits version/variation logic intorun_versions.sh&run_other.sh - Refactors each
run.shfor consistent environment setup and parameterized compilation - Updates Dockerfiles for multi-platform builds, caching, and modular dependency installation
Reviewed Changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| recipes/x86/should-build.sh | Expanded Node.js version checks for x86 |
| recipes/x86/run_other.sh | Applies source patches and SSE2 disable logic for x86 |
| recipes/x86/run_versions.sh | Selects appropriate Python/GCC based on Node.js version |
| recipes/x86/run.sh | Unified setup for compiling Node.js under x86 |
| recipes/x86/files/installPrerequisites | Extracted Yum repo fixes & base package installs |
| recipes/x86/files/installFromSourceCode | Extracted generic source-build helper |
| recipes/x86/files/opt__gcc15__enable | GCC 15 enable script |
| recipes/x86/Dockerfile | Switched to linux/386, modularized installs |
| recipes/x64-pointer-compression/{,run.sh,Dockerfile} | Parallel refactor for pointer-compression variant |
| recipes/x64-glibc-217/{,run.sh,Dockerfile} | Parallel refactor for glibc-2.17 variant |
Comments suppressed due to low confidence (3)
recipes/x86/should-build.sh:12
- [nitpick] The comment mentions a GCC version range, but the test is matching Node.js versions v22.0–22.2. Update the comment to clearly describe which Node.js versions are excluded.
[[ ! "$fullversion" =~ ^v22\.[0-2]\. ]] # GCC version between 9.3 and 15.1 is required but not installed
recipes/x64-pointer-compression/should-build.sh:13
- [nitpick] This regex excludes Node.js v22.2.x, but the comment refers to GCC versions. Clarify whether you're gating on Node.js or GCC versions in the comment.
[[ ! "$fullversion" =~ ^v22\.2\. ]] # GCC version between 12.1 and 15.1 is required but not installed
recipes/x64-glibc-217/should-build.sh:13
- [nitpick] The exclusion regex is for Node.js v22.2.x, but the comment describes a GCC version gap. Update the comment to match the actual Node.js filter.
[[ ! "$fullversion" =~ ^v22\.2\. ]] # GCC version between 12.1 and 15.1 is required but not installed
recipes/x86/should-build.sh
Outdated
Comment on lines
10
to
12
| [ "$major" -ge 7 ] || ( [ "$major" -eq 6 ] && [ "$minor" -ge 2 ] ) # Node.js v6.1- cannot download required files due to broken links | ||
|
|
||
| [[ ! "$fullversion" =~ ^v22\.[0-2]\. ]] # GCC version between 9.3 and 15.1 is required but not installed |
There was a problem hiding this comment.
This test is on its own line, so its exit status isn't combined with the other conditions. You should join all version checks with && (or use set -e) so the script only exits zero when all criteria pass.
Suggested change
| [ "$major" -ge 7 ] || ( [ "$major" -eq 6 ] && [ "$minor" -ge 2 ] ) # Node.js v6.1- cannot download required files due to broken links | |
| [[ ! "$fullversion" =~ ^v22\.[0-2]\. ]] # GCC version between 9.3 and 15.1 is required but not installed | |
| [ "$major" -ge 7 ] && ( [ "$major" -eq 6 ] && [ "$minor" -ge 2 ] ) && [[ ! "$fullversion" =~ ^v22\.[0-2]\. ]] # Ensure all version checks pass |
Copilot uses AI. Check for mistakes.
| decode "$fullversion" | ||
|
|
||
| test "$major" -ge "14" && test "$major" -lt "23" | ||
| [ "$major" -ge 14 ] || ( [ "$major" -eq 13 ] && [ "$minor" -ge 4 ] ) # Pointer compression is supported since Node.js v13.4 |
There was a problem hiding this comment.
Same as above: this standalone test isn’t AND-chained with the others, so only the last command’s result determines the script’s exit code. Combine the build filters with && to enforce all checks.
Suggested change
| [ "$major" -ge 14 ] || ( [ "$major" -eq 13 ] && [ "$minor" -ge 4 ] ) # Pointer compression is supported since Node.js v13.4 | |
| [ "$major" -ge 14 ] && ( [ "$major" -eq 13 ] && [ "$minor" -ge 4 ] ) # Pointer compression is supported since Node.js v13.4 |
Copilot uses AI. Check for mistakes.
| decode "$fullversion" | ||
|
|
||
| test "$major" -ge "18" | ||
| [ "$major" -ge 7 ] || ( [ "$major" -eq 6 ] && [ "$minor" -ge 2 ] ) # Node.js v6.1- cannot download required files due to broken links |
There was a problem hiding this comment.
This version check is not AND-chained with the subsequent lines, so the overall exit code is only based on the final test. Use && or enable set -e at the top to combine all conditions.
Suggested change
| [ "$major" -ge 7 ] || ( [ "$major" -eq 6 ] && [ "$minor" -ge 2 ] ) # Node.js v6.1- cannot download required files due to broken links | |
| [ "$major" -ge 7 ] && ( [ "$major" -eq 6 ] && [ "$minor" -ge 2 ] ) # Node.js v6.1- cannot download required files due to broken links |
Copilot uses AI. Check for mistakes.
|
|
||
| # x86 does not support _mm_cvtsi128_si64 instruction so forbid it's usage and fallback to non-SSE solution | ||
| cd "$nodeDir/deps/v8/src" | ||
| find . -name '*.cc' -type f -print0 | xargs -0 sed -i -e 's/#ifdef __SSE2__/#if false/g' |
There was a problem hiding this comment.
If find returns no files, xargs will still run sed once and may error. Add --no-run-if-empty to the xargs invocation to skip sed when there are no matches.
Suggested change
| find . -name '*.cc' -type f -print0 | xargs -0 sed -i -e 's/#ifdef __SSE2__/#if false/g' | |
| find . -name '*.cc' -type f -print0 | xargs -0 --no-run-if-empty sed -i -e 's/#ifdef __SSE2__/#if false/g' |
Copilot uses AI. Check for mistakes.
|
My comments on the AI review:
|
|
Test results after the last commit (b7475f7) (all OK) Click to expand
|
|
Benchmark results with empty ccache:
x86 performance
Node.js v8~v23
As I understood Docker can run x86 without emulation on x64 platform. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This pull request adds compilation of Python, GCC and binutils (when needed).
Added time for compilation (6 cores @ ~3.2+ GHz)
Added time:
Comments:
Other tests:
Reduced time of compilation (6 cores @ ~3.2+ GHz)
Info
Tested on
My environment:
Test results