Skip to content

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.

@ilonatommy ilonatommy requested a review from steveisok August 19, 2024 10:32
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
@dotnet-policy-service
Copy link
Contributor

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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