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

Implement .NET 6 features for templates. #3018

Merged
merged 24 commits into from
Nov 2, 2021

Conversation

jamesmontemagno
Copy link
Member

@jamesmontemagno jamesmontemagno commented Oct 18, 2021

Implicit/Global Usings, Scoped Namespaces, & more. Aligns with other projects in .NET 6

This has impact on the .NET MAUI, .NET MAUI Blazor, and the .NET MAUI Library. No impact on the item templates as we can not guarantee that the project is using implicit usings or has global usings specified.

Fixes #1497

@BretJohnson
Copy link
Member

BretJohnson commented Oct 18, 2021

@jamesmontemagno - Do you know what the MS convention is for using spaces versus tabs in templates? I believe it's spaces - that's what I see used in the other templates I tested (including Xamarin.Forms). And it looks like you mostly use spaces as well, but some tabs slipped in there.

@Eilon Eilon added the area-templates Project templates, Item Templates for Blazor and MAUI label Oct 18, 2021
Copy link
Member

@Eilon Eilon left a comment

Choose a reason for hiding this comment

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

Loving it!

@Eilon
Copy link
Member

Eilon commented Oct 18, 2021

@jamesmontemagno please also check out #1497 and #1564 in case there are any relevant notes in there. For example, I'd be fine with the GlobalUsings.cs change in here for now, but we ultimately need to consider having the MAUI SDK import those namespaces instead of the templates.

@jamesmontemagno
Copy link
Member Author

I agree @Eilon that I would like .net maui to have it's own implict usings. The other platforms do which is nice, but .NET MAUI will need to. This would be a nice file->new until it is implemented and then we could remove.

@BretJohnson yeah i have it to tabs so probably need to adjust it. Will do now

@dotMorten
Copy link
Contributor

but we ultimately need to consider having the MAUI SDK import those namespaces instead of the templates.

Please don't do that. This is exactly what was being warned against with this feature: That every single SDK out there starts littering the global usings with their own namespaces. We were more or less promised that wouldn't happen, and the default implicit namespaces would be kept to a bare minimum. It creates a huge amount of class name conflicts, no easy/obvious way to remove individual namespaces, and much harder to share code. At least with the template approach I can go and delete some entries.

Comment on lines 1 to 7
global using Microsoft.Extensions.DependencyInjection;
global using Microsoft.Maui;
global using Microsoft.Maui.Controls;
global using Microsoft.Maui.Controls.Hosting;
global using Microsoft.Maui.Controls.Xaml;
global using Microsoft.Maui.Essentials;
global using Microsoft.Maui.Hosting;
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this should be defined in the workload instead of in templates:

<ItemGroup Condition="'$(ImplicitUsings)' == 'true' or '$(ImplicitUsings)' == 'enable'">
  <Using Include="Microsoft.Extensions.DependencyInjection" />
  <!-- .etc -->
</ItemGroup>

A key feature of global using is that SDKs should define these from MSBuild targets. Users can turn them off in their .csproj or list new ones.

This is how we did the other projects types:

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I agree! Can we do that? Where would I put that? Can you update my PR maybe?

Copy link
Member

@Eilon Eilon Oct 19, 2021

Choose a reason for hiding this comment

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

I would suggest keeping this PR focused on the changes you have, and then we do the SDK global imports as a separate PR to fix issue #1564.

Copy link
Member

Choose a reason for hiding this comment

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

Sure yeah, we could merge this first and do another PR to delete GlobalUsings.cs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good to me! Yeah, when we do the SDK global imports then we can remove. For Essentials I think that will needs its own as well because it is separate.

Comment on lines 4 to 1
using Application = Microsoft.Maui.Controls.Application;
using Application = Microsoft.Maui.Controls.Application;
Copy link
Member

Choose a reason for hiding this comment

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

We should be able to define this one by default as well:

<Using Include="Microsoft.Maui.Controls.Application" Alias="Application" />

You can do similar in C#: global using Application = Microsoft.Maui.Controls.Application;

But I think all these should be defined from MSBuild.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmmm I think I love it? Not sure though.... I go back and forth on this one.

Copy link
Member

Choose a reason for hiding this comment

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

I would be careful about this one being in the SDK because it uses up a super generic name Application... I'd rather debate that in a separate PR to resolve issue #1564.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have updated the Using her to be "MauiApplication" I think it is better that way IMHO.

Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this using? That is the type name. If the namespace is added, we get that for free?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is why we need some MSBuild-side changes here, I think we need:

<Using Remove="Android.App" />

Copy link
Member

@Eilon Eilon left a comment

Choose a reason for hiding this comment

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

Just some tiny comments.

Comment on lines 4 to 1
using Application = Microsoft.Maui.Controls.Application;
using Application = Microsoft.Maui.Controls.Application;
Copy link
Member

Choose a reason for hiding this comment

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

I would be careful about this one being in the SDK because it uses up a super generic name Application... I'd rather debate that in a separate PR to resolve issue #1564.

@Eilon
Copy link
Member

Eilon commented Oct 19, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@Eilon
Copy link
Member

Eilon commented Oct 20, 2021

@jamesmontemagno the template build test is failing:

  D:\agent\1\templatesTest\maui Space-Dash\Platforms\Windows\App.xaml.cs(26,9): error CA1416: This call site is reachable on: 'Windows' 10.0.17763.0 and later. 'MauiWinUIApplication.OnLaunched(LaunchActivatedEventArgs)' is only supported on: 'Windows' 10.0.18362.0 and later. [D:\agent\1\templatesTest\maui Space-Dash\maui Space-Dash.csproj]
  D:\agent\1\templatesTest\maui Space-Dash\MainPage.xaml.cs(15,3): error CA1416: This call site is reachable on: 'Windows' 10.0.17763.0 and later. 'Label.Text' is only supported on: 'Windows' 10.0.18362.0 and later. [D:\agent\1\templatesTest\maui Space-Dash\maui Space-Dash.csproj]
  D:\agent\1\templatesTest\maui Space-Dash\MauiProgram.cs(12,5): error CA1416: This call site is reachable on: 'Windows' 10.0.17763.0 and later. 'FontCollectionExtensions.AddFont(IFontCollection, string, string?)' is only supported on: 'Windows' 10.0.18362.0 and later. [D:\agent\1\templatesTest\maui Space-Dash\maui Space-Dash.csproj]
  D:\agent\1\templatesTest\maui Space-Dash\App.xaml.cs(11,3): error CA1416: This call site is reachable on: 'Windows' 10.0.17763.0 and later. 'Application.MainPage' is only supported on: 'Windows' 10.0.18362.0 and later. [D:\agent\1\templatesTest\maui Space-Dash\maui Space-Dash.csproj]
  D:\agent\1\templatesTest\maui Space-Dash\Platforms\Windows\App.xaml.cs(28,9): error CA1416: This call site is reachable on: 'Windows' 10.0.17763.0 and later. 'Platform.OnLaunched(LaunchActivatedEventArgs)' is only supported on: 'Windows' 10.0.18362.0 and later. [D:\agent\1\templatesTest\maui Space-Dash\maui Space-Dash.csproj]
  D:\agent\1\templatesTest\maui Space-Dash\MainPage.xaml.cs(17,3): error CA1416: This call site is reachable on: 'Windows' 10.0.17763.0 and later. 'SemanticScreenReader.Announce(string)' is only supported on: 'Windows' 10.0.18362.0 and later. [D:\agent\1\templatesTest\maui Space-Dash\maui Space-Dash.csproj]
  D:\agent\1\templatesTest\maui Space-Dash\MauiProgram.cs(8,3): error CA1416: This call site is reachable on: 'Windows' 10.0.17763.0 and later. 'MauiAppBuilderExtensions.UseMauiApp<App>(MauiAppBuilder)' is only supported on: 'Windows' 10.0.18362.0 and later. [D:\agent\1\templatesTest\maui Space-Dash\maui Space-Dash.csproj]
  D:\agent\1\templatesTest\maui Space-Dash\MauiProgram.cs(15,10): error CA1416: This call site is reachable on: 'Windows' 10.0.17763.0 and later. 'MauiAppBuilder.Build()' is only supported on: 'Windows' 10.0.18362.0 and later. [D:\agent\1\templatesTest\maui Space-Dash\maui Space-Dash.csproj]
  D:\agent\1\templatesTest\maui Space-Dash\MainPage.xaml.cs(17,33): error CA1416: This call site is reachable on: 'Windows' 10.0.17763.0 and later. 'Label.Text' is only supported on: 'Windows' 10.0.18362.0 and later. [D:\agent\1\templatesTest\maui Space-Dash\maui Space-Dash.csproj]
  D:\agent\1\templatesTest\maui Space-Dash\MauiProgram.cs(8,3): error CA1416: This call site is reachable on: 'Windows' 10.0.17763.0 and later. 'FontsMauiAppBuilderExtensions.ConfigureFonts(MauiAppBuilder, Action<IFontCollection>?)' is only supported on: 'Windows' 10.0.18362.0 and later. [D:\agent\1\templatesTest\maui Space-Dash\maui Space-Dash.csproj]
  D:\agent\1\templatesTest\maui Space-Dash\MauiProgram.cs(7,17): error CA1416: This call site is reachable on: 'Windows' 10.0.17763.0 and later. 'MauiApp.CreateBuilder()' is only supported on: 'Windows' 10.0.18362.0 and later. [D:\agent\1\templatesTest\maui Space-Dash\maui Space-Dash.csproj]

    0 Warning(s)
    12 Error(s)

