-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
[v8.x] Cherry-pick fix for make test-v8
from master
#21534
Conversation
On Node.js v8.x, gn will pass a sysroot parameter to clang to use a downloaded sysroot files while running `make test-v8`. Recently, chromium build tools switched to use Debian sid sysroot files instead of Debian jessie. This patch updates our V8 GYP files to conform with those changes. Ref: nodejs#21433 PR-URL: nodejs#21494 Refs: nodejs#21433 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Leave a 👍 in this comment if you agree this should be fast-tracked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I see failures on PPC and s390 related to
I don't think those should block this PR as the failure is unrelated to both the original problem or the change. @mmallick-ca could you give us an update on where our investigation is into the gmock issue for PPC and s390 ? |
Feel free to land once this has enough approvals. I'll be traveling and won't be able to do so before Wednesday. |
@mmarchini For some reason it also failed on the x86 machines which is of concern. Not sure why it did not fail that way in earlier tests? |
Definitely looks related 😕 |
Running again after cleaning up the workspace on ubuntu1604-intel-64: https://ci.nodejs.org/job/node-test-commit-v8-linux/1487/ Hope this fixes the issue. |
@mhdawson we think its failing because git is not able to complete the checkout . This is why the gmock files are not present. Currently testing a potential fix. |
@mmallick-ca any updates? |
@ofrobots The gitattributes file associated with the v8 master branch has been updated to treat files with *.png as binary. We have verified locally that after this change that build completes successfully. The error was occurring due to git treating certain *.png files in the master branch as text files during "fetch v8". This results in some modifications in the png files such that the subsequent checkout of the appropriate branch fails. |
@mmallick-ca thanks! New V8-CI: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/1492/ |
Failed on x86 with
|
@ofrobots I'm still trying to figure out why it's failing on x86. |
Looks like something changed upstream again, which fixed our V8 CI, and now this PR is breaking it ¯_(ツ)_/¯ V8 CI passing on v8.x-staging: https://ci.nodejs.org/job/node-test-commit-v8-linux/1493/ |
/cc @hashseed, fyi upstream changes that have been causing churn here. |
I'll close this and revert #21494. I couldn't find what changed upstream, but the |
Cherry-picking fc45b16 from
master
to unbreak V8 CI on v8.x-staging.Closes: #21433
cc @nodejs/release @nodejs/v8-update @targos @ofrobots @hashseed
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes