Skip to content
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] Clean emsdk setup #106403

Open
wants to merge 40 commits into
base: main
Choose a base branch
from

Conversation

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented Aug 14, 2024

Fixes #105439.

This PR is enhanced version of previous attempt of cleanup done in 2 PRs: in emsdk: dotnet/emsdk#851 and runtime: #105612. What it does:

  • removes overwriting the emsdk_env* script
  • removes overwriting emscripten config file
  • skips moving node and python nugets to EMSDK_PATH location
  • instead it makes sure the required paths are set before provisioning emscripten - running pre_emsdk_env (better names are welcome)

Justification of changes in EmSdkRepo.Defaults.props:

  • It does not break lib tests/WBT/samples and we expect node and python nuget packages to be in nuget directory.
  • Moving the changes to WasmApp.InTree.props was not successful - the .props file is imported too late to matter in the stage of checking if node / python paths are set.
  • Moving the changes to WasmApp.InTree.targets would require a workaround that would make the target run before _SetupEmscripten task and also before EmSdkRepo.Defaults.props import that is "caching" an error about python path on Windows. Such a workaround would not be as straightforward as setting the paths in EmSdkRepo.Defaults.props.

@ilonatommy ilonatommy added arch-wasm WebAssembly architecture area-Build-mono labels Aug 14, 2024
@ilonatommy ilonatommy self-assigned this Aug 14, 2024
@ilonatommy
Copy link
Member Author

ilonatommy commented Aug 16, 2024

I have a problem with native build on Windows.

build-native.proj does not have access to any of properties set in:

  • src\mono\Directory.Build.props (ProvisionEmscriptenDir, EMSDK_PATH)
  • nor in src\mono\mono.proj (_PreEmsdkEnvScriptPath),

so when it executes
"C:\Users\user\source\repos\runtime-fork\src\native\libs\build-native.cmd" wasm Release outconfig net9.0-browser-Release-wasm -os browser icudir "C:\Users\user\source\repos\nugets\microsoft.netcore.runtime.icu.transport\9.0.0-rc.1.24373.1/runtimes/browser-wasm/native",
it does it with empty env variables. In other words, .emscripten config reads the following:
DOTNET_EMSCRIPTEN_LLVM_ROOT, DOTNET_EMSCRIPTEN_NODE_JS, DOTNET_EMSCRIPTEN_BINARYEN_ROOT as empty.

I have to find a way to append required variables to _BuildNativeEnvironmentVariables in build-native project (not from fixed paths - then it works)

<DOTNET_EMSCRIPTEN_LLVM_ROOT>C:\Users\user\source\repos\runtime-fork\src\mono\browser\emsdk\bin</DOTNET_EMSCRIPTEN_LLVM_ROOT>
<_BuildNativeEnvironmentVariables>$(_BuildNativeEnvironmentVariables);DOTNET_EMSCRIPTEN_LLVM_ROOT=$(DOTNET_EMSCRIPTEN_LLVM_ROOT);EM_LLVM_ROOT=$(DOTNET_EMSCRIPTEN_LLVM_ROOT)</_BuildNativeEnvironmentVariables>

edit:
actually the only thing we're missing is value of EMSDK_PATH in the native build project, with it the build would be fixed.

Copy link
Member

@radekdoulik radekdoulik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ilonatommy
Copy link
Member Author

ilonatommy commented Sep 2, 2024

Connected failure:
https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-106403-merge-377c58235651459b9d/WasmTestOnV8-ST-System.Runtime.Tests/1/console.9f62bb6b.log?helixlogtype=result

Building a proxy for the original test project, to AOT on helix. In order to do that, this recreates the original inputs for the *wasm* part of the build. See /root/helix/work/workitem/e/publish/ProxyProjectForAOTOnHelix.proj, and /root/helix/work/workitem/e/publish/ProxyProjectForAOTOnHelix.props. **
/root/helix/work/correlation/build/wasm-shared/WasmApp.Common.targets(508,5): error : Specified Emscripten sdk at $(EMSDK_PATH)=/root/helix/work/correlation/build/emsdk/ is missing some paths: $(EmscriptenNodeToolsPath)=runtime.-.microsoft.netcore.runtime.wasm.node.transport/tools/- . SDK is required for AOT'ing assemblies. 

command build and run to reproduce (BuildAOTTestsOnHelix=true):

build.sh -ci -arch wasm -os browser  -s mono+libs+host+packs+libs.tests -c Release /p:ArchiveTests=true /p:MonoEnableAssertMessages=true /p:BrowserHost=linux /p:RunSmokeTestsOnly=True   /p:InstallV8ForTests=true /p:EnableAggressiveTrimming=true /p:BuildAOTTestsOnHelix=true /p:RunAOTCompilation=true /p:AotHostArchitecture=x64 /p:AotHostOS=linux 
./dotnet.sh build -bl /t:Test src/libraries/System.Runtime/tests/System.Runtime.Tests/System.Runtime.Tests.csproj /p:Configuration=Release /p:BuildAOTTestsOnHelix=true /p:RunAOTCompilation=true /p:AotHostArchitecture=x64 /p:AotHostOS=linux  /p:TargetOS=browser /p:RunSmokeTestsOnly=True  > tests.txt

Edit:
even if we pass the property values from building machine to helix, the packages are not in same location.

@ilonatommy ilonatommy added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-wasm WebAssembly architecture area-Build-mono NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[wasm] move emsdk setup from runtime to emsdk tooling
3 participants