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] Fix tests failing due to trimming - System.Runtime.Loader #52088

Closed
wants to merge 1 commit into from

Conversation

radical
Copy link
Member

@radical radical commented Apr 30, 2021

Test failing with:

fail: [FAIL] System.Runtime.Loader.Tests.AssemblyLoadContextTest.LoadAssemblyByPath_ValidUserAssembly
info: System.TypeLoadException : Could not resolve type with token 01000014 from typeref (expected class 'System.MulticastDelegate' in assembly 'System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a')
info:    at System.Reflection.RuntimeModule.GetTypes()
info:    at System.Reflection.Assembly.GetTypes()
info:    at System.Reflection.Assembly.get_DefinedTypes()
info:    at System.Runtime.Loader.Tests.AssemblyLoadContextTest.LoadAssemblyByPath_ValidUserAssembly()
info:    at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

This is failing while trying to enumerate the types on a dependent test
assembly - System.Runtime.Loader.Test.Assembly. This assembly is
loaded by the test explicitly, into an ALC. But it is loaded from the
assembly's embedded resources.

The main test project(System.Runtime.Loader.Tests.csproj) has:

    <ProjectReference Include="System.Runtime.Loader.Test.Assembly\System.Runtime.Loader.Test.Assembly.csproj"
                      ReferenceOutputAssembly="false"
                      OutputItemType="EmbeddedResource" />
  • This will cause the referenced project to get built

    • And the assembly(System.Runtime.Loader.Test.Assembly.dll) added to @(EmbeddedResource)
    • but since we have ReferenceOutputAssembly=false, it won't be added
      to the list of references used for compiling the main tests assembly
      (System.Runtime.Loader.Tests).
      • but this also means that the test assembly from the referenced
        project does not get added to the list of assemblies to be linked.
  • The result of all this is that when trimming:

    • the linker has the set of assemblies to trim, edits the
      assemblies, and updates the metadata tokens (type references, for
      example) to be consistent within that set.
    • Since, *Test.Assembly.dll was not in the above set, it still
      has some old tokens which are no longer valid.
      • and that fails when resolving the tokens in the *Test.Assembly
        to assemblies like System.Runtime.
  • the fix is to add such assemblies to @(ManagedAssemblyToLink), so
    they are included in the linker's set.

Fixes #51893

Test failing with:

```
fail: [FAIL] System.Runtime.Loader.Tests.AssemblyLoadContextTest.LoadAssemblyByPath_ValidUserAssembly
info: System.TypeLoadException : Could not resolve type with token 01000014 from typeref (expected class 'System.MulticastDelegate' in assembly 'System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a')
info:    at System.Reflection.RuntimeModule.GetTypes()
info:    at System.Reflection.Assembly.GetTypes()
info:    at System.Reflection.Assembly.get_DefinedTypes()
info:    at System.Runtime.Loader.Tests.AssemblyLoadContextTest.LoadAssemblyByPath_ValidUserAssembly()
info:    at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
```

This is failing while trying to enumerate the types on a dependent test
assembly - `System.Runtime.Loader.Test.Assembly`. This assembly is
loaded by the test explicitly, into an ALC. But it is loaded from the
assembly's embedded resources.

The main test project(`System.Runtime.Loader.Tests.csproj`) has:
```xml
    <ProjectReference Include="System.Runtime.Loader.Test.Assembly\System.Runtime.Loader.Test.Assembly.csproj"
                      ReferenceOutputAssembly="false"
                      OutputItemType="EmbeddedResource" />
```

- This will cause the referenced project to get built
    - And the assembly(`System.Runtime.Loader.Test.Assembly.dll`) added to `@(EmbeddedResource)`
    - but since we have `ReferenceOutputAssembly=false`, it won't be added
      to the list of references used for compiling the main tests assembly
      (`System.Runtime.Loader.Tests`).
      - but this also means that the test assembly from the referenced
        project does *not* get added to the list of assemblies to be linked.

- The result of all this is that when trimming:
    - the linker has the set of assemblies to trim, edits the
      assemblies, and updates the metadata tokens (type references, for
      example) to be consistent within that set.
    - Since, `*Test.Assembly.dll` was *not* in the above set, it still
      has some old tokens which are no longer valid.
      - and that fails when resolving the tokens in the `*Test.Assembly`
        to assemblies like `System.Runtime`.

- the fix is to add such assemblies to `@(ManagedAssemblyToLink)`, so
  they are included in the linker's set.

Fixes dotnet#51893
@radical radical added arch-wasm WebAssembly architecture area-Build-mono trimming-for-aot `EnableAggressiveTrimming=true` used for running tests with AOT labels 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

Test failing with:

fail: [FAIL] System.Runtime.Loader.Tests.AssemblyLoadContextTest.LoadAssemblyByPath_ValidUserAssembly
info: System.TypeLoadException : Could not resolve type with token 01000014 from typeref (expected class 'System.MulticastDelegate' in assembly 'System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a')
info:    at System.Reflection.RuntimeModule.GetTypes()
info:    at System.Reflection.Assembly.GetTypes()
info:    at System.Reflection.Assembly.get_DefinedTypes()
info:    at System.Runtime.Loader.Tests.AssemblyLoadContextTest.LoadAssemblyByPath_ValidUserAssembly()
info:    at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

This is failing while trying to enumerate the types on a dependent test
assembly - System.Runtime.Loader.Test.Assembly. This assembly is
loaded by the test explicitly, into an ALC. But it is loaded from the
assembly's embedded resources.

The main test project(System.Runtime.Loader.Tests.csproj) has:

    <ProjectReference Include="System.Runtime.Loader.Test.Assembly\System.Runtime.Loader.Test.Assembly.csproj"
                      ReferenceOutputAssembly="false"
                      OutputItemType="EmbeddedResource" />
  • This will cause the referenced project to get built

    • And the assembly(System.Runtime.Loader.Test.Assembly.dll) added to @(EmbeddedResource)
    • but since we have ReferenceOutputAssembly=false, it won't be added
      to the list of references used for compiling the main tests assembly
      (System.Runtime.Loader.Tests).
      • but this also means that the test assembly from the referenced
        project does not get added to the list of assemblies to be linked.
  • The result of all this is that when trimming:

    • the linker has the set of assemblies to trim, edits the
      assemblies, and updates the metadata tokens (type references, for
      example) to be consistent within that set.
    • Since, *Test.Assembly.dll was not in the above set, it still
      has some old tokens which are no longer valid.
      • and that fails when resolving the tokens in the *Test.Assembly
        to assemblies like System.Runtime.
  • the fix is to add such assemblies to @(ManagedAssemblyToLink), so
    they are included in the linker's set.

Fixes #51893

Author: radical
Assignees: -
Labels:

arch-wasm, area-Build-mono, trimming-for-aot

Milestone: -

@radical
Copy link
Member Author

radical commented Apr 30, 2021

There are other ways to fix this too, like reference the dependent project assemblies.

@MichalStrehovsky
Copy link
Member

System.Runtime.Loader is pretty fundamentally incompatible with trimming. I don't know whether it's the best idea to twist its arms into working.

ManagedAssemblyToLink is explicitly documented as "do not add stuff to this". ("Do not add or remove items to/from ManagedAssemblyToLink, because the SDK computes this set during publish and expects it not to change. The supported metadata is:")

Cc @vitek-karas

@vitek-karas
Copy link
Member

Dynamic assembly loading will not work with trimming. I don't see much value in testing this in that mode since users should not do this. And if they it's bound to break (as visible in this test). In theory we could fix this by adding support for listing "potential plugins" in the SDK - which is what this fix is effectively trying to do, but that doesn't exist. So hacking it in tests doesn't seem right.

I think we should simply disable this test for scenarios which involve trimming.

@radical
Copy link
Member Author

radical commented Apr 30, 2021

@lewing @steveisok if we can't use trimming, then what should we do about AOT for this?

@steveisok
Copy link
Member

I'd agree with Vitek. If it doesn't make sense, skip it.

@radical
Copy link
Member Author

radical commented Apr 30, 2021

I'd agree with Vitek. If it doesn't make sense, skip it.

Disable System.Runtime.Loader.Tests completely?

@lewing
Copy link
Member

lewing commented Apr 30, 2021

Dynamic assembly loading will not work with trimming. I don't see much value in testing this in that mode since users should not do this. And if they it's bound to break (as visible in this test). In theory we could fix this by adding support for listing "potential plugins" in the SDK - which is what this fix is effectively trying to do, but that doesn't exist. So hacking it in tests doesn't seem right.

I think we should simply disable this test for scenarios which involve trimming.

Blazor does late loaded assemblies (but not by path) so this is definitely a thing that happens in practice. That said I think it's handled right now by linking normally and then not including the late loaded assemblies in the list to load initially which should work.

@radical
Copy link
Member Author

radical commented Apr 30, 2021

@vitek-karas @MichalStrehovsky Just trying to understand this better. In case of wasm+aot, users would have to depend on trimming too, IIUC, because it takes too long to aot without that. In that case, would System.Runtime.Loader be unsupported?

@radical
Copy link
Member Author

radical commented Apr 30, 2021

Ok, so in case of blazor you have to specify such late loaded assemblies with: <BlazorWebAssemblyLazyLoad Include="GrantImaharaRobotControls.dll" />

.. so, blazor will be able to pick these up, and have them trimmed correctly.

But this would be a problem, at least for non-blazor wasm scenarios, or possibly any others that use trimming. So, maybe we need to have a clear way to add such assemblies for linking, even from project references. And document that? And fix it the same way for the tests here?

@radical
Copy link
Member Author

radical commented Apr 30, 2021

@lambdageek is adding some tests for hot reload on wasm. He will handle the case for trimming. I can close this PR then, and the specific tests will remain disabled.

@lambdageek
Copy link
Member

Chatted with @radical on Discord - we shouldn't disable the whole teatsuite. The hot reload tests will eventually land in here (#51144) and they do their own feature checks to eventually try to run on wasm

@vitek-karas
Copy link
Member

We simply didn't have a need to support trimming for plugin-enabled apps. Mainly because it would require the app to know about all of its plugins at the time the app is produced. In which case you can just build the plugins with the app and ship everything together. In which case trimming will work. You can then choose not to ship some assemblies in the app - but that happens after the whole thing is built.

The one missing capability is to add assemblies to the app without having a ProjectReferece or PackageReference or some other reference. I'm not sure - it might be solvable in today's SDK by some clever usage of ExcludeAssets and similar metadata items.

@steveisok
Copy link
Member

@radical Can you please update if this is still good?

@radical
Copy link
Member Author

radical commented Jun 1, 2021

Based on #52088 (comment), and #52088 (comment) , closing this PR.

@radical radical closed this Jun 1, 2021
@radical radical deleted the fix-embed-dll branch June 1, 2021 16:36
@ghost ghost locked as resolved and limited conversation to collaborators Jul 1, 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 trimming-for-aot `EnableAggressiveTrimming=true` used for running tests with AOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[wasm][eat] System.Runtime.Loader test failing due to missing System.MulticastDelegate in System.Runtime
6 participants