Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

deps: fix V8 compilation on GCC 12 #53728

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

deps: fix V8 compilation on GCC 12 #53728

wants to merge 2 commits into from
+7 −2

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jul 4, 2024 •
edited
Loading

I am not sure if this is acceptable in the upstream considering they are dropping support for MSVC and GCC support would also be limited in the near future. But this should address the problem of not being able to build Node.js with GCC 12 on certain Linux distros.

Refs: #45427
Refs: nodejs/help#4406
Refs: #53633
Refs: nodejs/help#4430
Refs: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=aeba3e009b0abfccaf01797556445dbf891cc8dc

cc @targos
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/security-wg
  • @nodejs/v8-update
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Jul 4, 2024
@joyeecheung
Copy link
Member Author

Upstreamed https://chromium-review.googlesource.com/c/v8/v8/+/5679182 to see if it's acceptable in the upstream...if not I think we should float it to prevent a hole in our GCC >= 10.1 on Linux support matrix?
@joyeecheung joyeecheung force-pushed the fix-gcc-12 branch 3 times, most recently from 0a9e556 to c896192 Compare July 4, 2024 18:07
@richardlau
Copy link
Member

I can confirm this fixes building Node.js with the 12.2.0 gcc Docker container (main fails without this in that container, succeeds in the gcc:12.3.0 container).
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 4, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 4, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@richardlau richardlau mentioned this pull request Jul 5, 2024
Open
@joyeecheung
Copy link
Member Author

joyeecheung commented Jul 5, 2024 •
edited
Loading

Extended the range check to include 12.1 which is also broken. Also changed the referenced link to the GCC fix.
@joyeecheung @richardlau
fixup! deps: fix V8 compilation on GCC 12
75a4d43 
Co-authored-by: Richard Lau <rlau@redhat.com>
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60108/
@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/6063/
@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/6063/
@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/6063/
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60123/
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60136/
@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/6068/
@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/6068/
@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/6068/
@joyeecheung
Copy link
Member Author

This will be merged in https://chromium-review.googlesource.com/c/v8/v8/+/5679182 too, so I will change it into a V8 cherry-pick when the V8 commit queue finishes.
@richardlau
Copy link
Member

This will be merged in https://chromium-review.googlesource.com/c/v8/v8/+/5679182 too, so I will change it into a V8 cherry-pick when the V8 commit queue finishes.

@joyeecheung I can't figure out how to comment in https://chromium-review.googlesource.com/c/v8/v8/+/5679182, but it appears to be missing the 75a4d43 fixup.
@joyeecheung
Copy link
Member Author

@richardlau Thanks for catching it, I fixed it in the CL, the commit queue request should be cancelled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency.
9 participants
@joyeecheung @nodejs-github-bot @richardlau @jasnell @santigimeno @lpinca @gengjiawen @daeyeon @legendecas