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

Add unmanaged assembly skipping to ILStrip task #106267

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

jkurdek
Copy link
Member

@jkurdek jkurdek commented Aug 12, 2024

Feeding unmanaged assembly to ILStrip resulted in task failure. This change allows ILStrip to skip unmanaged assemblies. The final list of processed assemblies is to be found at UpdatedAssemblies output. Changes to xamarin-macios will be done in separate PR.

Contributes to #101967

Copy link
Contributor

Tagging subscribers to this area: @directhex, @matouskozak
See info in area-owners.md if you want to be subscribed.

@jkurdek
Copy link
Member Author

jkurdek commented Aug 12, 2024

cc/ @vitek-karas

catch (ImageFormatException)
{
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

It might be helpful to catch general failures and report them as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

The caller of this function has a try catch block reporting all the other failures.

@steveisok
Copy link
Member

steveisok commented Aug 12, 2024

Should this and the aot compiler task share the same way? Not intending to hold this up btw.

using var assemblyFile = File.OpenRead(assemblyPath);
using PEReader reader = new(assemblyFile, PEStreamOptions.Default);
if (!reader.HasMetadata)
{
Log.LogMessage(MessageImportance.Low, $"Skipping unmanaged {assemblyPath} for AOT");
continue;
}

@vitek-karas
Copy link
Member

If we really wanted to solve this for everybody then ILLink has the same problem and has its own solution as well. And I assume NativeAOT has it as well - but I don't know what the solution there is. Would be nice to have one solution for everybody. But I definitely don't want to block this fix on that.

@jkurdek
Copy link
Member Author

jkurdek commented Aug 12, 2024

I did it like this to avoid adding unnecessary overhead. I do agree it would be much nicer to have a one way of filtering the unmanaged assemblies for all the tasks. Maybe following the ILLink way and using a separate MSBuild task to filter the assemblies would be fit them all solution?

I will be merging this one so that that we can start working on xamarin-macios fix.

@jkurdek jkurdek merged commit 78f5e87 into dotnet:main Aug 12, 2024
138 of 140 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants