search

dotnet / linker

388 stars 126 forks source link

Root entire assembly for non-exe projects #3131

Closed sbomer closed 1 year ago

sbomer commented 1 year ago

After fixing the root behavior in https://github.com/dotnet/linker/pull/3124, the linker update https://github.com/dotnet/runtime/pull/78020 is failing to build wasm test projects because they PublishTrimmed non-exe test projects which don't have an entry point:

ILLink : error IL1034: Root assembly 'System.Runtime.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' does not have entry point.

This isn't really a supported scenario so we could work around it in dotnet/runtime, but in this case I think it makes sense to put the logic in the official targets here. This will root the entire assembly for non-exe projects, which is what the "default" root mode did (for non-exes) before https://github.com/dotnet/linker/pull/3124.

MichalStrehovsky commented 1 year ago

In general it's preferable to do workarounds for unsupported things local to where we do the unsupported thing (so in the runtime repo in this case).

It is extra code to maintain and extra potential bugs. This diff would introduce a bug for OutputType=WinExe.

vitek-karas commented 1 year ago

I agree with Michal - the SDK doesn't support trimming anything but apps right now. Adding support for trimming libraries is much more complicated and thus I would not even try. I would prefer this change to be in the runtime.

Also - if we were to support classlibs in the SDK this way, then the correct solution would be to do "library" rooting for the assembly, not "all". The wasm tests in runtime relied on all because they're tests, they don't want to be trimmed, but I would prefer it they made this requirement explicit.

sbomer commented 1 year ago

Closing per feedback. I'm now trying to fix this in dotnet/runtime: https://github.com/dotnet/runtime/pull/78020/commits/01ef2b0b5baf036fb2cf5a6b83a87a0db8996338.