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

[release/6.0-preview3] Backport of Add support for using emscripten from packages #50077 #50227

Merged
merged 1 commit into from
Mar 25, 2021

Conversation

lewing
Copy link
Member

@lewing lewing commented Mar 25, 2021

Backport of #50077 to release/6.0-preview3

/cc @radical @pranavkm

Customer Impact

Enables Blazor AOT using packaged versions of the emscripten sdk. Will enable a much smoother experience for Blazor
AOT.

Testing

Tested in main.

Risk

Low. Changes AOT build scenarios that are new in this preview

* [wasm] Add support for using emscripten from packages

When using a workload that has the emscripten packages, certain
properties get set. If that isn't set then we fallback to using
`$(EMSDK_PATH)`.

This should allow being able to use emscripten from workloads, but for
other cases where we depend on an installed emscripten, that should work
too.

* Fix setting PATH on windows

* Using msbuild NormalizeDirectory to construct dir path

* fix setting PATH on windows, when paths have trailing slash

* [wasm] Improve checking emscripten from workload, and EMSDK_PATH

- If a workload pack is being used, then ignore `EMSDK_PATH`
- And error in that order too
- Emit better error messages

Example messages:
- Missing python dir when relinking:

`warning : Specified Emscripten sdk at $(EMSDK_PATH)=. is missing some paths: $(EmscriptenPythonToolsPath)=/Users/radical/dev/r3/artifacts/bin/Wasm.Build.Tests/net6.0-Release/browser-wasm/e10djxv0.wdr/python/3.7.4-2_64bit/ . Emscripten SDK is required for building native files.`

- Missing node directory, or the wrong version dir:

`warning : Specified Emscripten sdk at $(EMSDK_PATH)=/Users/radical/dev/r3/src/mono/wasm/emsdk is missing some paths: $(EmscriptenNodeToolsPath)=/Users/radical/dev/r3/src/mono/wasm/emsdk/node/12.18.1_64bit/ . Emscripten SDK is required for building native files.`

- Empty `EMSDK_PATH`

`Error : Could not find Emscripten sdk at $(EMSDK_PATH)=/Users/radical/dev/r3/src/mono/wasm/emsdk/. Emscripten SDK is required for AOT'ing assemblies.`

- Bad `EMSDK_PATH`

`error : Specified Emscripten sdk at $(EMSDK_PATH)=. is missing some paths: $(EmscriptenPythonToolsPath)=/Users/radical/dev/r3/artifacts/bin/Wasm.Build.Tests/net6.0-Release/browser-wasm/l2mcsipf.isn/python/3.7.4-2_64bit/ . Emscripten SDK is required for AOT'ing assemblies.`

* [wasm] Don't hardcode versions for python/node bundled in emsdk

.. and listen to @lewing's suggestions!

* [wasm] GetDirectories doesn't like non-existant paths

* Fix the earlier fix

* improve errors when some paths are missing

* cleanup

* debugging on ci

* [wasm] emscripten uses system python on linux, so don't try to find it in emscripten sdk
@lewing lewing added Servicing-consider Issue for next servicing release review area-Build-mono labels Mar 25, 2021
@lewing lewing changed the title [release/6.0-6review3] Backport of #50077 [release/6.0-preview3] Backport of #50077 Mar 25, 2021
@danmoseley danmoseley changed the title [release/6.0-preview3] Backport of #50077 [release/6.0-preview3] Backport of Add support for using emscripten from packages #50077 Mar 25, 2021
@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Mar 25, 2021
@leecow leecow added this to the 6.0.0 milestone Mar 25, 2021
@lewing
Copy link
Member Author

lewing commented Mar 25, 2021

@Anipik this is one is needed and the failures are not related to Wasm only builds.

@lewing lewing requested a review from Anipik March 25, 2021 18:46
@Anipik Anipik merged commit 6000597 into dotnet:release/6.0-preview3 Mar 25, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Build-mono Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants