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

Root entire assembly for non-exe projects #3131

Closed
wants to merge 1 commit into from
Closed

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Nov 23, 2022

After fixing the root behavior in #3124, the linker update dotnet/runtime#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 #3124.

@MichalStrehovsky
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

sbomer commented Nov 28, 2022

Closing per feedback. I'm now trying to fix this in dotnet/runtime: dotnet/runtime@01ef2b0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants