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

Reduce the number of patches we float on V8 #45118

Open
4 of 7 tasks
targos opened this issue Oct 22, 2022 · 13 comments
Open
4 of 7 tasks

Reduce the number of patches we float on V8 #45118

targos opened this issue Oct 22, 2022 · 13 comments
Labels
help wanted Issues that need assistance from volunteers or PRs that need help to proceed. meta Issues and PRs related to the general management of the project. v8 engine Issues and PRs related to the V8 dependency.

Comments

@targos
Copy link
Member

targos commented Oct 22, 2022 •
edited
Loading

We should aim for 0 floating patches from our side (except cherry-picks of commits that landed upstream).

Here's the list of everything that we currently float on top of V8 10.7:

@nodejs/v8-update
@targos targos added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. v8 engine Issues and PRs related to the V8 dependency. meta Issues and PRs related to the general management of the project. labels Oct 22, 2022
@targos targos mentioned this issue Oct 22, 2022
Merged
@gengjiawen
Copy link
Member

deps: V8: fix v8-cppgc.h for MSVC
Not sure why this wasn't submitted upstream.

I know this. I send the patch, but v8 team want it to be compatible with cpp20, so it got stuck

@bnoordhuis bnoordhuis mentioned this issue Oct 22, 2022
Closed
@bnoordhuis
Copy link
Member

bnoordhuis commented Oct 22, 2022 •
edited
Loading

"make V8 compilable with older glibc" can be dropped, "silence irrelevant V8 warning" can be dropped if we turn off -Werror.

W.r.t. the msvc patches: maybe we should just switch to clang-cl? We'll want V8's headers to be usable by msvc (for add-ons) but that's a lot less work than keeping V8 itself buildable under msvc.

@targos
Copy link
Member Author

targos commented Oct 22, 2022

W.r.t. the msvc patches: maybe we should just switch to clang-cl?

Let me rebase #35433 to see how it goes.

@targos
Copy link
Member Author

targos commented Oct 22, 2022

/cc @richardlau who added -Werror in fcc183c

@richardlau
Copy link
Member

/cc @richardlau who added -Werror in fcc183c

fcc183c was a follow on to #32685 where the original request to enable -Werror came from. I think this may have been a request from Red Hat (prior to me transferring over from IBM).
cc @danbev

@bnoordhuis bnoordhuis mentioned this issue Oct 24, 2022
Closed
@mhdawson
Copy link
Member

In terms of -Werror, is the suggestion to turn it off just for V8 or for all of Node.js. I'd be ok with turning it off for deps like v8, but would want us to keep it for our code.

nodejs-github-bot pushed a commit that referenced this issue Oct 25, 2022
@targos
Revert "deps: V8: forward declaration of Rtl*FunctionTable"
c6c18a9 
This reverts commit 01bc8e6.

PR-URL: #45119
Refs: #45118
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@targos targos mentioned this issue Oct 25, 2022
Merged
@targos
Copy link
Member Author

targos commented Oct 25, 2022

In terms of -Werror, is the suggestion to turn it off just for V8 or for all of Node.js. I'd be ok with turning it off for deps like v8, but would want us to keep it for our code.

I'm all for it, but I don't know how to set that up.

@bnoordhuis
Copy link
Member

is the suggestion to turn it off just for V8 or for all of Node.js

For dependencies, like V8, since we don't control those.

-Werror can stay for node itself (on CI - it's anti-social to impose that on people building node from source.)

@richardlau
Copy link
Member

I've reminded myself of how we set this up -- we already only turn on (in the CI through the --error-on-warn configure option) -Werror for node itself and not for dependencies. The issue here is that the warning is coming from a V8 header file so the node part of the build errors out when including v8.h.

nodejs-github-bot pushed a commit that referenced this issue Oct 27, 2022
@targos
Revert "deps: make V8 compilable with older glibc"
466d36e 
This reverts commit 84d455e.

PR-URL: #45162
Refs: #45118
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Beth Griggs <bethanyngriggs@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos
Copy link
Member Author

targos commented Oct 27, 2022

@Flarna It's not obvious what your edit changed (apart from removing the first check)

@Flarna
Copy link
Member

Flarna commented Oct 27, 2022

Actually I didn't want to change anything. Whatever was done by me here was per accident. Sorry

RafaelGSS pushed a commit that referenced this issue Nov 1, 2022
@targos @RafaelGSS
Revert "deps: V8: forward declaration of Rtl*FunctionTable"
983bc57 
This reverts commit 01bc8e6.

PR-URL: #45119
Refs: #45118
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
RafaelGSS pushed a commit that referenced this issue Nov 1, 2022
@targos @RafaelGSS
Revert "deps: make V8 compilable with older glibc"
1650b3e 
This reverts commit 84d455e.

PR-URL: #45162
Refs: #45118
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Beth Griggs <bethanyngriggs@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this issue Nov 10, 2022
@targos @RafaelGSS
Revert "deps: V8: forward declaration of Rtl*FunctionTable"
0a6ed6f 
This reverts commit 01bc8e6.

PR-URL: #45119
Refs: #45118
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
RafaelGSS pushed a commit that referenced this issue Nov 10, 2022
@targos @RafaelGSS
Revert "deps: make V8 compilable with older glibc"
f589961 
This reverts commit 84d455e.

PR-URL: #45162
Refs: #45118
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Beth Griggs <bethanyngriggs@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@gengjiawen
Copy link
Member

deps: V8: fix v8-cppgc.h for MSVC
Not sure why this wasn't submitted upstream.

I know this. I send the patch, but v8 team want it to be compatible with cpp20, so it got stuck

I will give another try after MSVC fix landed #45427 (comment)

@targos targos mentioned this issue Dec 1, 2022
Closed
@bnoordhuis
Copy link
Member

deps: fix V8 build issue with inline methods and deps: fix V8 build on Windows with MSVC stand a good chance of being fixed by nodejs/node-v8@691f1c4.

(That whole class of compile-time trouble is likely going away now.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted Issues that need assistance from volunteers or PRs that need help to proceed. meta Issues and PRs related to the general management of the project. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@bnoordhuis @targos @gengjiawen @richardlau @mhdawson @Flarna