-
Notifications
You must be signed in to change notification settings - Fork 29
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] move emsdk config and emsdk_env
files from runtime to emsdk
#851
Conversation
emsdk_env
files from runtime to emsdk
@@ -1,9 +1,10 @@ | |||
import os | |||
|
|||
LLVM_ROOT = os.path.expanduser(os.getenv('DOTNET_EMSCRIPTEN_LLVM_ROOT', '')) | |||
NODE_JS = os.path.expanduser(os.getenv('DOTNET_EMSCRIPTEN_NODE_JS', '')) | |||
BINARYEN_ROOT = os.path.expanduser(os.getenv('DOTNET_EMSCRIPTEN_BINARYEN_ROOT', '')) |
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.
we're setting these here, I don't think we can just remove them: https://github.com/dotnet/runtime/blob/28c4dce2c6a9a3619faa612095ed2125d03fbecd/src/mono/browser/build/BrowserWasmApp.targets#L202-L204
i.e. we need to make sure the toolchain still works when using the nuget packages on end-user machines.
I'm not sure everything uses the emsdk_env scripts for example.
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.
In the current state we are overwriting the contents of this file anyway, during provisioning, before it gets used.
i.e. we need to make sure the toolchain still works when using the nuget packages on end-user machines.
I produced nugets in this repo, copied them to runtime repo and tested how the build process goes and it worked fine. If that succeeded, where else it could go wrong? What other tests do you propose?
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 runtime build is kinda special. I'd recommend trying a normal blazor wasm build.
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, tested blazor:
- run
- publish
- AOT publish
No errors detected.
closing in favor of dotnet/runtime#106403 |
Fixes dotnet/runtime#105439.
This PR:
emsdk_env*
scripts.Best way to read is commit-wise.
After the nuget with this change will get distributed, we will be able to remove
_EmscriptenPaths
from runtime repo, see:https://github.com/dotnet/runtime/blob/ebbebaca1184940f06df609d5a40096f628200ce/src/mono/mono.proj#L244
etc, done by dotnet/runtime#105612.
This is a non-breaking change, tested locally.
The runtime's changes of the scripts touched here were introduced in dotnet/runtime#100266.