Skip to content

Conversation

@Numpsy
Copy link
Contributor

@Numpsy Numpsy commented May 5, 2024

Enable both trimming and AOT analysis for the whole solution, to see if it finds any more possible issues on top of the ReflectionStateGraphicsFactory situaiton.

Running it locally, I get this set of additional warnings:

image

I'm not sure If those can all be fixed inside the library, or if it might need the functions to be attributed so that the issues get reported to external callers (I haven't looked at it in any more details than this)

@BobLd
Copy link
Collaborator

BobLd commented May 9, 2024

@Numpsy let me have a look at these exceptions here, pretty sure it comes from code I wrote a while ago

@BobLd
Copy link
Collaborator

BobLd commented May 9, 2024

I came to the same conclusion. Don't think it can actually be fixed.

I think something we could think about is to create a separate NuGet package for the classes in UglyToad.PdfPig.DocumentLayoutAnalysis.Export...

@EliotJones what do you think?

@EliotJones
Copy link
Member

I came to the same conclusion. Don't think it can actually be fixed.

I think something we could think about is to create a separate NuGet package for the classes in UglyToad.PdfPig.DocumentLayoutAnalysis.Export...

@EliotJones what do you think?

Is it possible to use a preprocessor directive to remove these classes when targetting AOT builds? Not really familiar with AOT stuff.

@BobLd
Copy link
Collaborator

BobLd commented May 9, 2024

ew might be able to create our own #if AOT... haven't looked into that though

@BobLd
Copy link
Collaborator

BobLd commented May 9, 2024

I'd also be fine removing these from the main code base and creating/managing a UglyToad.PdfPig.DocumentLayoutAnalysis.Export NuGet package

@Numpsy
Copy link
Contributor Author

Numpsy commented May 9, 2024

I'm unclear if it's possible to avoid any of the issues by using DynamicallyAccessedMembersAttribute inside the library, but otherwise it's possible to annotate public methods with RequiresUnreferencedCode such that anyone calling the functions AND doing Trimming or AOT will get a warning themselves, e.g.

image

A consuming application might then be able to avoid some issues by configuring its trimming settings to ensure that anything that it's actually using is always kept.

@BobLd
Copy link
Collaborator

BobLd commented May 9, 2024

I think the RequiresUnreferencedCode is the most sensible. We are passing the warning from the xml lib to the consuming app. Makes sense to me

@EliotJones
Copy link
Member

Sounds good to me, presumably the consumer only gets warnings if actually using annotated methods/members?

@BobLd
Copy link
Collaborator

BobLd commented May 14, 2024

Yes this is how I understand it too. It's up to the consumer to make sure everything okay with AOT. We can have a 2nd look once people start using exporters in AOT

@Numpsy
Copy link
Contributor Author

Numpsy commented May 15, 2024

Sounds good to me, presumably the consumer only gets warnings if actually using annotated methods/members?

That's the idea. (e.g. I didn't see these warnings when I created #664 as I wasn't calling the functions from my app)

@Numpsy
Copy link
Contributor Author

Numpsy commented Jun 12, 2024

I tried annotating some more of the functions, and that resulted in a couple of cases of this:


build: src/UglyToad.PdfPig.DocumentLayoutAnalysis/Export/AltoXmlTextExporter.cs#L115
Member 'UglyToad.PdfPig.DocumentLayoutAnalysis.Export.AltoXmlTextExporter.Get(Page)' with 'RequiresUnreferencedCodeAttribute' implements interface member 'UglyToad.PdfPig.DocumentLayoutAnalysis.Export.ITextExporter.Get(Page)' without 'RequiresUnreferencedCodeAttribute'. 'RequiresUnreferencedCodeAttribute' annotations must match across all interface implementations or overrides.

As ITextExporter isn't annotated (and there are implementatoins of that that don't have any warnings anyway).
Saying that, I'm not sure what ITextExporter is used for - it doesn't seem to be referenced anywhere inside the library outside of the classes that implement it?

@Numpsy
Copy link
Contributor Author

Numpsy commented Nov 20, 2024

I'll close this in favour of #939

@Numpsy Numpsy closed this Nov 20, 2024
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