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] Integrate naot-llvm into workload manifest #101801

Closed
wants to merge 35 commits into from

Conversation

maraf
Copy link
Member

@maraf maraf commented May 2, 2024

TODO

  • Don't re-enable browser workload from emscripten workload manifest
  • Add WBT test
    • Setup NativeAOT-LLVM dependencies on Helix
    • Wasi
    • Browser
  • Needs to drop ImportRuntimeIlcPackageTarget target override in naot-llvm
  • Split the version property out to separate file to make it overridable by automation
  • Update Microsoft.NETCore.App (KnownFrameworkReference) version to correct version of JS interop generator or ship the generator other way (fixed in naot branch)
  • Prepare gh action to update the version
  • Should we print some message/warning about experimental feature?
  • Should we once in a while check for newer version LLVM packages?

An alternative approach as a result for this experimentation

<UsingBrowserRuntimeWorkload>false</UsingBrowserRuntimeWorkload>
<UsingWasiRuntimeWorkload>false</UsingWasiRuntimeWorkload>
<UsingEmscriptenWorkload>true</UsingEmscriptenWorkload>

In combination with current NativeAOT-LLVM setup these props will allow to use emscripten workload with NativeAOT-LLVM

@maraf maraf added arch-wasm WebAssembly architecture area-Build-mono labels May 2, 2024
@maraf maraf added this to the 9.0.0 milestone May 2, 2024
@maraf maraf self-assigned this May 2, 2024
Copy link
Contributor

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

@maraf
Copy link
Member Author

maraf commented Jun 21, 2024

@am11
Copy link
Member

am11 commented Jun 21, 2024

Prepare gh action to update the version
Should we once in a while check for newer version LLVM packages?

Maybe @dotnet/runtime-infrastructure might have ideas but sounds like it could use DARC subscription for runtimelab -> runtime package sync (like the regular code flow), instead of a standalone GH action?

</PropertyGroup>

<ItemGroup Condition="'$(_IsUsingNativeAOT)' == 'true' and ('$(RuntimeIdentifier)' == 'browser-wasm' or '$(RuntimeIdentifier)' == 'wasi-wasm')">
<KnownILCompilerPack Remove="Microsoft.DotNet.ILCompiler" />
Copy link
Member

Choose a reason for hiding this comment

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

This is me being nit-picky - I would move this down near where we set KnownRuntimePack and the like.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

runtimelab and the runtime experimental feed does not have infrastructure for servicing.

I do not think that introducing a dependency on runtimelab from dotnet/runtime is a good idea. If you really want to do that, you will have to work through the impact on servicing workflows (and likely introduce a bunch of extra infrastructure and process for dotnet/runtimelab to make that work).

@jkotas
Copy link
Member

jkotas commented Jul 8, 2024

If you would like to promote the current runtimelab project to "shipping with dotnet/runtime", the proper way to do that is by integrating it to dotnet/runtime.

@maraf
Copy link
Member Author

maraf commented Jul 9, 2024

If you would like to promote the current runtimelab project to "shipping with dotnet/runtime", the proper way to do that is by integrating it to dotnet/runtime.

We don't want to promote the runtimelab project to shipping with dotnet.
The goal is to make the integration easier. Primarily by enabling usage of our build of emscripten and remove the need for disabling workload resolvers, but since the deeper integration on wasm scope worked, I tried to go deeper. We have the wasm-experimental workload which seemed like a reasonable place to introduce such integration.

Anyway, if the current scope is too deep, I'm happy to reduce it to follow the original goals.

@jkotas
Copy link
Member

jkotas commented Jul 9, 2024

It is fine to make small target changes in dotnet/runtime repo to support the projects that we run in runtimelab.

dotnet/runtime repo should not be taking dependencies on runtimelab artifacts or feeds. Referencing https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-experimental feed in dotnet/runtime is a red flag. runtimelab can reference dotnet/runtime feeds, but not the other way around.

@dotnet-policy-service dotnet-policy-service bot removed this from the 9.0.0 milestone Aug 8, 2024
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.

@maraf maraf reopened this Aug 14, 2024
# Conflicts:
#	eng/Versions.props
#	eng/testing/scenarios/BuildWasmAppsJobsList.txt
#	src/mono/wasm/Wasm.Build.Tests/Common/BuildEnvironment.cs
@@ -1,6 +1,7 @@
<!-- Licensed to the .NET Foundation under one or more agreements. The .NET Foundation licenses this file to you under the MIT license. -->
<Project>
<PropertyGroup>
<_IsUsingNativeAOT Condition="'$(PublishAot)' == 'true'">true</_IsUsingNativeAOT>
Copy link
Member

Choose a reason for hiding this comment

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

Can we not use PublishAot == true condition directly? MSBuild evaluates it the same way (since there is no use in any target), so it seems redundant.

Copy link
Member Author

@maraf maraf Aug 14, 2024

Choose a reason for hiding this comment

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

We can. I wanted to extract it, because we weren't sure if using PublishAot for this highly experimental thing is the right choice.

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.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants