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.

Sign up for GitHub

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

Jump to bottom

Dotnet trimmer testing #144

Open
tranb3r opened this issue Mar 3, 2023 · 9 comments
Open

Dotnet trimmer testing #144

tranb3r opened this issue Mar 3, 2023 · 9 comments
Labels
enhancement New feature or request

Comments

@tranb3r
Copy link
Contributor

tranb3r commented Mar 3, 2023

Hi,

When migrating playground/tests to dotnet maui, the <AndroidLinkMode>Full</AndroidLinkMode> property has been removed from the release build.
I don't know if it's been done on purpose, but I think you could add now the corresponding dotnet trimmer property: <TrimMode>full</TrimMode>.

Also, are you sure the Preserve attributes in the plugin code are still needed?
Maybe it's worth testing it again with dotnet trimmer. What do you think?

Thanks!
@tranb3r tranb3r mentioned this issue Mar 12, 2023
Closed
@tranb3r
Copy link
Contributor Author

tranb3r commented Sep 8, 2023

When trimming my application, android's MyFirebaseMessagingService is removed.
I can workaround that issue by disabling trimming for Plugin.Firebase.CloudMessaging.
But maybe this could be fixed in the plugin's code, by ensuring the service is always preserved?

@TobiasBuchholz
Copy link
Owner

Okay yes, I will add the Preserve attribute to the MyFirebaseMessagingService and release it with the next Plugin.Firebase.CloudMessaging version 👍

@TobiasBuchholz TobiasBuchholz added the enhancement New feature or request label Sep 12, 2023
@tranb3r
Copy link
Contributor Author

tranb3r commented Sep 12, 2023

Ok but I'm not 100% sure that the Preserve attribute is still the way to preserve code in dotnet maui.
Can you confirm that?

@TobiasBuchholz
Copy link
Owner

No, I can't confirm that, I was just assuming it would work. What would be the alternative?

@tranb3r
Copy link
Contributor Author

tranb3r commented Sep 12, 2023

I've asked for help on discord, here is what I've understood.

The Preserve attribute is now obsolete, and the linker is ignoring it.
You should instead use the DynamicDependency as explained here.
The best place to add this attribute would be the entry point of the library.

So I think the proper way to fix the missing MyFirebaseMessagingService would be to add an Initialize method to CrossFirebaseCloudMessaging and decorate it with the DynamicDependency attribute.

@tranb3r tranb3r mentioned this issue Feb 5, 2024
Closed
@Quaybe
Copy link

Quaybe commented Feb 27, 2024

Disabling trimming completely until this is resolved. The workarounds no longer work for me.

@tranb3r tranb3r mentioned this issue May 13, 2024
Closed
jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this issue Jul 11, 2024
@jonathanpeppers
[Microsoft.Android.Sdk.ILLink] preserve types with `IJniNameProviderA…
1297bf0 
…ttribute`

Fixes: dotnet#8940
Context: TobiasBuchholz/Plugin.Firebase#144

Using the NuGet package:

    <PackageReference Include="Plugin.Firebase.CloudMessaging" Version="3.0.0" />

Includes a service:

    namespace Plugin.Firebase.CloudMessaging.Platforms.Android;

    [Service(Exported = true)]
    [IntentFilter(new[] { "com.google.firebase.MESSAGING_EVENT" })]
    public class MyFirebaseMessagingService : FirebaseMessagingService

Unfortunately, using `TrimMode=full` completely trims away the above
service, which is required for push notifications to work.

I could reproduce this problem in a test using the above NuGet package.

To fix this, we can modify `MarkJavaObjects` to preserve types with
attributes that implement `Java.Interop.IJniNameProviderAttribute`,
and the new test now passes.
@jonathanpeppers jonathanpeppers mentioned this issue Jul 11, 2024
Merged
@jonathanpeppers
Copy link

Until this one ships:

I found putting this in your app seems to workaround the issue:

[DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(MyFirebaseMessagingService))]

Can anyone confirm if it solve the problem for them?

Is this only happening when using TrimMode=full, or does it also occur with default settings?

@dartasen
Copy link

@jonathanpeppers Seems like it's working just fine with DynamicDependency attribute !
Even if Preserve attribute is flagged as Obsolete, still being usable is a bit confusing since it seems to have stopped working properly

@jonathanpeppers
Copy link

jonathanpeppers commented Jul 13, 2024 •
edited
Loading

No, I don't think we can remove [ObsoleteAttribute] as that would be a breaking change. Random Xamarin.Android class libraries (such as old NuGets) could throw TypeLoadException (or possibly even more inscrutable runtime errors).

It does seem like we could add [Obsolete(error: true)] in .NET 9, so you'd get a build error in new source code if you used [PreserveAttribute]. I'll think on it.

@dartasen dartasen mentioned this issue Jul 14, 2024
Open
jonathanpeppers added a commit to dotnet/android that referenced this issue Jul 15, 2024
@jonathanpeppers
[Microsoft.Android.Sdk.ILLink] preserve types with `IJniNameProviderA…
611c269 
…ttribute` (#9099)

Fixes: #8940
Context: TobiasBuchholz/Plugin.Firebase#144

Using the NuGet package:

    <PackageReference Include="Plugin.Firebase.CloudMessaging" Version="3.0.0" />

Includes a service:

    namespace Plugin.Firebase.CloudMessaging.Platforms.Android;

    [Service(Exported = true)]
    [IntentFilter(new[] { "com.google.firebase.MESSAGING_EVENT" })]
    public class MyFirebaseMessagingService : FirebaseMessagingService

Unfortunately, using `TrimMode=full` completely trims away the above
service, which is required for push notifications to work.

I could reproduce this problem in a test using the above NuGet package.

To fix this, we can modify `MarkJavaObjects` to preserve types with
attributes that implement `Java.Interop.IJniNameProviderAttribute`,
and the new test now passes.

With one exception, `Android.Runtime.RegisterAttribute`, should not be
preserved as that would be any Java type bound for C#.
jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this issue Jul 15, 2024
@jonathanpeppers
[Mono.Android] [Preserve] now has [Obsolete(error:true)]
ee569ca 
Context: TobiasBuchholz/Plugin.Firebase#144 (comment)

Even since .NET 6 (in e604833), we have marked `[Preserve]` as
`[Obsolete]`. However, you could still use the attribute *thinking* it
might do something, while it actually does nothing.

Let's add in .NET 9:

* `error: true` so you get a build error

* An actual docs link (the best one I could find)
@jonathanpeppers jonathanpeppers mentioned this issue Jul 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@jonathanpeppers @TobiasBuchholz @dartasen @tranb3r @Quaybe