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] Use source built emsdk packages for the runtime build #100266

Merged
merged 47 commits into from
May 9, 2024

Conversation

steveisok
Copy link
Member

This change moves away from using upstream emsdk to our source built packages as part of the wasm build.

@steveisok
Copy link
Member Author

@akoeplinger I hand edited the eng/Version.xml/props. Do we have to do anything w/ darc to help resolve what's failing?

@akoeplinger
Copy link
Member

It was just a typo :)

I'm wondering though whether we need to add one of these source-build .Intermediate dependencies too?


<ItemGroup>
<EmsdkFiles Condition="'%(PackageReference.Identity)' != 'runtime.$(_portableHostOS)-$(BuildArchitecture).Microsoft.NETCore.Runtime.Wasm.Node.Transport' and '%(PackageReference.Identity)' != 'Microsoft.NET.Runtime.Emscripten.3.1.34.Python.win-$(BuildArchitecture)'"
Include="$(NuGetPackageRoot)\$([System.String]::Copy(%(PackageReference.Identity)).ToLowerInvariant())\%(PackageReference.Version)\tools\**" />
Copy link
Member

@akoeplinger akoeplinger Mar 26, 2024

Choose a reason for hiding this comment

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

I think instead of NuGetPackageRoot you should be able to use GeneratePackagePath on the PackageReference

Copy link
Member

Choose a reason for hiding this comment

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

I tried that and it gets complicated as the package names are based on other properties, so the generated property name is for example PkgMicrosoft_NET_Runtime_Emscripten_3_1_34_Cache_$(_portableHostOS)-$(BuildArchitecture). It would work much better if the package path was added to the PackageReference items instead.

@lewing
Copy link
Member

lewing commented Mar 27, 2024

I want to start using the packages asap but I don't think we want to use individual props for each item here when the package name needs to change whenever we do an emsdk bump.

@steveisok
Copy link
Member Author

I want to start using the packages asap but I don't think we want to use individual props for each item here when the package name needs to change whenever we do an emsdk bump.

Change the package name out of emsdk then?

Works for the most part out of the box, but fails on the part where we npm install after
the libraries build. The reason for the failure is our node transport package is incomplete. If
you replace it with a legit node, the build works fully.
@@ -101,7 +101,7 @@ extends:
image: mcr.microsoft.com/dotnet-buildtools/prereqs:centos-stream8

browser_wasm:
image: mcr.microsoft.com/dotnet-buildtools/prereqs:cbl-mariner-2.0-webassembly-20230913040940-1edc1c6
image: mcr.microsoft.com/dotnet-buildtools/prereqs:cbl-mariner-2.0-webassembly
Copy link
Member

Choose a reason for hiding this comment

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

is there anything else left in the -webassembly image that we need or can we switch to the plain mariner image?

Copy link
Member

Choose a reason for hiding this comment

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

Wasmtime and WASI SDK (which we ignore at the moment)?

Copy link
Member

Choose a reason for hiding this comment

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

isn't that for the wasi leg? I think this only deals with browser

Copy link
Member

Choose a reason for hiding this comment

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

I have opened #102069 to try that. I am not sure whether we still run some tests with v8.

eng/Versions.props Outdated Show resolved Hide resolved
Copy link
Member

@lewing lewing left a comment

Choose a reason for hiding this comment

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

The versions.props suggestion needs to land but it looks like we are almost there

eng/Versions.props Outdated Show resolved Hide resolved
eng/Version.Details.xml Outdated Show resolved Hide resolved
@radekdoulik radekdoulik merged commit 5263848 into dotnet:main May 9, 2024
157 of 159 checks passed
<runtimewinx64MicrosoftNETCoreRuntimeWasmNodeTransportPackageVersion>9.0.0-alpha.1.24175.1</runtimewinx64MicrosoftNETCoreRuntimeWasmNodeTransportPackageVersion>
<EmsdkPackageVersion>$(MicrosoftNETRuntimeEmscriptenVersion)</EmsdkPackageVersion>
<NodePackageVersion>$(runtimewinx64MicrosoftNETCoreRuntimeWasmNodeTransportPackageVersion)</NodePackageVersion>
<EmsdkVersion>3.1.34</EmsdkVersion>
Copy link
Member

Choose a reason for hiding this comment

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

please move these up next to MicrosoftNETWorkloadEmscriptenCurrentManifest90100TransportVersion

<InstallCmd Condition="'$(HostOS)' == 'windows'">powershell -NonInteractive -command &quot;&amp; $(InstallCmd); Exit $LastExitCode &quot;</InstallCmd>
<ActivateCmd Condition="'$(HostOS)' == 'windows'">powershell -NonInteractive -command &quot;&amp; $(ActivateCmd); Exit $LastExitCode &quot;</ActivateCmd>
<PythonCmd Condition="'$(HostOS)' == 'windows'and '$(TargetsBrowser)' == 'true'">setlocal EnableDelayedExpansion &amp;&amp; call &quot;$([MSBuild]::NormalizePath('$(EMSDK_PATH)', 'emsdk_env.bat'))&quot; &amp;&amp; !EMSDK_PYTHON!</PythonCmd>
<_EmsdkPaths Condition="'$(HostOS)' != 'windows'">
Copy link
Member

Choose a reason for hiding this comment

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

please move the scripts into separate files, they shouldn't be inside mono.proj

Include="$(NuGetPackageRoot)\$([System.String]::Copy(%(PackageReference.Identity)).ToLowerInvariant())\%(PackageReference.Version)\tools\**" />
<NodeFiles Condition="'%(PackageReference.Identity)' == 'runtime.$(_portableHostOS)-$(BuildArchitecture).Microsoft.NETCore.Runtime.Wasm.Node.Transport'"
Include="$(NuGetPackageRoot)\$([System.String]::Copy(%(PackageReference.Identity)).ToLowerInvariant())\%(PackageReference.Version)\tools\$(_portableHostOS)-$(BuildArchitecture)\**" />
<PythonFiles Condition="'$(HostOS)' == 'windows' and '%(PackageReference.Identity)' == 'Microsoft.NET.Runtime.Emscripten.3.1.34.Python.win-$(BuildArchitecture)'"
Copy link
Member

Choose a reason for hiding this comment

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

this shouldn't hardcode 3.1.34 here

Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
…100266)

* Use the emsdk transport packages to build wasm instead of a cloned emsdk

Works for the most part out of the box, but fails on the part where we npm install after
the libraries build. The reason for the failure is our node transport package is incomplete. If
you replace it with a legit node, the build works fully.

* Pull in node transport package as opposed to the one packaged in emsdk. Streamline package deps

* Make paths friendly for windows

* Work in windows transport packages, copy python to its own directory, and adjust a bunch of paths

* Bump to latest version of node that contains icu

* Add windows deps and add DOTNET_EMSCRIPTEN_NODE_PATH because windows can't see npm without it

* Bump emscripten packages to the latest

* Fix typos in Version.Details.xml

* Switch out to the plain mariner container to ensure no EMSDK already exists

* Revert back to the browser-wasm docker image and bump to the latest

* Container type-o

* Fix condition where the node path isn't set properly

* Provision even when building offsets

* Fix goofy paths

* Update new template to use the latest browser-wasm container

* Don't put python.exe on the path for windows, but the folder instead

* So that was a bad idea. May have to have emsdk_env.cmd have two entries

* Fix offsets generation for browser

* Fix emsdk's python path

It is not versioned anymore as we use the content of our nugets

* Fix EMSDK_PATH on non-windows platforms

* Do not add link flags to compile flags

To avoid:

    C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(825,5): error : emcc: warning: linker setting ignored during compilation: 'EXPORT_ES6' [-Wunused-command-line-argument] [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
    C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(825,5): error : emcc: warning: linker setting ignored during compilation: 'EXPORT_EXCEPTION_HANDLING_HELPERS' [-Wunused-command-line-argument] [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]

* Set the emsdk paths relative to the script location

* Escape few characters and fix the added script

* Fix .emscripten script

* Use EM_CONFIG intead of __file__

* Fix emsdk_env.cmd script

* Revert "Do not add link flags to compile flags"

This reverts commit eb19ade.

* Revert changes in package-lock.json

* Feedback + cleaning

* More cleaning

* Try not to use the replace in the EMSDK_PATH

* Better property names

* Use package and emsdk version properties

Co-authored-by: Larry Ewing <[email protected]>

* Use the updated properties

* Fix emsdk version

* Use a single version of emsdk and take node version from flow

* Update eng/Version.Details.xml

Co-authored-by: Larry Ewing <[email protected]>

---------

Co-authored-by: Alexander Köplinger <[email protected]>
Co-authored-by: Radek Doulik <[email protected]>
Co-authored-by: Radek Doulik <[email protected]>
Co-authored-by: Larry Ewing <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Jun 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants