-
Notifications
You must be signed in to change notification settings - Fork 30k
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
tools: use Ubuntu 24.04 and Clang on GitHub actions #53212
Conversation
Review requested:
|
Default Clang on |
|
Looks like we've got something broken with the Clang build config:
|
The addon didn't build: https://github.com/nodejs/node/actions/runs/9315062604/job/25640471713?pr=53212#step:7:5466
|
Oh, interesting. I wouldn't expect tests to run if part of the build failed. |
@nodejs/node-api Could one of you have a look? IIUC the bug is in node/benchmark/napi/ref/addon.c Lines 37 to 55 in 54035ac
|
Would either need to remove node/benchmark/napi/ref/addon.c Line 2 in 54035ac
Or change the type of |
We discussed today in the node-api team meeting, @targos can you just remove #define NAPI_EXPERIMENTAL from node/benchmark/napi/ref/addon.c as part of your PR? We think that is the right thing to do in this case. |
With further discussion it would also be great if you could change line 38 to IncrementCounter(node_api_nogc_env env, void* data, void* hint) { In your PR. It won't be neeed without NAPI_EXPERIMENTAL but would be needed later. If that is not possible let @mhdawson know and he'll take a look at doing it. |
Thanks! I opened a separate PR: #53233 and also added the change here to see if it works with Clang. |
Refs: #53212 (comment) PR-URL: #53233 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
Refs: #53212 (comment) PR-URL: #53233 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
Here's a run for the daily |
Problem with LTO build:
|
There's also:
/cc @gengjiawen who added the job. |
@targos need to resolve conflicts. |
This puts us closer to what V8 actively supports. GCC is still covered a lot by Jenkins CI. Co-authored-by: Moshe Atlow <[email protected]>
@MoLow can you review one last time? The setup is working now. |
Landed in db09f62 |
This puts us closer to what V8 actively supports. GCC is still covered a lot by Jenkins CI. Co-authored-by: Moshe Atlow <[email protected]> PR-URL: #53212 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
Refs: nodejs#53212 (comment) PR-URL: nodejs#53233 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
This puts us closer to what V8 actively supports. GCC is still covered a lot by Jenkins CI. Co-authored-by: Moshe Atlow <[email protected]> PR-URL: nodejs#53212 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
Refs: nodejs#53212 (comment) PR-URL: nodejs#53233 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
This puts us closer to what V8 actively supports. GCC is still covered a lot by Jenkins CI. Co-authored-by: Moshe Atlow <[email protected]> PR-URL: nodejs#53212 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
Refs: #53212 (comment) PR-URL: #53233 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
Refs: #53212 (comment) PR-URL: #53233 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
This puts us closer to what V8 actively supports. GCC is still covered a lot by Jenkins CI. Co-authored-by: Moshe Atlow <[email protected]> PR-URL: #53212 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
This puts us closer to what V8 actively supports. GCC is still covered a lot by Jenkins CI. Co-authored-by: Moshe Atlow <[email protected]> PR-URL: #53212 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
Refs: nodejs/node#53212 (comment) PR-URL: nodejs/node#53233 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
Refs: nodejs/node#53212 (comment) PR-URL: nodejs/node#53233 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
This puts us closer to what V8 really supports.
GCC is still covered a lot by Jenkins CI.