-
Notifications
You must be signed in to change notification settings - Fork 44
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
Update the emscripten version to 2.0.23 #681
Update the emscripten version to 2.0.23 #681
Conversation
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.
@Devika-Tantry some feedback:
- The
Error: No tool or SDK found by name '2.0.20'.
is likely due to the version of the emsdk tools used. We checkout a specific commit (web reference, web-windows reference) because the tools can be unreliable sometimes. Try changing to one of the newer commits from emscripten-core/emsdk. I would recommend the latest emsdk 2.0.23 release commit. - In the PR comment can you link to the associated ASW work item
- Can you run an ASW PR build with your local vireo change and make sure to enable the [VireoTest] suite for the build (see the section "Trigger the Web Module Squad tests").
- Include a link to the ASW build result run with [VireoTest] in the PR
- (optional but it's cool) Sujay shared a screenshot of being able to debug Vireo inside Chrome Canary which is awesome! Can you add a screenshot showing debugging in the browser to the PR description? Shows the benefit of updating the toolchain :)
For the flag changes could you add comments in the PR for those lines that points to the change description? For example it looks like this is a reference to the NO_EXIT_RUNTIME change.Went ahead and added some comments as I was interested in the changes.
Looks like the PR is still in-work but a couple more notes:
|
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.
I have not additional questions. Please address the feedback given by @rajsite.
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.
Thanks. Would like to take another look with feedback addressed.
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.
Discussed in a meeting, some outcomes:
- For WriteDoubleToMemory we should use ConvertFloatToInt instead and use it for all the num types, including 64bit
- The Double.Test.js should be updated to use the saturating values where relevant
- We should undo the failed ResizeDimension handling preferring to let the app segfault. We should create a Tech Debt to create an abort mechanism that can abort the runtime quickly closer to the allocation failure (looks like we can call the C abort function).
- The ExecuteSlicesUntilClumpsFinshed.Test.js error handling test should use a try catch around the execute slices and assert that the error happens. We should create a Tech Debt task to make an instruction that can intentionally fail with a seg fault and update the test to call that instruction.
|
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.
Looking good, just some minor tweaks remaining
@Devika-Tantry We have been creating tech debt items as GitHub issues so far: https://github.com/ni/VireoSDK/issues |
Created two issues (issue1, issue2) in github to track the issue and added these issue links in corresponding AzDo for squad tracking purpose. |
@Devika-Tantry minor comment, won't block the PR, but thanks for the work on upgrading the compiler version! Make sure to add yourself to the AUTHORS list :) |
@Devika-Tantry Looks like the PR is almost ready 👍
Edit: |
These are some timeout error seems like these are common in PR build and these all not related to vireo changes. If we rerun, different set of tests fail due to timeout. |
ASW work item : Update emscripten tooling used to build Vireo
Ran ASW PR with this updated package for test.
Changes:
-
BINARYEN_TRAP_MODE=js
is depreciated.-
-s NO_EXIT_RUNTIME=1
is by default set to 1 so removed.-
EXTRA_EXPORTED_RUNTIME_METHODS
renamed toEXPORTED_RUNTIME_METHODS
- Updated the flag in debug build to
-g
so now debug output will have DWARF info.
Note: With this updated vireo package we can run the web gvi's in Chrome Canary.(with setup mentioned here).
-g
applies on debug build so we have to use debug wasm to directly interact with CPP files.In WebVI, we use Viero SDK wasm build. Screenshots of putting breakpoints in Vireo SDK CPP files and watching values in the browser in WebVI.
