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-preview4][wasm] Fix Blazor AOT builds inside Visual Studio #52078

Merged
merged 9 commits into from
May 3, 2021

Conversation

lewing
Copy link
Member

@lewing lewing commented Apr 29, 2021

Description

Fix Blazor AOT builds inside Visual Studio by multi-targeting the build tasks.

Customer Impact

Without these fixes the build fails when run inside Visual Studio causing that experience to be worse than other build methods (command line and VS Code)

Regression

No, these tasks are new in this release

Testing

Manual Testing against the 6.0.100-preview.4.21229.17 sdk and VS Version 16.10.0 Preview 2.1

Risk

Very Low. The changes only impact the AOT workload

radical added 2 commits April 29, 2021 16:05
- In case of `WasmAppBuilder.dll`, we were packaging only the task assembly, and
  `System.Reflection.MetadataLoadContext` for net472, same as net6.0 .

- But for net472, VS fails with errors like:

```
C:\Program Files\dotnet\packs\Microsoft.NET.Runtime.WebAssembly.Sdk\6.0.0-preview.4.21222.10\Sdk\WasmApp.targets(424,4): Error MSB4018: The "PInvokeTableGenerator" task failed unexpectedly.
System.IO.FileNotFoundException: Could not load file or assembly 'System.Reflection.Metadata, Version=1.4.5.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies. The system cannot find the file specified.
File name: 'System.Reflection.Metadata, Version=1.4.5.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'
   at System.Reflection.MetadataLoadContext.LoadFromStreamCore(Stream peStream)
   at System.Reflection.MetadataLoadContext.LoadFromAssemblyPath(String assemblyPath)
   at System.Reflection.PathAssemblyResolver.Resolve(MetadataLoadContext context, AssemblyName assemblyName)
   at System.Reflection.MetadataLoadContext.TryFindAssemblyByCallingResolveHandler(RoAssemblyName refName)
   at System.Reflection.MetadataLoadContext.ResolveToAssemblyOrExceptionAssembly(RoAssemblyName refName)
   at System.Reflection.MetadataLoadContext.TryResolveAssembly(RoAssemblyName refName, Exception& e)
   at System.Reflection.MetadataLoadContext.TryGetCoreAssembly(String coreAssemblyName, Exception& e)
   at System.Reflection.TypeLoading.CoreTypes..ctor(MetadataLoadContext loader, String coreAssemblyName)
   at System.Reflection.MetadataLoadContext..ctor(MetadataAssemblyResolver resolver, String coreAssemblyName)
   at PInvokeTableGenerator.GenPInvokeTable(String[] pinvokeModules, String[] assemblies) in /Users/radical/dev/r2/src/tasks/WasmAppBuilder/PInvokeTableGenerator.cs:line 42
   at PInvokeTableGenerator.Execute() in /Users/radical/dev/r2/src/tasks/WasmAppBuilder/PInvokeTableGenerator.cs:line 28
   at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute()
   at Microsoft.Build.BackEnd.TaskBuilder.<ExecuteInstantiatedTask>d__26.MoveNext()
```

- So, bundle all the dependent assemblies from the publish folder
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@lewing lewing changed the title [testing][wasm] Backport task fixes for VS [release/6.0-preview4][wasm] Fix Blazor AOT builds inside Visual Studio Apr 30, 2021
@lewing lewing added the Servicing-consider Issue for next servicing release review label Apr 30, 2021
@lewing lewing marked this pull request as ready for review April 30, 2021 17:57
@lewing lewing added the arch-wasm WebAssembly architecture label Apr 30, 2021
@ghost
Copy link

ghost commented Apr 30, 2021

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

Issue Details

Description

Fix Blazor AOT builds inside Visual Studio by multi-targeting the build tasks.

Customer Impact

Without these fixes the build fails when run inside Visual Studio causing that experience to be worse than other build methods (command line and VS Code)

Regression

No, these tasks are new in this release

Testing

Manual Testing against the 6.0.100-preview.4.21229.17 sdk and VS Version 16.10.0 Preview 2.1

Risk

Very Low. The changes only impact the AOT workload

Author: lewing
Assignees: -
Labels:

Servicing-consider, arch-wasm, area-Build-mono

Milestone: -

lewing pushed a commit to lewing/runtime that referenced this pull request Apr 30, 2021
src/tasks/AotCompilerTask/MonoAOTCompiler.csproj Outdated Show resolved Hide resolved
src/tasks/AotCompilerTask/MonoAOTCompiler.csproj Outdated Show resolved Hide resolved
src/tasks/WasmAppBuilder/WasmAppBuilder.csproj Outdated Show resolved Hide resolved
src/tasks/WasmAppBuilder/WasmAppBuilder.csproj Outdated Show resolved Hide resolved
Comment on lines +27 to +31
<!-- needed for publishing with multi-targeting. We are publishing essentially to get the SR.MetadataLoadContext.dll :/ -->
<ItemGroup>
<_PublishFramework Include="$(TargetFrameworks)" />
</ItemGroup>
<MSBuild Projects="$(MSBuildProjectFile)" Targets="Publish" Properties="TargetFramework=%(_PublishFramework.Identity)" />
Copy link
Member

Choose a reason for hiding this comment

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

This looks a bit scary. Aren't the dependencies that need to be packaged included in the build output folder?

Copy link
Member

Choose a reason for hiding this comment

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

In case of net472, yes, but not for net6.0 :|

Copy link
Member

Choose a reason for hiding this comment

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

The existing code uses Publish target to get the assembly.

src/tasks/WasmAppBuilder/WasmAppBuilder.csproj Outdated Show resolved Hide resolved
src/tasks/WasmAppBuilder/WasmAppBuilder.csproj Outdated Show resolved Hide resolved
radical and others added 6 commits April 30, 2021 14:59
Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>
Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>
Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>
Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>
Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>
@lewing
Copy link
Member Author

lewing commented Apr 30, 2021

Validated a local build of 2926086 packages successfully

lewing added a commit that referenced this pull request May 1, 2021
* Port additional fixes in #52078 main

* Update src/tasks/AotCompilerTask/MonoAOTCompiler.csproj

Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>

* Update src/tasks/WasmAppBuilder/WasmAppBuilder.csproj

Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>

* Update src/tasks/WasmAppBuilder/WasmAppBuilder.csproj

Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>

* Apply suggestions from code review

Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>

* Use a property for net472

* Use consistent separator character

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>

Co-authored-by: Ankit Jain <radical@gmail.com>
Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
@lewing lewing added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels May 3, 2021
@lewing
Copy link
Member Author

lewing commented May 3, 2021

Approved in email.

@mmitche mmitche merged commit bfd6048 into dotnet:release/6.0-preview4 May 3, 2021
@lewing lewing deleted the wasm-multitarget branch May 3, 2021 18:09
@ghost ghost locked as resolved and limited conversation to collaborators Jun 2, 2021
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 Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants