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

use gcc 12 on rhel8-s390x for Node.js 22 #3659

Merged
merged 4 commits into from
Apr 17, 2024
Merged

use gcc 12 on rhel8-s390x for Node.js 22 #3659

merged 4 commits into from
Apr 17, 2024

Conversation

richardlau
Copy link
Member

  • ansible: add libatomic-devel to gcc toolsets
  • ansible: build newer version of gn
  • jenkins: use gcc 12 on rhel8-s390x for Node.js 22

Refs: #3630
Fixes: #3335

@richardlau
Copy link
Member Author

I've deployed this onto test-ibm-rhel8-s390x-4 and am running a test build with nodejs/node#52183: https://ci.nodejs.org/job/richardlau-node-test-commit-v8-linux/546/nodes=rhel8-s390x,v8test=v8test/

This was referenced Mar 21, 2024
Closed
Closed
targos
targos approved these changes Mar 21, 2024
View reviewed changes
ansible/roles/baselayout/vars/main.yml Outdated Show resolved Hide resolved
@richardlau
Copy link
Member Author

I've deployed this onto test-ibm-rhel8-s390x-4 and am running a test build with nodejs/node#52183: https://ci.nodejs.org/job/richardlau-node-test-commit-v8-linux/546/nodes=rhel8-s390x,v8test=v8test/

Looks like we need v8/v8@f8d5e57 (https://chromium-review.googlesource.com/c/v8/v8/+/5280535) as well.

@richardlau
Copy link
Member Author

Updated nodejs/node#52183 to include https://chromium-review.googlesource.com/c/v8/v8/+/5280535.

Retry: https://ci.nodejs.org/job/richardlau-node-test-commit-v8-linux/547/nodes=rhel8-s390x,v8test=v8test/

@richardlau
Copy link
Member Author

Retry: https://ci.nodejs.org/job/richardlau-node-test-commit-v8-linux/547/nodes=rhel8-s390x,v8test=v8test/

This is failing two tests:

This is expected while we wait for https://issues.redhat.com/browse/RHEL-29952 to backport https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113960.

FWIW the Red Hat team maintaining the upstream s390x V8 port are currently manually patching one of the libstdlibc++ headers as per https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113960#c11 in the container they use to build/test V8:
https://github.com/nodeshift/v8-build/blob/85b7ce18cb95cacd68ba49adae2ed7bc1f23be2c/Dockerfile.base#L42-L47

@targos
Copy link
Member

targos commented Mar 22, 2024

We don't build Node.js with C++20 (yet) so it's OK?

@richardlau
Copy link
Member Author

We don't build Node.js with C++20 (yet) so it's OK?

Yes, wrt https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113960. My understanding is that V8 is building with C++20, hence this showing up in V8 build -- presumably the V8 CI is inheriting C++20 from some part of the gn build process.

I was hoping to avoid needing to patch the libstdc++ headers on the s390x machines ourselves and if possible wait for the gcc-toolset-12 packages to be updated, but patching the headers ourselves is an (temporary) option if we want to move to C++20 for Node.js for 22.

@richardlau
Copy link
Member Author

I was hoping to avoid needing to patch the libstdc++ headers on the s390x machines ourselves and if possible wait for the gcc-toolset-12 packages to be updated, but patching the headers ourselves is an (temporary) option if we want to move to C++20 for Node.js for 22.

Latest from RH for updating gcc-toolset-12 to patch https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113960 is "around May". I've added a commit to temporarily patch the libstdc++ header file for gcc-toolset-12, which we'll revert when the toolset is updated.

Started a test build with the patched header:
https://ci.nodejs.org/job/richardlau-node-test-commit-v8-linux/577/nodes=rhel8-s390x,v8test=v8test/

@richardlau
ansible: temporarily patch libstdc++ header on s390x
a9e2757 
As a stop-gap until gcc-toolset-12 in RHEL 8 is updated, manually patch
the affected libstdc++ header file.

Refs: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113960
mhdawson
mhdawson approved these changes Apr 17, 2024
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@richardlau richardlau merged commit 9c149d0 into main Apr 17, 2024
3 checks passed
@richardlau richardlau mentioned this pull request May 23, 2024
Merged
richardlau added a commit that referenced this pull request May 30, 2024
@richardlau
Revert "ansible: temporarily patch libstdc++ header on s390x" (#3736)
bec735f 
This reverts commit 9c149d0.

`gcc-toolset-12` now includes the fix we were temporarily patching for.

Refs: #3659 (comment)
Refs: nodeshift/v8-build#7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@targos targos targos approved these changes

@anonrig anonrig anonrig approved these changes

Assignees
No one assigned
Labels
ansible platform:linux-intel platform:linuxONE platform:ppc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unpin or update GN version
4 participants
@richardlau @targos @anonrig @mhdawson