-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[wasm] Enable wasm relinking by default for Release config #49251
Conversation
Test can be added once #47683 is merged. |
A couple of thoughts.
|
Also, native linking is very slow, so making it the default will hurt user experience. |
yes, In interpreter mode we don't require it unless the default icall/pinvoke tables are missing symbols (extra symbols are fine) and that will only happen if we've added references to native assets in which case we'll need to relink to include them and add to the tables. And ideally we we only do that if the native assets change. |
I don't think there is much concern for the release/deploy configuration to be slow because it runs more optimizations. |
What do you mean by "interpreter mode"?
Could you enumerate those, or at least the ones that we definitely want in this PR? |
- But set to `false` if `$(PublishTrimmed)` is not true Fixes: dotnet#48349
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.
Needs to be false by default if emsdk is not present
Something like this? I'm guessing that we would want to avoid silently skipping relinking, because |
yes |
.. and update tests to track the changes.
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.
The logic looks good and I'm ok with landing this as is to let packages flow to aspnetcore but let's not close #48349 yet. The messages need some work and we will need a follow up that abstracts the conditional logic in a slightly different way.
Yeah, the conditional logic will get simplified, but there are bunch of PRs in flight right now, after which it should be sorted out. re:messages, yes!! |
@radical - I think your change has broken the https://dev.azure.com/dnceng/public/_build?definitionId=861 All jobs after your check-in now show: Job build_Browser_wasm_release_Runtime_Release depends on unknown job mono_browser_offsets. Could you please take a look? Cc @directhex who is still the test cop AFAIK, otherwise he'll forward as appropriate. Cc @dotnet/runtime-infrastructure |
Set default value for
$(WasmBuildNative)
based on:If
$(WasmBuildNative)
is already set (eg. by user project) totrue
, then error ifEMSDK_PATH
is unsetif unset, and
$(PublishTrimmed) == false
, then set to falseif unset, and
$(Configuration) == "Release"
, then set to trueIf it is
$(WasmBuildNative) == true
after above steps, and emsdk is missing=> then emit a warning, and set it to
false
Fixes: #48349