-
Notifications
You must be signed in to change notification settings - Fork 71
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
Review state of canary in CI #223
Comments
@targos FYI we'd need to add ICU 71 (the new minimum) to the sharedlib containers for the CI dynamically linked ICU builds to pass. |
@richardlau that would be in https://github.com/nodejs/build/blob/master/ansible/roles/docker/templates/ubuntu2004_sharedlibs.Dockerfile.j2 ? I can try to make a PR. |
@targos we're actually still using https://github.com/nodejs/build/blob/master/ansible/roles/docker/templates/ubuntu1804_sharedlibs.Dockerfile.j2 but yeah we should probably start moving the sharedlib testing onto Ubuntu 20.04 (or jump to 22.04). We could also remove ICU 64 from the containers once Node.js 12 goes EOL. |
I don't get the Windows debug failure (https://ci.nodejs.org/job/node-compile-windows-debug/11712/nodes=win-vs2019/console).
|
On my PC:
|
I commented on the V8 issue related to TurboShaft: https://bugs.chromium.org/p/v8/issues/detail?id=12783#c2 |
Edit: canary branch was just updated... |
New run with 5dab1f0 CI: https://ci.nodejs.org/job/node-test-commit/54350/ |
Build fails on ppc and s390:
@nodejs/platform-ppc @nodejs/platform-s390 Edit: can be reproduced in direct V8 build so it's not specific to our GYP config: https://ci.nodejs.org/job/node-test-commit-v8-linux/4761/nodes=rhel8-s390x,v8test=v8test/console |
This doesn't seem to be a problem with V8 but rather an issue with building |
@miladfarca I see. Would you be able to investigate what we are doing wrong? These flags have been passed from node-v8/tools/v8_gypfiles/toolchain.gypi Lines 288 to 322 in f316cfc
|
Is https://chromium.googlesource.com/v8/v8/+/28b5d299845f6ca289647241badd08844b51bb7c relevant? (Apologies if you've already aware/have seen, I just had a cursory glance and it seems like it could be related.) |
@BethGriggs I was not aware, thanks! It might be this commit, but I don't see how. |
The problem is that https://chromium.googlesource.com/v8/v8/+/28b5d299845f6ca289647241badd08844b51bb7c has moved the arch detection to
The V8 part (using gn/ninja) of https://ci.nodejs.org/job/node-test-commit-v8-linux/4761/nodes=rhel8-s390x,v8test=v8test/consoleFull succeeded -- it's the Node.js build that is now broken as we only have the
My worry would be if we now need extra definitions for including |
FWIW I'm checking if richardlau@85d6c5b improves things. |
Well at least on s390x we're on a new failure: https://ci.nodejs.org/job/node-test-commit-v8-linux/4767/nodes=rhel8-s390x,v8test=v8test/console
cc @nodejs/platform-s390 |
@miladfarca suggests that one might be resolved by https://chromium-review.googlesource.com/c/v8/v8/+/3755141. |
https://ci.nodejs.org/job/node-test-commit-v8-linux/4769 is:
All three platforms now fail with #231. |
Looks good! Are you able to upstream the patch? |
No, but @miladfarca has on my behalf 🙇♂️ : https://chromium-review.googlesource.com/c/v8/v8/+/3757944 |
Thanks @richardlau / @miladfarca! New run with 8627566 |
I'll open a new issue for the next round of reviews. |
Commit: ae3407f
node-test-commit
: https://ci.nodejs.org/job/node-test-commit/53135/node-test-commit-v8-linux
: https://ci.nodejs.org/job/node-test-commit-v8-linux/4652/The text was updated successfully, but these errors were encountered: