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

Make use of a specific iOS + MacCatalyst TFM #2243

Closed
wants to merge 8 commits into from

Conversation

bijington
Copy link
Contributor

@bijington bijington commented Oct 1, 2024

Description of Change

Linked Issues

PR Checklist

  • Has a linked Issue, and the Issue has been approved(bug) or Championed (feature/proposal)
  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard
  • Documentation created or updated: https://github.com/MicrosoftDocs/CommunityToolkit/pulls

Additional information

@brminnick
Copy link
Collaborator

brminnick commented Oct 1, 2024

Hey Shaun! Two quick thoughts:

  1. If we make this change for iOS, let's also do it for MacCatalyst too
  2. Is it possible to create unit test that validates this fixes the NativeAOT/Trimming bugs?
    • We may need to first merge the Device Tests PR
    • If you can figure out how to get CI / CD pipeline working for the Device Tests PR, we can merge it now to help you complete this bug fix
      • i.e. We don't need to write a complete DeviceTest suite in order to first merge the Device Tests PR

(Also - If this is PR still a work-in-progress, could you flip it to be a Draft PR please? I get distracted by shiny new PRs and want to review them, but I wait when the PR is still in Draft.)

@bijington
Copy link
Contributor Author

Hey Shaun! Two quick thoughts:

  1. If we make this change for iOS, let's also do it for MacCatalyst too

  2. Is it possible to create unit test that validates this fixes the NativeAOT/Trimming bugs?

    • We may need to first merge the Device Tests PR

    • If you can figure out how to get CI / CD pipeline working for the Device Tests PR, we can merge it now to help you complete this bug fix

      • i.e. We don't need to write a complete DeviceTest suite in order to first merge the Device Tests PR

(Also - If this is PR still a work-in-progress, could you flip it to be a Draft PR please? I get distracted by shiny new PRs and want to review them, but I wait when the PR is still in Draft.)

This isn't aimed at fixing any NativeAOT or Trimming bugs but aimed at the TypeLoadException due to mismatched TFMs when developers consume our library. Based on this issue/discussion xamarin/xamarin-macios#21335
I mostly just wanted to see if it would build for now which it does not.

I'll be happy to work on any possible testing options although I am not sure how we can right now.

@bijington bijington added the needs discussion Discuss it on the next Monthly standup label Oct 1, 2024
@bijington
Copy link
Contributor Author

I am actually starting to wonder whether this might also fix other issues such as #1374 based on this documentation page https://learn.microsoft.com/en-us/dotnet/standard/frameworks#precedence I read it as apps could be built targeting net8.0 support as this may be closer to what the developer is targeting. It's either going to help or make it worse...

@bijington bijington marked this pull request as ready for review October 1, 2024 20:18
@bijington
Copy link
Contributor Author

bijington commented Oct 2, 2024

@brminnick I'm not convinced we can create a unit test for this. I do wonder if we could add something to the pipeline to make sure that the packaged information meets what we expect. What do you think?

@brminnick
Copy link
Collaborator

Let's chat about it in tomorrow's standup!

I'm fairly confident that as long as we set up our CI / CD pipeline correctly to match our targeted frameworks and run the Device Tests in RELEASE configuration with NativeAOT enabled, <PublishAot>true</PublishAot>, it should replicate any errors our users are experiencing.

@filipnavara
Copy link
Contributor

We can replicate the error in a CI pipelines but it’s not straightforward. The latest breakage required .NET 9 RC SDK installed side-by-side to trigger it. Alternatively you can deliberately install old (fixed) workload set instead of the latest one. The trouble with that is that you either need to use custom manifests or start with 8.0.4xx SDK which introduced the workload set support.

@bijington
Copy link
Contributor Author

I was thinking of just inspecting the nuget package generated and verifying it targets the expected TFMs for Apple platforms

@bijington
Copy link
Contributor Author

I guess a nice test would be to try consuming the nightly build and see if that still exhibits the issue. I've not be able to reproduce the issue @filipnavara is this something that you could easily test/verify?

@filipnavara
Copy link
Contributor

filipnavara commented Oct 2, 2024

I guess a nice test would be to try consuming the nightly build and see if that still exhibits the issue. I've not be able to reproduce the issue @filipnavara is this something that you could easily test/verify?