There's also some other weirdo error on another leg of the build but I'm not sure what's causing it.

@jamesmontemagno
Copy link
Member Author

This is all odd to me. Are we running these against older windows builds that don't support c# 10? @PureWeen

@jamesmontemagno
Copy link
Member Author

This error comes up with the new templates if the TFM is set to net6.0-windows10.0.19041 and then the supportOS is:

		<SupportedOSPlatformVersion Condition="$(TargetFramework.Contains('-windows'))">10.0.17763.0</SupportedOSPlatformVersion>

This goes away if we build against 10.0.18362.0

However it is odd because in the templates it is 10.0.18362.0

@jamesmontemagno
Copy link
Member Author

@mattleibow ?

@jamesmontemagno
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 3018 in repo dotnet/maui

@Eilon
Copy link
Member

Eilon commented Oct 20, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jamesmontemagno
Copy link
Member Author

@PureWeen @Redth @mattleibow can you verify this is alright 7718ff3

@Redth Redth added this to the 6.0.101-preview.10 milestone Oct 25, 2021
@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

I think this is a good first step, and we can "MSBuild-ify" it later. We should put this on main, but not the current release going out.

The plan for .NET 6, I think will be:

  1. I'll go update the Android, iOS, etc. workloads to put Platform="Android", etc. on each platforms' list of @(Using).
  2. After these changes flow to MAUI, I can remove the platform-specific @(Using) in .targets.
  3. I'll move any GlobalUsing.cs to be in .NET MAUI's MSBuild targets.

.NET 7 might have additional designs around implicit usings, and we can adopt those down the road.

@jamesmontemagno jamesmontemagno changed the base branch from release/6.0.1xx-preview10 to main October 29, 2021 15:24
Copy link
Member

@Eilon Eilon left a comment

Choose a reason for hiding this comment

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

I think this is good to go now?

@jamesmontemagno
Copy link
Member Author

jamesmontemagno commented Oct 29, 2021

@Eilon I think so.... for p11 I believe. @Redth ? I changed it back to main per @jonathanpeppers

jonathanpeppers added a commit to jonathanpeppers/maui that referenced this pull request Nov 1, 2021
Context: dotnet#3018

This depends on dotnet#3108 merged first, and we will need the other mobile
workloads to implement:

    <Using Include="Android.App" Platform="Android" />
    <Using Include="Android.Widget" Platform="Android" />
    <Using Include="Android.OS.Bundle" Alias="Bundle" Platform="Android" />
jonathanpeppers added a commit to jonathanpeppers/xamarin-macios that referenced this pull request Nov 1, 2021
Context: dotnet/maui#3018 (review)

In order for the .NET MAUI workload to properly implement implicit
global usings:

1. The .NET MAUI workload will add many `@(Using)` entries that
   conflict with each platform's APIs.

2. We need *something* to identify `@(Using)` is for a specific
   platform, so we can use a new `%(Platform)` metadata for this.

3. Late in .NET MAUI's MSBuild targets, we can do:

    <ItemGroup Condition=" '$(UseMaui)' == 'true' and ('$(ImplicitUsings)' == 'true' or '$(ImplicitUsings)' == 'enable') ">
      <Using Remove="@(Using->HasMetadata('Platform'))" />
    </ItemGroup>

In .NET 7, we might have a nicer design around this, but for now this
is the plan for .NET 6.
jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this pull request Nov 1, 2021
Context: dotnet/maui#3018 (review)

In order for the .NET MAUI workload to properly implement implicit
global usings:

1. The .NET MAUI workload will add many `@(Using)` entries that
   conflict with each platform's APIs.

2. We need *something* to identify `@(Using)` is for a specific
   platform, so we can use a new `%(Platform)` metadata for this.

3. Late in .NET MAUI's MSBuild targets, we can do:

    <ItemGroup Condition=" '$(UseMaui)' == 'true' and ('$(ImplicitUsings)' == 'true' or '$(ImplicitUsings)' == 'enable') ">
      <Using Remove="@(Using->HasMetadata('Platform'))" />
    </ItemGroup>

In .NET 7, we might have a nicer design around this, but for now this
is the plan for .NET 6.
@jonathanpeppers
Copy link
Member

jonathanpeppers commented Nov 1, 2021

So far the idea is working: e310dd1

MAUI can do this:

<ItemGroup Condition=" '$(UseMaui)' == 'true' and ('$(ImplicitUsings)' == 'true' or '$(ImplicitUsings)' == 'enable') ">
  <Using Remove="@(Using->HasMetadata('Platform'))" />
</ItemGroup>

PRs for each platform to add %(Platform):

Filed issue for Windows:

@Eilon
Copy link
Member

Eilon commented Nov 1, 2021

Does anyone object to getting this PR in as-is? I think there are great ideas for follow-ups but I think this PR stands for itself and I'd love to get it in so we avoid merge conflicts.

Comment on lines 4 to 1
using Application = Microsoft.Maui.Controls.Application;
using MauiApplication = Microsoft.Maui.Controls.Application;
Copy link
Member

Choose a reason for hiding this comment

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

Why use MauiApplication?

Copy link
Member

Choose a reason for hiding this comment

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

FYI @jamesmontemagno this one change was reverted because it wasn't clear it's necessary and it might create even more confusion with MAUI's 4 or 5 different "Application" types.

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds fine to me... why was it ever needed? Is application a conflict with the new usings at all though?

@mattleibow mattleibow enabled auto-merge (squash) November 1, 2021 18:00
jonathanpeppers added a commit to dotnet/android that referenced this pull request Nov 1, 2021
Context: dotnet/maui#3018 (review)

In order for the .NET MAUI workload to properly implement implicit
global usings:

1. The .NET MAUI workload will add many `@(Using)` entries that
   conflict with each platform's APIs.

2. We need *something* to identify `@(Using)` is for a specific
   platform, so we can use a new `%(Platform)` metadata for this.

3. Late in .NET MAUI's MSBuild targets, we can do:

    <ItemGroup Condition=" '$(UseMaui)' == 'true' and ('$(ImplicitUsings)' == 'true' or '$(ImplicitUsings)' == 'enable') ">
      <Using Remove="@(Using->HasMetadata('Platform'))" />
    </ItemGroup>

In .NET 7, we might have a nicer design around this, but for now this
is the plan for .NET 6.
rolfbjarne pushed a commit to xamarin/xamarin-macios that referenced this pull request Nov 2, 2021
Context: dotnet/maui#3018 (review)

In order for the .NET MAUI workload to properly implement implicit
global usings:

1. The .NET MAUI workload will add many `@(Using)` entries that
   conflict with each platform's APIs.

2. We need *something* to identify `@(Using)` is for a specific
   platform, so we can use a new `%(Platform)` metadata for this.

3. Late in .NET MAUI's MSBuild targets, we can do:

    <ItemGroup Condition=" '$(UseMaui)' == 'true' and ('$(ImplicitUsings)' == 'true' or '$(ImplicitUsings)' == 'enable') ">
      <Using Remove="@(Using->HasMetadata('Platform'))" />
    </ItemGroup>

In .NET 7, we might have a nicer design around this, but for now this
is the plan for .NET 6.
@mattleibow mattleibow merged commit e9eb388 into dotnet:main Nov 2, 2021
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this pull request Nov 4, 2021
Context: dotnet#3018

PR dotnet#3108 was a good start in enabling implicit C# global usings.
However, there were a couple of issues to work through:

1. There is a `GlobalUsings.cs` file. Can we do this in MSBuild, so
   it's just there by default? I added:

    <ItemGroup Condition=" '$(UseMaui)' == 'true' and ('$(ImplicitUsings)' == 'true' or '$(ImplicitUsings)' == 'enable') ">
      <!-- %(Sdk) is only here if something later needs to identify these -->
      <Using Include="Microsoft.Extensions.DependencyInjection" Sdk="Maui" />
      <Using Include="Microsoft.Maui" Sdk="Maui" />
      <Using Include="Microsoft.Maui.Controls" Sdk="Maui" />
      <Using Include="Microsoft.Maui.Controls.Hosting" Sdk="Maui" />
      <Using Include="Microsoft.Maui.Controls.Xaml" Sdk="Maui" />
      <Using Include="Microsoft.Maui.Graphics" Sdk="Maui" />
      <Using Include="Microsoft.Maui.Essentials" Sdk="Maui" />
      <Using Include="Microsoft.Maui.Hosting" Sdk="Maui" />
    </ItemGroup>

2. The platform-specific usings were not cleared. So types like
   `Microsoft.Maui.Controls.Application` could conflict with
   `Android.App.Application`.

To solve this, I added a `%(Platform)` metadata in the Android, iOS,
macOS, MacCatalyst, and tvOS workloads:

    <Using Include="Android.App" Platform="Android" />
    <Using Include="Android.Widget" Platform="Android" />
    <Using Include="Android.OS.Bundle" Alias="Bundle" Platform="Android" />

Then in .NET MAUI's MSBuild targets we can do:

    <ItemGroup Condition=" '$(UseMaui)' == 'true' and '$(MauiEnablePlatformUsings)' != 'true' and ('$(ImplicitUsings)' == 'true' or '$(ImplicitUsings)' == 'enable') ">
      <Using Remove="@(Using->HasMetadata('Platform'))" />
    </ItemGroup>

I created `$(MauiEnablePlatformUsings)`, just in case someone ever
needs to turn this off.

I updated the templates to specify `ImplicitUsings=enable`.

I updated `Controls.Sample.Tests.csproj`, as it was a .NET 6 project.
We probably can't update the other samples yet, because there are
Xamarin projects that would not support C# 10 language features.
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this pull request Nov 11, 2021
Context: dotnet#3018

PR dotnet#3108 was a good start in enabling implicit C# global usings.
However, there were a couple of issues to work through:

1. There is a `GlobalUsings.cs` file. Can we do this in MSBuild, so
   it's just there by default? I added:

    <ItemGroup Condition=" '$(UseMaui)' == 'true' and ('$(ImplicitUsings)' == 'true' or '$(ImplicitUsings)' == 'enable') ">
      <!-- %(Sdk) is only here if something later needs to identify these -->
      <Using Include="Microsoft.Extensions.DependencyInjection" Sdk="Maui" />
      <Using Include="Microsoft.Maui" Sdk="Maui" />
      <Using Include="Microsoft.Maui.Controls" Sdk="Maui" />
      <Using Include="Microsoft.Maui.Controls.Hosting" Sdk="Maui" />
      <Using Include="Microsoft.Maui.Controls.Xaml" Sdk="Maui" />
      <Using Include="Microsoft.Maui.Graphics" Sdk="Maui" />
      <Using Include="Microsoft.Maui.Essentials" Sdk="Maui" />
      <Using Include="Microsoft.Maui.Hosting" Sdk="Maui" />
    </ItemGroup>

2. The platform-specific usings were not cleared. So types like
   `Microsoft.Maui.Controls.Application` could conflict with
   `Android.App.Application`.

To solve this, I added a `%(Platform)` metadata in the Android, iOS,
macOS, MacCatalyst, and tvOS workloads:

    <Using Include="Android.App" Platform="Android" />
    <Using Include="Android.Widget" Platform="Android" />
    <Using Include="Android.OS.Bundle" Alias="Bundle" Platform="Android" />

Then in .NET MAUI's MSBuild targets we can do:

    <ItemGroup Condition=" '$(UseMaui)' == 'true' and '$(MauiEnablePlatformUsings)' != 'true' and ('$(ImplicitUsings)' == 'true' or '$(ImplicitUsings)' == 'enable') ">
      <Using Remove="@(Using->HasMetadata('Platform'))" />
    </ItemGroup>

I created `$(MauiEnablePlatformUsings)`, just in case someone ever
needs to turn this off.

I updated the templates to specify `ImplicitUsings=enable`.

I updated `Controls.Sample.Tests.csproj`, as it was a .NET 6 project.
We probably can't update the other samples yet, because there are
Xamarin projects that would not support C# 10 language features.
@github-actions github-actions bot locked and limited conversation to collaborators Dec 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-templates Project templates, Item Templates for Blazor and MAUI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MAUI project templates use latest C# language idioms
7 participants