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

Bump .NET version to 7.0 #807

Merged
merged 30 commits into from
Jan 29, 2024
Merged

Bump .NET version to 7.0 #807

merged 30 commits into from
Jan 29, 2024

Conversation

stephen-hawley
Copy link
Contributor

Yes, this is a large PR. I tried to make each commit its own story, so in reviewing this you should probably go through each commit rather than trying to look at the whole.

This PR moves Binding Tools for Swift from Mono to .NET 7.0
The changes fall into a few categories:

  • updating various constants to reflect the current build environment
  • updating makefiles to use dotnet to build things
  • updating code to use dotnet to compile C# code
  • updating library code to use [UnmanagedCallersOnly]
  • updating generation code to use [UnmanagedCallersOnly]
  • updating generation code to use delegate *unmanaged<...> syntax
  • updating dynamo to make changes easier

In its current state 1416 tests pass and 175 fail. Those that fail fall into the following categories:

  • crashing when running as a macOS app (cause TBD)
  • tests that require tables for generic types: [UnmanagedCallersOnly] can't appear in generic types
  • tests that was NativeHandle for ClassHandle
  • tests that are looking for the wrong SDK
  • tests that are not finding XamGlue (yet)

In addition iPhone/iPad tests don't happen (yet). At this point, I want to get this PR in so I focus on getting things running so I don't hold up @kotlarmilos

<Folder Include="SwiftMarshal\" />
</ItemGroup>
<ItemGroup>
<Reference Include="Xamarin.Mac" />
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look right, you shouldn't be referencing Xamarin.Mac.dll anymore (you should automatically get a reference to Microsoft.macOS.dll because TargetFramework=net7.0-macos).

Copy link
Contributor Author

@stephen-hawley stephen-hawley Jan 22, 2024

Choose a reason for hiding this comment

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

This got managed in a later commit.

Copy link

@mcumming mcumming left a comment

Choose a reason for hiding this comment

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

Only reviewed the csroj files so far, but I'll come back for the rest later. Also check to make sure your Before* and After* targets are actually running. IIRC, those targets do not run if defined in an SDK style project, you have to configure them differently. See https://learn.microsoft.com/en-us/visualstudio/msbuild/how-to-extend-the-visual-studio-build-process?view=vs-2022#example-aftertargets-and-beforetargets

<RuntimeIdentifiers>ios-arm64</RuntimeIdentifiers>
</PropertyGroup>
<ItemGroup>
<Reference Include="SwiftRuntimeLibrary.iOS">

Choose a reason for hiding this comment

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

Why use a binary reference instead of a project reference?

Copy link
Contributor Author

@stephen-hawley stephen-hawley Jan 23, 2024

Choose a reason for hiding this comment

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

Likely because this was set up by vsmac years ago and was hit by the migration tool. I wouldn't worry too much about PigLatin - it's likely going to go away/get rebuilt from scratch as a sample.

<ItemGroup>
<Compile Remove="Properties\AssemblyInfo.cs" />
</ItemGroup>
<ProjectExtensions>

Choose a reason for hiding this comment

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

This should be converted to a .editorconfig and removed from here.

<Platforms>x86;AnyCPU</Platforms>
</PropertyGroup>
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|x86' ">
<OutputPath>bin\Debug</OutputPath>

Choose a reason for hiding this comment

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

This is a default and can be removed

<ExternalConsole>true</ExternalConsole>
</PropertyGroup>
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|x86' ">
<OutputPath>bin\Release</OutputPath>

Choose a reason for hiding this comment

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

This is a default and can be removed

<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|x86' ">
<OutputPath>bin\Debug</OutputPath>
<DefineConstants>DEBUG;</DefineConstants>
<ExternalConsole>true</ExternalConsole>

Choose a reason for hiding this comment

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

This is duplicated for both configurations, move it to a common PropertyGroup

</ItemGroup>
<Import Project="$(MSBuildBinPath)\Microsoft.CSharp.targets" />
<ItemGroup>
<Reference Include="System" />
</ItemGroup>
<ItemGroup>
<Folder Include="SwiftMarshal\" />

Choose a reason for hiding this comment

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

This can probably be removed

</Properties>
</MonoDevelop>
</ProjectExtensions>
<Project Sdk="Microsoft.NET.Sdk">

Choose a reason for hiding this comment

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

You may want to consider migrating this to match the MAUI project templates over the migrated Xamarin.Forms template:
`

<PropertyGroup>
	<TargetFrameworks>net9.0-android;net9.0-ios;net9.0-maccatalyst</TargetFrameworks>
	<TargetFrameworks Condition="$([MSBuild]::IsOSPlatform('windows'))">$(TargetFrameworks);net9.0-windows10.0.19041.0</TargetFrameworks>
	<!-- Uncomment to also build the tizen app. You will need to install tizen by following this: https://github.com/Samsung/Tizen.NET -->
	<!-- <TargetFrameworks>$(TargetFrameworks);net6.0-tizen</TargetFrameworks> -->
	<OutputType>Exe</OutputType>
	<RootNamespace>MauiTest</RootNamespace>
	<UseMaui>true</UseMaui>
	<SingleProject>true</SingleProject>
	<ImplicitUsings>enable</ImplicitUsings>

	<!-- Display name -->
	<ApplicationTitle>MauiTest</ApplicationTitle>

	<!-- App Identifier -->
	<ApplicationId>com.companyname.mauitest</ApplicationId>
	<ApplicationIdGuid>c8937955-7050-4e07-9013-1bda386aa558</ApplicationIdGuid>

	<!-- Versions -->
	<ApplicationDisplayVersion>1.0</ApplicationDisplayVersion>
	<ApplicationVersion>1</ApplicationVersion>

	<SupportedOSPlatformVersion Condition="$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)')) == 'ios'">14.2</SupportedOSPlatformVersion>
	<SupportedOSPlatformVersion Condition="$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)')) == 'maccatalyst'">14.0</SupportedOSPlatformVersion>
	<SupportedOSPlatformVersion Condition="$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)')) == 'android'">21.0</SupportedOSPlatformVersion>
	<SupportedOSPlatformVersion Condition="$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)')) == 'windows'">10.0.17763.0</SupportedOSPlatformVersion>
	<TargetPlatformMinVersion Condition="$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)')) == 'windows'">10.0.17763.0</TargetPlatformMinVersion>
	<SupportedOSPlatformVersion Condition="$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)')) == 'tizen'">6.5</SupportedOSPlatformVersion>
</PropertyGroup>

<PropertyGroup>
	<MtouchLink>SdkOnly</MtouchLink>
</PropertyGroup>

<ItemGroup>
	<PackageReference Include="Microsoft.Maui.Controls" Version="9.0.0-ci.net9.9888" />
</ItemGroup>

<ItemGroup>
	<!-- App Icon -->
	<MauiIcon Include="Resources\AppIcon\appicon.svg" ForegroundFile="Resources\AppIcon\appiconfg.svg" Color="#512BD4" />

	<!-- Splash Screen -->
	<MauiSplashScreen Include="Resources\Splash\splash.svg" Color="#512BD4" BaseSize="128,128" />

	<!-- Images -->
	<MauiImage Include="Resources\Images\*" />
	<MauiImage Update="Resources\Images\dotnet_bot.svg" BaseSize="168,208" />

	<!-- Custom Fonts -->
	<MauiFont Include="Resources\Fonts\*" />

	<!-- Raw Assets (also remove the "Resources\Raw" prefix) -->
	<MauiAsset Include="Resources\Raw\**" LogicalName="%(RecursiveDir)%(Filename)%(Extension)" />
</ItemGroup>
`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I expect the samples to be redone entirely.

<PackageReference Include="NUnit3TestAdapter" Version="4.5.0" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.8.0" />
</ItemGroup>
<ItemGroup>

Choose a reason for hiding this comment

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

Not sure what this next ItemGroup accomplishes. Why are all these files being removed from the project? If it is correct, please convert to globbing patterns to reduce the noise.

<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net7.0</TargetFramework>
<DefaultItemExcludes>$(DefaultItemExcludes);*.sh;*.md;*.props</DefaultItemExcludes>

Choose a reason for hiding this comment

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

💯

</ItemGroup>
<ItemGroup>
<Compile Remove="TypeMetadataSet.cs" />

Choose a reason for hiding this comment

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

Why is this file being removed? Add a comment, or if it is no longer used entirely, delete the file.

@stephen-hawley stephen-hawley merged commit 3b06585 into main Jan 29, 2024
1 of 3 checks passed
@mcumming mcumming deleted the cs-version branch January 29, 2024 15:49
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