Nightly build of what? CommunityToolkit.Maui? If you have a link I can test the scenarios easily and report back.

I assume I just need to use this feed:

    <add key="CommunityToolkit-PullRequests" value="https://pkgs.dev.azure.com/dotnet/CommunityToolkit/_packaging/CommunityToolkit-PullRequests/nuget/v3/index.json" />

...and then reference the build number from this PR, right?

@bijington
Copy link
Contributor Author

There might be a neater way but I have just knocked up a little console app that could be published as a dotnet tool to perform this verification. The code is:

using System.IO.Compression;
using CommandLine;

namespace PackageVerifier;

class Program
{
    static void Main(string[] args)
    {
        Parser.Default.ParseArguments<Options>(args)
            .WithParsed<Options>(o =>
            {
                Console.WriteLine($"Verifying package '{o.PackagePath}'");

                var extractDirectory = Path.Join(Environment.CurrentDirectory, "abc");
                Console.WriteLine($@"Extracting package to directory '{extractDirectory}'");
                
                ZipFile.ExtractToDirectory(o.PackagePath, extractDirectory);

                Console.WriteLine($"Checking for TFMs: '{string.Join(',', o.TargetFrameworkMonikers)}'");
                
                var libDirectory = Path.Join(extractDirectory, "lib");
                var directories = new DirectoryInfo(libDirectory).GetDirectories().Select(x => x.Name).ToList();

                foreach (var unexpectedDirectory in directories.Except(o.TargetFrameworkMonikers))
                {
                    Console.WriteLine($"WARNING: TFM '{unexpectedDirectory}' was not expected");
                }
                
                foreach (var unexpectedDirectory in o.TargetFrameworkMonikers.Except(directories))
                {
                    Console.WriteLine($"ERROR: TFM '{unexpectedDirectory}' is missing");
                }
                
                foreach (var unexpectedDirectory in o.TargetFrameworkMonikers.Intersect(directories))
                {
                    Console.WriteLine($"SUCCESS: Expected TFM '{unexpectedDirectory}' exists");
                }
            });
    }
}

Usage is something like:

dotnet packageverifier --tfms "net9.0-ios17.5" --package-path "/Users/shaunlawrence/Documents/work/projects/toolkits/mct/src/Maui/src/CommunityToolkit.Maui/bin/Release/CommunityToolkit.Maui.1.0.0-pre1.nupkg.zip"

And the output currently looks something like:

Verifying package '/Users/shaunlawrence/Documents/work/projects/toolkits/mct/src/Maui/src/CommunityToolkit.Maui/bin/Release/CommunityToolkit.Maui.1.0.0-pre1.nupkg.zip'
Extracting package to directory '/Users/shaunlawrence/Documents/work/projects/investigations/PackageVerifier/PackageVerifier/bin/Debug/net8.0/abc'
Checking for TFMs: 'net9.0-ios17.5'
WARNING: TFM 'net9.0-maccatalyst17.5' was not expected
WARNING: TFM 'net9.0' was not expected
WARNING: TFM 'net9.0-android35.0' was not expected
SUCCESS: Expected TFM 'net9.0-ios17.5' exists

With a bit more effort it could be modified to be more user friendly and allow it to cause a pipeline build to fail. What do you think?

@bijington
Copy link
Contributor Author

I guess a nice test would be to try consuming the nightly build and see if that still exhibits the issue. I've not be able to reproduce the issue @filipnavara is this something that you could easily test/verify?

Nightly build of what? CommunityToolkit.Maui? If you have a link I can test the scenarios easily and report back.

I assume I just need to use this feed:

    <add key="CommunityToolkit-PullRequests" value="https://pkgs.dev.azure.com/dotnet/CommunityToolkit/_packaging/CommunityToolkit-PullRequests/nuget/v3/index.json" />

...and then reference the build number from this PR, right?

Yes that looks like it is the correct feed and approach for the build number as per here https://github.com/CommunityToolkit/Maui/wiki/Preview-Packages#pull-requests-

Let me find the build number

@filipnavara
Copy link
Contributor

Let me find the build number

99.0.0-build-2243.110238+e95e119 .. no need, it's in the Checks.

@filipnavara
Copy link
Contributor

Yes, I can confirm that with .NET 9 RC 1 SDK I now get the correct iOS assets. With 9.1.0 on NuGet.org I get the generic net8.0 assets.

@bijington
Copy link
Contributor Author

Yes, I can confirm that with .NET 9 RC 1 SDK I now get the correct iOS assets. With 9.1.0 on NuGet.org I get the generic net8.0 assets.

That's fantastic news! Thank you so much for testing this and all of the input to get us to this stage!

@filipnavara
Copy link
Contributor

Thank you so much for testing this and all of the input to get us to this stage!

Thanks for addressing it so quickly. :-) Let me know if there's anything else you need to test.

@bijington
Copy link
Contributor Author

I have had some fun playing around with this so I have stuck it in a GitHub repo: https://github.com/bijington/package-verifier

@brminnick brminnick changed the title Make use of a specific ios version in the TFM Make use of a specific iOS + MacCatalyst TFM Oct 3, 2024
@brminnick brminnick added the hacktoberfest-accepted A PR that has been approved during Hacktoberfest label Oct 3, 2024
@brminnick
Copy link
Collaborator

brminnick commented Oct 7, 2024

Hey Shaun! I just tested this NuGet Package on a local environment that only has .NET SDK v8.0.303 installed (which does not include net-ios18.0), and IDE still allowed me to add the NuGet Package and compile my code with no warnings or build errors. I assume that in this test, my MAUI app is using code from the net8.0 version of CommunityToolkit.Maui for the net8.0-ios build because the app crashes at runtime with TypeLoadException.

So, the good news is that this PR solves one very specific problem where the user has net8.0-ios18.0 installed via .NET SDK 8.0.402, but also has .NET SDK 9.0 installed which doesn't yet include net8.0-ios18.0; this PR ensures that the dev's machine will revert to using the installed .NET 8.0 SDK + net8.0-ios18.0 instead of the .NET 9.0 SDK.

But it doesn't solve the problem where CommunityToolkit.Maui requires a specific version of a .NET MAUI Workload installed and provides a build error when the user doesn't have the workload installed.

I think our best move is to add a MSBuild task that checks the user's local environment and produces a build error with a helpful message letting them know which MAUI workload we require. I'll play around with adding something like this today.

@bijington
Copy link
Contributor Author

bijington commented Oct 7, 2024

So, the good news is that this PR solves one very specific problem where the user has net8.0-ios18.0 installed via .NET SDK 8.0.402, but also has .NET SDK 9.0 installed which doesn't yet include net8.0-ios18.0; this PR ensures that the dev's machine will revert to using the installed .NET 8.0 SDK + net8.0-ios18.0 instead of the .NET 9.0 SDK.

Does it? Wasn't this the original issue because we were implicitly targeting net8-ios18.0 in the last release (9.1.0) - surely this means we are just preserving that issue?

I have the following setup:

dotnet --info

.NET SDKs installed:
  8.0.401 [/usr/local/share/dotnet/sdk]
  9.0.100-rc.1.24452.12 [/usr/local/share/dotnet/sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.App 8.0.8 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 9.0.0-rc.1.24452.1 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 8.0.8 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 9.0.0-rc.1.24431.7 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]

When I pull in the latest build https://dev.azure.com/dotnet/CommunityToolkit/_build/results?buildId=110421&view=results from the feed I see the dreaded

System.NotSupportedException: PlatformSetColor is only supported on iOS and Android 23 and later

On my setup if I revert to the build that targets 17.5 https://dev.azure.com/dotnet/CommunityToolkit/_releaseProgress?_a=release-pipeline-progress&releaseId=9003 then I no longer see this exception. I think we should ship a patch fix with 17.5 now to avoid the large number of developers coming to us with this issue and then ship an 18.0 in the next release.

I can confirm that I don't currently have iOS 18.0 support on my Mac but I would like us to be able to safely handle this situation. Perhaps your suggestion of the MS Build task might prevent this?

Given that iOS 18.0 was only made available to the public on 16th September 2024 (less than one month ago) I think it might be wise that we don't force users to update all of their tooling so quickly. If they have updated their tooling then us targeting 17.5 should work just fine for them.

@filipnavara
Copy link
Contributor

this PR ensures that the dev's machine will revert to using the installed .NET 8.0 SDK + net8.0-ios18.0 instead of the .NET 9.0 SDK.

No, it doesn't fix that as is. You should change it back to target ios17.5 to fix it.

@brminnick
Copy link
Collaborator

@filipnavara To workaround your specific problem, I recommend adding a global.json file to your project that prevents it from using .NET 9 until we add support for .NET 9. This will ensure that the .NET tooling only uses the .NET 8 SDK when you build your project.

global.json

{
  "sdk": {
    "version": "8.0.402",
    "rollForward": "latestFeature",
    "allowPrerelease": false
  }
}

@filipnavara
Copy link
Contributor

filipnavara commented Oct 7, 2024

To workaround your specific problem, I recommend adding a global.json file to your project that prevents it from using .NET 9 until we add support for .NET 9.

I am aware of that workaround (alongside few others). It's just that .NET 9 SDK builds are noticeably faster on some machines so our devs prefer to use it.

@filipnavara
Copy link
Contributor

Note that technically there should be no benefit for libraries to target net8.0-ios18.0 over net8.0-ios17.5 if you don't use any of the new APIs. It's a different story for apps where you need to target newer SDK due to the way the Xcode tooling is versioned, app store submissions, and compatibility with newer simulators for dev cycle.

@brminnick
Copy link
Collaborator

Thanks for confirming the workaround works for you, @filipnavara.

We'll close this PR and work on a way that we can leverage MSBuild Task to ensure developer's local environments are using the correct .NET SDK, .NET MAUI Workload and Xcode versions.

@brminnick brminnick closed this Oct 7, 2024
@filipnavara
Copy link
Contributor

filipnavara commented Oct 7, 2024

We'll close this PR and work on a way that we can leverage MSBuild Task to ensure developer's local environments are using the correct .NET SDK, .NET MAUI Workload and Xcode versions.

It would be nice to share more details about your plan.

I don't think libraries should force users to upgrade SDKs unless there's a compelling reason (eg. relying on new API, tooling or fix). Workloads are already tied to Xcode versions and they have the necessary checks. It is also entirely valid to use .NET 9 SDK to compile net8.0-XXX TFMs, and .NET 9 RC 1 SDK was already released with go-live support license.

Also, NuGet largely runs before most of the MSBuild evaluation takes place. You can detect that you are compiling for net8.0-ios TFM and that you incorrectly consumed the net8.0 asset of your library but you cannot really fix it unless you bypass the the whole NuGet resolution and rely on MSBuild script and Reference item group. Essentially you can detect the mismatch and issue an error but to fully fix the problem you still need to downgrade the TFM inside the NuGet to target net8.0-ios17.X.

@bijington
Copy link
Contributor Author

to fully fix the problem you still need to downgrade the TFM inside the NuGet to target net8.0-ios17.X.

As Brandon has pointed out to me offline of this discussion; downgrading the TFM and shipping a version would effectively break anyone targeting iOS 18.0 already. So I agree it best that we don't break those users especially if we have a workaround for this issue.

The PR for the MSBuild task is #2266. It's frustrating that we have to guard this rather than Nuget doing it but that sadly appears to be some pain of relying on workloads

@filipnavara
Copy link
Contributor

filipnavara commented Oct 8, 2024

downgrading the TFM and shipping a version would effectively break anyone targeting iOS 18.0 already.

How? Lower version of library TFMs is still consumable by net8.0-ios18.0. That’s easy to verify if you create new iOS app with the latest workloads (specifies net8.0-ios which is implicitly transformed into net8.0-ios18.0 by the workload) and reference CommunityToolkit.Maui 9.0.3 which shipped older TFM.

(Even if there was such a case, which I still think there is NOT, you can still ship both net8.0-ios17.5 AND net8.0-ios18.0 assets, they are not exclusive.)

@bijington
Copy link
Contributor Author

You are correct, I have tested running our build targeting 17.5 from this PR against the latest SDK and workload and it behaves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted A PR that has been approved during Hacktoberfest needs discussion Discuss it on the next Monthly standup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Could not set up parent class, due to: Invalid generic instantiation
5 participants