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.

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

deps: fix V8 compilation on GCC 12 #53728

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jul 4, 2024

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.

EDIT: it has been merged in the upstream in https://chromium-review.googlesource.com/c/v8/v8/+/5679182

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.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@joyeecheung
Copy link
Member Author

joyeecheung commented Jul 5, 2024

Extended the range check to include 12.1 which is also broken. Also changed the referenced link to the GCC fix.

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2024
@joyeecheung
Copy link
Member Author

@richardlau Thanks for catching it, I fixed it in the CL, the commit queue request should be cancelled.

Original commit message:

    [base] fix builds with GCC 12 on certain Linux distributions

    With GCC 12 on certain Linux distributions (at least Debian 12,
    Alpine 3.18, Fedora 37, that ships GCC 12.2),
    std::is_trivially_copyable is broken
    and as a result, V8 fails to compile. This patch uses the
    same polyfill on MSVC to make it compile with GCC 12.2.

    See nodejs#45427 for more context.
    Refs: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=aeba3e009b0abfccaf01797556445dbf891cc8dc

    Change-Id: Ie0ab1bb1ec105bacbd80b341adf7dbd8569f031f
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5679182
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#95181}

Refs: v8/v8@35888fe
@joyeecheung
Copy link
Member Author

The V8 upstream patch has landed, so updated this PR to be a proper V8 backport.

Can you take a look again? Thanks! @jasnell @santigimeno @lpinca @gengjiawen @richardlau @daeyeon @legendecas

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 24, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 24, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 25, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 25, 2024
@nodejs-github-bot nodejs-github-bot merged commit 8027a7b into nodejs:main Jul 25, 2024
55 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 8027a7b

targos pushed a commit that referenced this pull request Jul 28, 2024
Original commit message:

    [base] fix builds with GCC 12 on certain Linux distributions

    With GCC 12 on certain Linux distributions (at least Debian 12,
    Alpine 3.18, Fedora 37, that ships GCC 12.2),
    std::is_trivially_copyable is broken
    and as a result, V8 fails to compile. This patch uses the
    same polyfill on MSVC to make it compile with GCC 12.2.

    See #45427 for more context.
    Refs: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=aeba3e009b0abfccaf01797556445dbf891cc8dc

    Change-Id: Ie0ab1bb1ec105bacbd80b341adf7dbd8569f031f
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5679182
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#95181}

Refs: v8/v8@35888fe
PR-URL: #53728
Refs: #45427
Refs: nodejs/help#4406
Refs: #53633
Refs: nodejs/help#4430
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Jul 28, 2024
Original commit message:

    [base] fix builds with GCC 12 on certain Linux distributions

    With GCC 12 on certain Linux distributions (at least Debian 12,
    Alpine 3.18, Fedora 37, that ships GCC 12.2),
    std::is_trivially_copyable is broken
    and as a result, V8 fails to compile. This patch uses the
    same polyfill on MSVC to make it compile with GCC 12.2.

    See nodejs#45427 for more context.
    Refs: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=aeba3e009b0abfccaf01797556445dbf891cc8dc

    Change-Id: Ie0ab1bb1ec105bacbd80b341adf7dbd8569f031f
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5679182
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#95181}

Refs: v8/v8@35888fe
PR-URL: nodejs#53728
Refs: nodejs#45427
Refs: nodejs/help#4406
Refs: nodejs#53633
Refs: nodejs/help#4430
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Jul 28, 2024
Original commit message:

    [base] fix builds with GCC 12 on certain Linux distributions

    With GCC 12 on certain Linux distributions (at least Debian 12,
    Alpine 3.18, Fedora 37, that ships GCC 12.2),
    std::is_trivially_copyable is broken
    and as a result, V8 fails to compile. This patch uses the
    same polyfill on MSVC to make it compile with GCC 12.2.

    See nodejs#45427 for more context.
    Refs: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=aeba3e009b0abfccaf01797556445dbf891cc8dc

    Change-Id: Ie0ab1bb1ec105bacbd80b341adf7dbd8569f031f
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5679182
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#95181}

Refs: v8/v8@35888fe
PR-URL: nodejs#53728
Refs: nodejs#45427
Refs: nodejs/help#4406
Refs: nodejs#53633
Refs: nodejs/help#4430
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit to targos/nodejs-build that referenced this pull request Jul 28, 2024
@RafaelGSS RafaelGSS mentioned this pull request Jul 30, 2024
RafaelGSS pushed a commit that referenced this pull request Aug 5, 2024
Original commit message:

    [base] fix builds with GCC 12 on certain Linux distributions

    With GCC 12 on certain Linux distributions (at least Debian 12,
    Alpine 3.18, Fedora 37, that ships GCC 12.2),
    std::is_trivially_copyable is broken
    and as a result, V8 fails to compile. This patch uses the
    same polyfill on MSVC to make it compile with GCC 12.2.

    See #45427 for more context.
    Refs: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=aeba3e009b0abfccaf01797556445dbf891cc8dc

    Change-Id: Ie0ab1bb1ec105bacbd80b341adf7dbd8569f031f
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5679182
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#95181}

Refs: v8/v8@35888fe
PR-URL: #53728
Refs: #45427
Refs: nodejs/help#4406
Refs: #53633
Refs: nodejs/help#4430
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit to nodejs/build that referenced this pull request Aug 6, 2024
targos pushed a commit to targos/node that referenced this pull request Aug 7, 2024
Original commit message:

    [base] fix builds with GCC 12 on certain Linux distributions

    With GCC 12 on certain Linux distributions (at least Debian 12,
    Alpine 3.18, Fedora 37, that ships GCC 12.2),
    std::is_trivially_copyable is broken
    and as a result, V8 fails to compile. This patch uses the
    same polyfill on MSVC to make it compile with GCC 12.2.

    See nodejs#45427 for more context.
    Refs: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=aeba3e009b0abfccaf01797556445dbf891cc8dc

    Change-Id: Ie0ab1bb1ec105bacbd80b341adf7dbd8569f031f
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5679182
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#95181}

Refs: v8/v8@35888fe
PR-URL: nodejs#53728
Refs: nodejs#45427
Refs: nodejs/help#4406
Refs: nodejs#53633
Refs: nodejs/help#4430
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Aug 9, 2024
Original commit message:

    [base] fix builds with GCC 12 on certain Linux distributions

    With GCC 12 on certain Linux distributions (at least Debian 12,
    Alpine 3.18, Fedora 37, that ships GCC 12.2),
    std::is_trivially_copyable is broken
    and as a result, V8 fails to compile. This patch uses the
    same polyfill on MSVC to make it compile with GCC 12.2.

    See nodejs#45427 for more context.
    Refs: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=aeba3e009b0abfccaf01797556445dbf891cc8dc

    Change-Id: Ie0ab1bb1ec105bacbd80b341adf7dbd8569f031f
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5679182
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#95181}

Refs: v8/v8@35888fe
PR-URL: nodejs#53728
Refs: nodejs#45427
Refs: nodejs/help#4406
Refs: nodejs#53633
Refs: nodejs/help#4430
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Aug 14, 2024
Original commit message:

    [base] fix builds with GCC 12 on certain Linux distributions

    With GCC 12 on certain Linux distributions (at least Debian 12,
    Alpine 3.18, Fedora 37, that ships GCC 12.2),
    std::is_trivially_copyable is broken
    and as a result, V8 fails to compile. This patch uses the
    same polyfill on MSVC to make it compile with GCC 12.2.

    See nodejs#45427 for more context.
    Refs: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=aeba3e009b0abfccaf01797556445dbf891cc8dc

    Change-Id: Ie0ab1bb1ec105bacbd80b341adf7dbd8569f031f
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5679182
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#95181}

Refs: v8/v8@35888fe
PR-URL: nodejs#53728
Refs: nodejs#45427
Refs: nodejs/help#4406
Refs: nodejs#53633
Refs: nodejs/help#4430
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Aug 16, 2024
Original commit message:

    [base] fix builds with GCC 12 on certain Linux distributions

    With GCC 12 on certain Linux distributions (at least Debian 12,
    Alpine 3.18, Fedora 37, that ships GCC 12.2),
    std::is_trivially_copyable is broken
    and as a result, V8 fails to compile. This patch uses the
    same polyfill on MSVC to make it compile with GCC 12.2.

    See #45427 for more context.
    Refs: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=aeba3e009b0abfccaf01797556445dbf891cc8dc

    Change-Id: Ie0ab1bb1ec105bacbd80b341adf7dbd8569f031f
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5679182
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#95181}

Refs: v8/v8@35888fe
PR-URL: #53728
Refs: #45427
Refs: nodejs/help#4406
Refs: #53633
Refs: nodejs/help#4430
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #54077
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants