-
Notifications
You must be signed in to change notification settings - Fork 166
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 CI is not working #886
Comments
Worth mentioning this is happening for any + all attempts to run CI right now |
To be clear, this is happening for all runs of node-test-commit-v8-linux, not @joransiu @jBarz do I recall you guys talking about something like this on Slack? |
This started failing after https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/656565.
|
Have put the work around into the job and kicked off a run to see if it resolves the issue. |
Assuming it's https://ci.nodejs.org/job/node-test-commit-v8-linux/912/ then it looks like it worked. We should leave this open to track the proper fix though, sounds like John is making progress. |
Looks like work around did the trick, lets leave this open until the proper fix is in place. |
@jBarz @joransiu this looks different than this issue https://bugs.chromium.org/p/chromium/issues/detail?id=764087&can=2&start=0&num=100&q=&colspec=ID%20Pri%20M%20Stars%20ReleaseBlock%20Component%20Status%20Owner%20Summary%20OS%20Modified&groupby=&sort= But it did remind me to ask where we are on this one. At some point I should remove the work around. |
It should be fixed now. All we need to do is adding an extra env variable. |
Looks like the temporary patch is now failing 😭 I'm hacking on the CI job right now to make a permanent fix |
Was not able to fix it. I attempted to revert all of the hacks we had currently put in and added Same error reported in OP |
Using https://ci.nodejs.org/job/node-test-commit-v8-linux-fedora24/nodes=fedora24,v8test=v8test/398/console as reference. |
Ok found some stuff:
The best I got is to revive the test on the |
@refack were you testing with |
In multiple variations with no success (with But now I see that we used to roll back
|
@MylesBorins need someone with job edit capas. That should do it. |
My copy and paste skills are pretty sharp, will have a go |
https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/962/ New log: # temporary work around for https://github.com/nodejs/build/issues/886
cd depot_tools
git checkout 633a9830a9fce875d277b248c6e6fb14d72e8183
git revert -n 509776e Fails with: + cd depot_tools
+ git checkout 633a9830a9fce875d277b248c6e6fb14d72e8183
Note: checking out '633a9830a9fce875d277b248c6e6fb14d72e8183'.
You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.
If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:
git checkout -b new_branch_name
HEAD is now at 633a983... [gsutil.vpython] Update packages.
+ git revert -n 509776e
error: could not revert 509776e... [gsutil] run through "vpython" (2)
hint: after resolving the conflicts, mark the corrected paths |
sorry new SHA - 77b7687e4837223f820e1e98c369d36696f3f2b3 |
git checkout 633a9830a9fce875d277b248c6e6fb14d72e8183
git revert -n 77b7687e4837223f820e1e98c369d36696f3f2b3 || true Seems healthier. Actually the previous SHA was fine, there was a conflict, but we're not committing so not an issue. https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/967/ 🙏 |
I think it should be:
And don't add the |
P.S. the conflict is the crux of the issue |
Okay, my flight is delayed so lots of time to fix this... Seems to be working better. |
FWIW this, and other things like the FreeBSD bugs, are really regressions in V8 and tools on platforms that Node supports but V8 doesn't (so for V8 I guess not technically regressions, but whatever). I'm not really sure how to solve this, but if we're running Node on V8 to make sure V8 commits don't break Node, we should probably think about platform support. Most regressions arent going to be on xLinux or macOS, they're going to be on less-used platforms. |
I think we can with not too much effort remove the dependency on I have a partial recipe for syncing V8 without |
Holly molly! It's green https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/968/ |
Looks like the git changes broke stuff... failures across all environments rn |
@MylesBorins that's a |
nvm... I think I broke stuff (in my branch)... Will check in again in am and confirm |
@john-yan I tried putting in the final fix by adding
But it is still failing on the PPC platforms. See this job which is a clone of the regular CI job with the change in place: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux-test-mdawson/configure |
Is it just me or does it look like the job is no longer running all of the tests ? |
Ignore my last comment, its just that they run in the middle of the job since there is recompilation for the hashseed test. |
open https://bugs.chromium.org/p/chromium/issues/detail?id=773857 to fix depot_tools on ppc |
Hello, depot_tools should be fixed on ppc linux now.
|
Thank @john-yan 🎉 |
Main job updated, run to validate works as expected: https://ci.nodejs.org/job/node-test-commit-v8-linux/999/ |
Looks good closing. |
Looks like an error with glient fetching v8
/cc @mhdawson @nodejs/v8
The text was updated successfully, but these errors were encountered: