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

AssemblyName.xml descriptors are not processed from copyused assemblies #1410

Closed
sbomer opened this issue Aug 4, 2020 · 8 comments
Closed
Assignees
Labels
Milestone

Comments

@sbomer
Copy link
Member

sbomer commented Aug 4, 2020

dotnet/runtime#40336 mentions warnings generated by invalid XML files in System.PrivateCoreLib. Such warnings show up with <TrimMode>link</TrimMode> but not <TrimMode>copyused</TrimMode>. The difference is due to the check
https://github.com/mono/linker/blob/8f32ed57a05eeb0bae3790c4db75b316c3da76c7/src/linker/Linker.Steps/BlacklistStep.cs#L126
which prevents processing of XMLs for copyused assemblies - hiding the invalid XML (which was pointing to COM members only present on Windows, for example).

I think the XML files should be processed on-demand (similar to #1164) only when an assembly is marked.

@marek-safar
Copy link
Contributor

I don't think we can process them on demand. Most XML formats allow specifying any assembly in their XML so we need to read them and process them. It'd also complicate the scenario when doing on-demand processing we need to load a new assembly and process it again.

I think that switch is missing copyused value.

@marek-safar marek-safar added this to the .NET 6.0 milestone Nov 13, 2020
@marek-safar marek-safar added task and removed proposal labels Nov 13, 2020
@sbomer sbomer changed the title Embedded XML processing should be done on demand AssemblyName.xml descriptors are not processed from copyused assemblies Feb 18, 2021
@sbomer
Copy link
Member Author

sbomer commented Feb 18, 2021

@marek-safar Could we get rid of the support for AssemblyName.xml entirely? The recommendation recently has been to call it ILLink.Descriptors.xml. If it helps, we could warn if the linker discovers an AssemblyName.xml to make sure it doesn't bite people.

@marek-safar
Copy link
Contributor

I think we'd need to phase it. This has been the original way to add descriptors and there is a decent chance that some of Xamarin libs will use it. We should add a warning first.

@sbomer
Copy link
Member Author

sbomer commented Feb 19, 2021

Copying @mateoatr's comment:

Does anyone else other than dotnet/runtime use the AssemblyName.xml for descriptors?

Looks like dotnet/wpf uses it too: https://github.com/dotnet/wpf/blob/78be6d01ca468a6d137af0681859890544210987/src/Microsoft.DotNet.Wpf/src/PresentationCore/PresentationCore.csproj#L1470-L1474

@vitek-karas
Copy link
Member

@sbomer - given that we now process descriptors on demand (effectively) can this be fixed? That is should we add CopyUsed to the switch here: https://github.com/mono/linker/blob/4f68312aeb4b8b906bee4e06438020ad6721c522/src/linker/Linker/EmbeddedXmlInfo.cs#L116-L124

@sbomer
Copy link
Member Author

sbomer commented Jun 17, 2021

We could, but I'm not sure it's worth it. I'm slightly worried that it could somehow regress .NET5 scenarios either in terms of size or by activating latent XML errors. Probably rare, but possible. This issue only exists for the combination of copyused and AssemblyName.xml, both of which are not recommended.

If we do fix it we should do a sanity check by publishing a .NET5 or .netcoreapp3 WPF app. What do you think?

@vitek-karas
Copy link
Member

If you think this should be very rare I would probably be OK taking the change.
That said my intent was to resolve this issue - one way or another. We could also choose to not fix this and turn that into a "feature".

@vitek-karas
Copy link
Member

Let's "not fix this", we haven't run into this for more than a year, so it's not a problem.

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

No branches or pull requests

3 participants