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

Extensions do not work on full trimming (audit Activator usage) #2010

Closed
Perksey opened this issue Apr 4, 2024 · 4 comments · Fixed by #2115
Closed

Extensions do not work on full trimming (audit Activator usage) #2010

Perksey opened this issue Apr 4, 2024 · 4 comments · Fixed by #2115
Labels
bug Something isn't working

Comments

@Perksey
Copy link
Member

Perksey commented Apr 4, 2024

Easy issue. May need some linking attributes littered some places. Reported in Discord.

@alexrp
Copy link
Collaborator

alexrp commented Apr 13, 2024

Took a quick peek. The vast majority of cases just need [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] on the type parameter and they should be good.

Cases like this, though, will never be trimming-compatible, I don't think:

public static bool TryAdd(string assemblyName)
{
try
{
var asm = Assembly.Load(assemblyName);
var attr = asm.GetCustomAttribute<InputPlatformAttribute>();
if (attr is null)
{
return false;
}
if (!typeof(IInputPlatform).IsAssignableFrom(attr.Type))
{
return false;
}
Add((IInputPlatform) Activator.CreateInstance(attr.Type, true));
return true;
}
catch
{
return false;
}
}

Unless we can somehow abuse InputPlatformAttribute to signal to the linker to keep the type... 🤔

@alexrp
Copy link
Collaborator

alexrp commented Apr 13, 2024

Since we build for TFMs that don't have [DynamicallyAccessedMembers], would you prefer to pull in PolySharp, or use #ifs?

@Perksey
Copy link
Member Author

Perksey commented Apr 13, 2024

Yeah the paths you linked are only used in non-AOT scenarios - users are already expected to use GlfwWindowing.Use and friends to register the platforms today on AOT.

I agree with your assessment as well, it seemed like this was just a case of adding attributes. It would be nice if we didn’t need to use the Activator at all, which in theory could be possible using static abstracts in interfaces but only on newer platforms, so I guess we can deal with it for now.

So in any case we’ll need to add some TFM hackery. We probably shouldn’t pull in PolySharp as we try to avoid pulling in non-.NET Foundation packages in core projects. Recommend an #ifdef or something similar to how I handled RequiresLocationAttribute.

Perksey added a commit that referenced this issue Apr 15, 2024
@Perksey
Copy link
Member Author

Perksey commented Apr 15, 2024

Modified the CoreRTTest project to add a TryGetExtension call with PublishAot=true, PublishTrimmed=true, and TrimMode=full. Fixed with the changes in #2115

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants