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

Add iOS/Mac default global usings in MSBuild targets #12084

Closed
Tracked by #12085
mhutch opened this issue Jul 7, 2021 · 6 comments · Fixed by #12173
Closed
Tracked by #12085

Add iOS/Mac default global usings in MSBuild targets #12084

mhutch opened this issue Jul 7, 2021 · 6 comments · Fixed by #12173
Assignees
Labels
dotnet An issue or pull request related to .NET (6) dotnet-pri1 .NET 6: important for stable release enhancement The issue or pull request is an enhancement msbuild Issues affecting our msbuild tasks/targets
Milestone

Comments

@mhutch
Copy link
Member

mhutch commented Jul 7, 2021

Following the pattern in dotnet/sdk#18459, the iOS/Mac SDK targets should contain default global usings that can be disabled by an MSBuild property.


Namespaces to be imported by default:

iOS: CoreGraphics, Foundation, UIKit
tvOS: CoreGraphics, Foundation, UIKit
macOS: AppKit, CoreGraphics, Foundation
Mac Catalyst: CoreGraphics, Foundation, UIKit

@mhutch mhutch added the msbuild Issues affecting our msbuild tasks/targets label Jul 7, 2021
@mhutch mhutch added this to the .NET 6 milestone Jul 7, 2021
@rolfbjarne rolfbjarne added dotnet An issue or pull request related to .NET (6) dotnet-pri1 .NET 6: important for stable release enhancement The issue or pull request is an enhancement labels Jul 12, 2021
@rolfbjarne
Copy link
Member

The templates have these usings (with number of occurrences):

rolf@ ~/work/maccore/main/xamarin-macios/dotnet $ git grep 'using.*;' | sed 's/.*using/using/' | sort | uniq -c | sort -n 
   1 using CoreFoundation;
   1 using System.Security.Cryptography;
   1 using System.Text;
   2 using System.IO;
   2 using System.Xml;
   3 using AppKit;
   5 using System;
  10 using UIKit;
  11 using Foundation;

If I understand correctly, the System* ones aren't ours to import. This leaves the namespaces we define: Foundation, UIKit/AppKit and CoreFoundation.

CoreFoundation contains more advanced API that most users wouldn't use, so my suggestion would be to only import by default Foundation + UIKit (for iOS, tvOS and Mac Catalyst) and AppKit (for macOS):

<ItemGroup Condition="'$(DisableImplicitNamespaceImports_Apple)' != 'true'">
    <Import Include="Foundation" />
    <Import Include="UIKit" Condition="'$(_PlatformName)' == 'iOS' Or '$(_PlatformName)' == 'tvOS' Or '$(_PlatformName)' == 'MacCatalyst'" />
    <Import Include="AppKit" Condition="'$(_PlatformName)' == 'macOS'" />
</ItemGroup>

One question comes up: should the condition be based on a single property for all our platforms (DisableImplicitNamespaceImports_Apple), or one property for each (DisableImplicitNamespaceImports_iOS, DisableImplicitNamespaceImports_tvOS, etc)?

@rolfbjarne
Copy link
Member

Another question: if the user sets DisableImplicitNamespaceImports=true, should that also disable our implicit usings?

@mhutch
Copy link
Member Author

mhutch commented Jul 13, 2021

Probably do it per platform (DisableImplicitNamespaceImports_iOS, DisableImplicitNamespaceImports_tvOS etc)? I think that would be consistent with what we're doing elsewhere. We could maybe have a case for DisableImplicitNamespaceImports_Apple to control Foundation and per-platform ones to control the UI namespaces but I don't see a strong precedent or use case for that TBH.

The logic in web is to default DisableImplicitNamespaceImports_Web to false if DisableImplicitNamespaceImports is not true and then check that DisableImplicitNamespaceImports_Web is not true. So that means DisableImplicitNamespaceImports_Web=false will override DisableImplicitNamespaceImports=true. Not sure whether that's desirable or whether DisableImplicitNamespaceImports should take precedence but either way we should make sure it's consistent across all app types /cc @DamianEdwards

Are there any other usings that are commonly used in apps but not in the templates? For example CoreGraphics?

Thought the more usings we add, the higher the risk of bringing in types that collide with types brought in by the Maui usings, as the iOS types will be brought in for the iOS build of multitargeted Maui apps. This will also be an issue for Android /cc
@jonathanpeppers @Redth

@DamianEdwards
Copy link

I thought we'd decided on DisableImplicitNamespaceImports being the master switch which had to be true for the SDK-specific properties to apply, so I'll add in @JunTaoLuo here to comment on the logic there.

@JunTaoLuo
Copy link

DisableImplicitNamespaceImports is the master switch. If this is set to true, the target that generates the actual .cs file containing all of these implicit usings will not be run. The SDK specific property controls whether namespaces are added to the Import items. So for the entire end to end to work, both the master switch must be on and the necessary namespaces added to Import items.

If I understand correctly, the System* ones aren't ours to import.

This is not strictly true. For example, in the the Web SDK we add System.Net.Http.Json: https://github.com/dotnet/sdk/blob/main/src/WebSdk/Web/Sdk/Sdk.props#L64. However for some of the proposed ones that are very general such as System.Text, it's worth having a conversation with the maintainers of the base .NET SDK where that should go.

@rolfbjarne
Copy link
Member

DisableImplicitNamespaceImports is the master switch. If this is set to true, the target that generates the actual .cs file containing all of these implicit usings will not be run.

OK, I believe that means that while the web logic is not incorrect, it's also slightly more complex than necessary (there's no need to make our additions to the Import items conditional on DisableImplicitNamespaceImports, we can add them as long as our own DisableImplicitNamespaceImports_* property is not true).

Are there any other usings that are commonly used in apps but not in the templates? For example CoreGraphics?

Yes, CoreGraphics is often used too, so I'll it as well.

@rolfbjarne rolfbjarne assigned rolfbjarne and unassigned dalexsoto Jul 14, 2021
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Jul 22, 2021
.

Also update our templates to remove any using statements for implicitly imported
namespaces.

Fixes xamarin#12084.
rolfbjarne added a commit that referenced this issue Jul 23, 2021
…12173)

* [dotnet] Add support for implicit namespace imports. Fixes #12084.

Also update our templates to remove any using statements for implicitly imported
namespaces.

Fixes #12084.

* [monotouch-test] Fix compilation error due to implicit namespace causing type conflict.

Fixes these errors:

    xamarin-macios/tests/monotouch-test/ImageIO/MutableImageMetadataTest.cs(54,54): error CS0104: 'CGImageProperties' is an ambiguous reference between 'CoreGraphics.CGImageProperties' and 'ImageIO.CGImageProperties'
    xamarin-macios/tests/monotouch-test/ImageIO/MutableImageMetadataTest.cs(54,88): error CS0104: 'CGImageProperties' is an ambiguous reference between 'CoreGraphics.CGImageProperties' and 'ImageIO.CGImageProperties'
    xamarin-macios/tests/monotouch-test/ImageIO/ImageMetadataTest.cs(40,54): error CS0104: 'CGImageProperties' is an ambiguous reference between 'CoreGraphics.CGImageProperties' and 'ImageIO.CGImageProperties'
    xamarin-macios/tests/monotouch-test/ImageIO/ImageMetadataTest.cs(40,88): error CS0104: 'CGImageProperties' is an ambiguous reference between 'CoreGraphics.CGImageProperties' and 'ImageIO.CGImageProperties'
vs-mobiletools-engineering-service2 pushed a commit to vs-mobiletools-engineering-service2/xamarin-macios that referenced this issue Jul 23, 2021
.

Also update our templates to remove any using statements for implicitly imported
namespaces.

Fixes xamarin#12084.
rolfbjarne added a commit that referenced this issue Jul 23, 2021
…e imports. Fixes #12084. (#12196)

* [dotnet] Add support for implicit namespace imports. Fixes #12084.

Also update our templates to remove any using statements for implicitly imported
namespaces.

Fixes #12084.

* [monotouch-test] Fix compilation error due to implicit namespace causing type conflict.

Fixes these errors:

    xamarin-macios/tests/monotouch-test/ImageIO/MutableImageMetadataTest.cs(54,54): error CS0104: 'CGImageProperties' is an ambiguous reference between 'CoreGraphics.CGImageProperties' and 'ImageIO.CGImageProperties'
    xamarin-macios/tests/monotouch-test/ImageIO/MutableImageMetadataTest.cs(54,88): error CS0104: 'CGImageProperties' is an ambiguous reference between 'CoreGraphics.CGImageProperties' and 'ImageIO.CGImageProperties'
    xamarin-macios/tests/monotouch-test/ImageIO/ImageMetadataTest.cs(40,54): error CS0104: 'CGImageProperties' is an ambiguous reference between 'CoreGraphics.CGImageProperties' and 'ImageIO.CGImageProperties'
    xamarin-macios/tests/monotouch-test/ImageIO/ImageMetadataTest.cs(40,88): error CS0104: 'CGImageProperties' is an ambiguous reference between 'CoreGraphics.CGImageProperties' and 'ImageIO.CGImageProperties'

Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
@ghost ghost locked as resolved and limited conversation to collaborators Apr 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dotnet An issue or pull request related to .NET (6) dotnet-pri1 .NET 6: important for stable release enhancement The issue or pull request is an enhancement msbuild Issues affecting our msbuild tasks/targets
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants