-
Notifications
You must be signed in to change notification settings - Fork 60
[wasm] detect WASM EXIT on v8 and nodejs #1014
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
Conversation
lewing
left a comment
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.
🤞
|
Let's hope this doesn't set everything on fire |
|
I hope that we would see the fire on the dependency update PR first |
|
It might be beneficial to invest into updating the E2E tests for WASM in this repo so that you don't need to postpone testing for runtime dependency PRs: #920 I can merge this PR for you through the timeouts in the Apple queues but the WASM leg is failing and that one should probably go green first. |
|
Copying '/datadisks/disk1/work/C4620A42/w/A473091C/u/test-main.js' Somehow, there is no expected subfolder |
|
@pavelsavara I didn't check your payload really, only noticed the mismatch in the name of the workitem so I fixed that. I can put a root-level directory in the zip that is uploaded. |
|
Ideally to latest, I have |
|
@pavelsavara is there a runtime issue that has details of what is being fixed here? |
I created new issue for it. I hope I have draft of fix on runtime repo dotnet/runtime#85542 |
@pavelsavara I guess we can update that, not sure if anyone else depends on that. I logged dotnet/arcade#13399 and ETA would be next Wednesday. |
|
@pavelsavara would it better to run this in one of your docker containers? I understand that's how you do it in runtime, right? |
eventually yes, but I don't want to invest my time into it right now. Is that easy to switch ? I think this one would be fine cc @radekdoulik |
|
@pavelsavara I think it might be easy enough by setting the |
|
using this helix image should be fine. we use it for most (if not all) helix runs in the runtime repo it should have the latest V8 https://github.com/dotnet/dotnet-buildtools-prereqs-docker/blob/main/src/ubuntu/18.04/helix/webassembly/Dockerfile#L45-L48 |
|
WASM.Helix.SDK.Tests.proj currently uses ubuntu 20.04, so you probably want to use |
radekdoulik
left a comment
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 besides the old v8 issue
|
I see |
|
@pavelsavara the notation is described here - https://github.com/dotnet/arcade/tree/main/src/Microsoft.DotNet.Helix/Sdk#docker-support |
|
Should we get this in? |
thanks for fixing it! I think we should merge it. @radical ? |
|
I'm seeing the introduce log in dotnet/runtime#96094:
|
|
This is the same test on runtime main Log It also didn't log exit. And this was reason why we wanted this detection. But we perhaps need to not set I will create another PR to make this softer. |
Makes detection of dotnet/runtime#85675 easier
also fixes #920