-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Fix an issue in wasm nortti build and add minimal build support for vcpkg #24012
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
base: main
Are you sure you want to change the base?
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.
You can commit the suggested changes from lintrunner.
Co-authored-by: Yulong Wang <[email protected]>
Co-authored-by: Yulong Wang <[email protected]>
### Description There are slightly mismatch for the build flags for Web build pipeline when using vcpkg. A [fix](#24012) is on the way but for now we need to disable vcpkg for the next patch release. ### Motivation and Context <!-- - Why is this change required? What problem does it solve? - If it fixes an open issue, please link to the issue here. -->
### Description There are slightly mismatch for the build flags for Web build pipeline when using vcpkg. A [fix](#24012) is on the way but for now we need to disable vcpkg for the next patch release. ### Motivation and Context <!-- - Why is this change required? What problem does it solve? - If it fixes an open issue, please link to the issue here. -->
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.
You can commit the suggested changes from lintrunner.
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.
You can commit the suggested changes from lintrunner.
Testing.... Testing Plan
|
The generate_vcpkg_triplets_for_emscripten function in tools\python\util\vcpkg_helpers.py didn't process the enable_rtti condition. So, when it is true, we should add -fno-rtti to cxxflags
Fix an issue related to DISABLE_EXCEPTION_CATCHING. Make it clear that there are three modes:
Set enable_minimal_onnx_build=True, enable_wasm_exception_catching=False
Set enable_minimal_onnx_build=False, enable_wasm_exception_catching=True
Set enable_minimal_onnx_build=False, enable_wasm_exception_catching=False
Debug build should only use the second one.
In release build by default emscripten disables catching C++ exceptions (specifically, emitting catch blocks). That's the second case. In a normal release build(what we ship),
But we do not want to rely on what default value is. So the vcpkg_helper.py script still explicitly set DISABLE_EXCEPTION_CATCHING to 1.
In onnxruntime_webassembly.cmake currently we have
But I think we need to set DISABLE_EXCEPTION_THROWING to 1 when the build is in the first mode(No EH). This PR also resolves #24279 , because vcpkg has native support for cross-compiling. Users do not need to specific a custom protoc path.