[Xamarin.Android.Build.Tasks] introduce XA1038
#8569
Conversation
|
My opinion is this should be an error rather than a warning + autofix. 🤷♂️ |
|
@jpobst: @jonathanpeppers thinking was that trying to target Maybe these aren't analagous situations, but we thought they rhymed? |
|
It occurs to me that Android and iOS should probably be consistent here, unless there's a good reason not to be, so I'm asking @dalexsoto: https://discord.com/channels/732297728826277939/732297808148824115/1184613129544531988 |
|
One thing we need to consider is how this will affect NuGet package resolving logic, as If I set my application to Am I locked out of the .NET 8 Android NuGet ecosystem? Or will that be fixed up as well? I guess a worse scenario would be Somewhat related: NuGet/NuGetGallery#9741 (comment) |
|
@jpobst: in the "warning" case as outlined in this PR, |
|
In the consistency question, @rolfbjarne has replied, and agrees with @jpobst that this scenario should be an error, not a warning. There was another good comment by "gkarabin" as well, seconding the "error" argument:
|
|
@jonathanpeppers: this solves "too low" but doesn't address "too high". If I take a On a "similar yet different 'too high'" take, what about ?! Okay, and which is at least reasonable. I suspect there's no helping the error message produced for |
| </PropertyGroup> | ||
| <PropertyGroup> | ||
| <_AndroidTargetingPackId Condition="$(TargetPlatformVersion.EndsWith('.0'))">$(TargetPlatformVersion.Substring(0, $(TargetPlatformVersion.LastIndexOf('.0'))))</_AndroidTargetingPackId> | ||
| <_AndroidTargetingPackId Condition="'$(_AndroidTargetingPackId)' == ''">$(TargetPlatformVersion)</_AndroidTargetingPackId> | ||
| <_AndroidRuntimePackId Condition=" '$(_AndroidTargetingPackId)' < '@ANDROID_LATEST_STABLE_API_LEVEL@' ">@ANDROID_LATEST_STABLE_API_LEVEL@</_AndroidRuntimePackId> | ||
| <!-- NOTE: adjust if a TargetFramework supports multiple API levels --> | ||
| <_AndroidWarnOnTargetPlatformVersion Condition=" '$(_AndroidTargetingPackId)' < '$(_AndroidLatestStableApiLevel)' ">$(_AndroidTargetingPackId)</_AndroidWarnOnTargetPlatformVersion> |
There was a problem hiding this comment.
This should also "do something" when '$(_AndroidTargetingPackId)' > '$(_AndroidLatestStableApiLevel)'.
This should also take $(EnablePreviewFeatures) into consideration, so that when net8.0-android35.0 previews are available, $(TargetFramework)=net8.0-android35 continues to emit an XA1038 error, while TargetFramework=net8.0-android35;EnablePreviewFeatures=true succeeds.
There was a problem hiding this comment.
35.0 gets a different error:
(_CheckForInvalidTargetPlatformVersion target) ->
dotnet/sdk/9.0.100-alpha.1.23628.5/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.TargetFrameworkInference.targets(243,5):
error NETSDK1140: 35.0 is not a valid TargetPlatformVersion for Android.
Valid versions include:
21.0
22.0
23.0
24.0
25.0
26.0
27.0
28.0
29.0
30.0
31.0
32.0
33.0
34.0
|
WIP commit message: Fixes: https://github.com/xamarin/xamarin-android/issues/8331
Building a `net8.0-android33` project currently fails with:
error NETSDK1181: Error getting pack version: Pack 'Microsoft.Android.Ref.33' was not present in workload manifests.
C:\Program Files\dotnet\sdk\8.0.100-preview.7.23376.3\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.FrameworkReferenceResolution.targets
To solve this, you would either use `net8.0-android34`, changing
33 to 34, or just use `net8.0-android` by removing the number, which
uses the default value.
Improve this situation by emitting an XA1038 error message instead of
the NETSDK1181 error message:
error XA1038: $(TargetPlatformVersion) 33 is not a valid target for 'net8.0-android' projects. Please update your $(TargetPlatformVersion) to a supported version (e.g. 34). |
| <AndroidError Code="XA1038" | ||
| ResourceName="XA1038" | ||
| Condition=" '$(_AndroidWarnOnTargetPlatformVersion)' != '' " | ||
| FormatArguments="$(_AndroidWarnOnTargetPlatformVersion);$(TargetFramework);$(_AndroidLatestStableApiLevel)" |
There was a problem hiding this comment.
I don't quite understand dotnet build handling of $(TargetFramework):
Assume a project with $(TargetFramework)=net8.0-android33.0.
By the time we encounter this line, is $(TargetFramework) changed to net8.0-android, moving the 33.0 into $(TargetFrameworkVersion)? Or does it remain net8.0-android33.0?
I'm curious because of the error message text: if it isn't changed to remove the 33.0, we'd have an error message such as:
$(TargetPlatformVersion) 33.0 is not a valid target for 'net8.0-android33.0 projects. Please update your $(TargetPlatformVersion) to a supported version (e.g. 34).
There was a problem hiding this comment.
This is ugly, but I think the best option is to use net$(TargetFrameworkVersion.TrimStart('vV')), so it will be:
$(TargetPlatformVersion) 33.0 is not a valid target for 'net8.0' projects. Please update your $(TargetPlatformVersion) to a supported version (e.g. 34).
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs
Outdated
Show resolved
Hide resolved
jonathanpeppers
force-pushed
the
XA1038
branch
2 times, most recently
from
92c21cd
to
14bc5a8
Compare
Fixes: dotnet#8331 Building a `net8.0-android33` project, currently fails with: error NETSDK1181: Error getting pack version: Pack 'Microsoft.Android.Ref.33' was not present in workload manifests. C:\Program Files\dotnet\sdk\8.0.100-preview.7.23376.3\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.FrameworkReferenceResolution.targets To solve this, you would either change 33 to 34, or just remove the number to rely on the default value. To make this easier: * Automatically switch to 34 if the user specifies 33 or less. * Emit the new `XA1038` error message. This allows these projects to build with a reasonable error message. The only concern down the road: if we ever support `net8.0-android35` alongside `net8.0-android34`, then we'd need to slightly adjust the logic here.
jonathanpeppers
force-pushed
the
XA1038
branch
from
14bc5a8
to
fa05ca7
Compare
|
I think there's a simpler way: change the FWIW this is .NET's error:
|
We've used to ignore the target platform version (the "17.0" part in "net8.0-ios17.0") since our initial .NET relaese - customers could specify any valid OS version between the minimum and maximum versions, and we'd completely ignore the value [1]. The purpose of the target platform version is to specify which bindings to choose: "net8.0-ios17.0" would mean that the developer wants packages that have bindings for iOS 17.0 (and earlier iOS versions, but not later iOS versions). So saying "net8.0-ios11.0" would technically mean that the developer would want our bindings for iOS 11.0 (and earlier iOS versions, but not later iOS versions). The problem is that we don't ship any such thing... we shipped iOS 17.0 bindings in .NET 8, and that's it, you can't choose to build with something that does *not* have bindings for iOS 17.0. This will change with multi-targeting: we'll support *some* matrix of bindings. For instance, we might want to support the OS version we shipped initial support in any given .NET release + the latest OS version. For example, we might want to support both of these: * net8.0-ios17.0 * net8.0-ios17.2 This means that the target platform version (17.0/17.2) can't keep staying ignored. The proper way to do this is to change the `SdkSupportedTargetPlatformVersion` item group to only include target platform version we actually support, but the downside of this approach is that it will make existing projects fail to compile, if they happened to include an invalid target platform version. So I've added a compatibility mode for .NET 8, where we show a warning (and tell the developer what to do) instead of an error, and then in .NET 9 we'll make the warning an error instead. This is accomplished by still lying in the `SdkSupportedTargetPlatformVersion` item group in .NET 8, and detect the condition in our own logic and report the error. For .NET 9 we can remove this logic, makes ure `SdkSupportedTargetPlatformVersion` is correct, and then .NET itself will show the proper error. Side note: Android is also making an invalid target platform version an error in .NET 9: dotnet/android#8569. [1]: We'd ignore the value for executable projects. It did have an effect for library projects that were packed into NuGets: the target platform version would be stored in the NuGet.
We've used to ignore the target platform version (the "17.0" part in "net8.0-ios17.0") since our initial .NET relaese - customers could specify any valid OS version between the minimum and maximum versions, and we'd completely ignore the value [1]. The purpose of the target platform version is to specify which bindings to choose: "net8.0-ios17.0" would mean that the developer wants packages that have bindings for iOS 17.0 (and earlier iOS versions, but not later iOS versions). So saying "net8.0-ios11.0" would technically mean that the developer would want our bindings for iOS 11.0 (and earlier iOS versions, but not later iOS versions). The problem is that we don't ship any such thing... we shipped iOS 17.0 bindings in .NET 8, and that's it, you can't choose to build with something that does *not* have bindings for iOS 17.0. This will change with multi-targeting: we'll support *some* matrix of bindings. For instance, we might want to support the OS version we shipped initial support in any given .NET release + the latest OS version. For example, we might want to support both of these: * net8.0-ios17.0 * net8.0-ios17.2 This means that the target platform version (17.0/17.2) can't keep staying ignored. The proper way to do this is to change the `SdkSupportedTargetPlatformVersion` item group to only include target platform version we actually support, but the downside of this approach is that it will make existing projects fail to compile, if they happened to include an invalid target platform version. So I've added a compatibility mode for .NET 8, where we show a warning (and tell the developer what to do) instead of an error, and then in .NET 9 we'll make the warning an error instead. This is accomplished by still lying in the `SdkSupportedTargetPlatformVersion` item group in .NET 8, and detect the condition in our own logic and report the error. For .NET 9 we can remove this logic, makes ure `SdkSupportedTargetPlatformVersion` is correct, and then .NET itself will show the proper error. Side note: Android is also making an invalid target platform version an error in .NET 9: dotnet/android#8569. [1]: We'd ignore the value for executable projects. It did have an effect for library projects that were packed into NuGets: the target platform version would be stored in the NuGet.
We've used to ignore the target platform version (the "17.0" part in "net8.0-ios17.0") since our initial .NET relaese - customers could specify any valid OS version between the minimum and maximum versions, and we'd completely ignore the value [1]. The purpose of the target platform version is to specify which bindings to choose: "net8.0-ios17.0" would mean that the developer wants packages that have bindings for iOS 17.0 (and earlier iOS versions, but not later iOS versions). So saying "net8.0-ios11.0" would technically mean that the developer would want our bindings for iOS 11.0 (and earlier iOS versions, but not later iOS versions). The problem is that we don't ship any such thing... we shipped iOS 17.0 bindings in .NET 8, and that's it, you can't choose to build with something that does *not* have bindings for iOS 17.0. This will change with multi-targeting: we'll support *some* matrix of bindings. For instance, we might want to support the OS version we shipped initial support in any given .NET release + the latest OS version. For example, we might want to support both of these: * net8.0-ios17.0 * net8.0-ios17.2 This means that the target platform version (17.0/17.2) can't keep staying ignored. The proper way to do this is to change the `SdkSupportedTargetPlatformVersion` item group to only include target platform version we actually support, but the downside of this approach is that it will make existing projects fail to compile, if they happened to include an invalid target platform version. So I've added a compatibility mode for .NET 8, where we show a warning (and tell the developer what to do) instead of an error, and then in .NET 9 we'll make the warning an error instead. This is accomplished by still lying in the `SdkSupportedTargetPlatformVersion` item group in .NET 8, and detect the condition in our own logic and report the error. For .NET 9 we can remove this logic, makes ure `SdkSupportedTargetPlatformVersion` is correct, and then .NET itself will show the proper error. Side note: Android is also making an invalid target platform version an error in .NET 9: dotnet/android#8569. [1]: We'd ignore the value for executable projects. It did have an effect for library projects that were packed into NuGets: the target platform version would be stored in the NuGet.
I don't think we can remove the lower ones like 21-33, as they provide the I think 21-33 also supports the analyzer that would warn about an API 24+-only API, which would require a |
This implementation doesn't seem very future-proof... according to the design it seems the Instead, I think we'll have to maintain a separate list of OS versions, that we both support, and have supported at some point. To put it to the extreme, we'd have to define all variables starting with iOS 2... (which we obviously aren't going to do, but we'll have to pick some sane lower value - probably the min OS version in .NET 6 - and then never change it).
I think this is keyed off the |
|
From the meeting, we might be able to do something like: <ItemGroup>
<!-- ancient ones, we just put some really low version -->
<SdkSupportedTargetPlatformVersion Include="21" MaximumNETVersion="1.0" />
<!-- ... -->
<SdkSupportedTargetPlatformVersion Include="33" MaximumNETVersion="7.0" />
<SdkSupportedTargetPlatformVersion Include="34" MinimumNETVersion="8.0" />
</ItemGroup>We might need a change in dotnet/sdk to enable this, as only "Minimum" is supported: |
I think this would have to be changed to take into account both a min and a max: |
|
I filed dotnet/sdk#38016 (and a PR with a suggested fix) about this. Note that I didn't go for the .NET version range validation, because it doesn't really work for us. We might want to have strange combination of valid values SdkSupportedTargetPlatformVersion. For a given iOS release, these might be the versions we'd want to support (all in .NET 8):
and the .NET version range check becomes weird in that case (we'd have to come up with something non-intuitive to exclude 17.1). |
We've used to ignore the target platform version (the "17.0" part in "net8.0-ios17.0") since our initial .NET relaese - customers could specify any valid OS version between the minimum and maximum versions, and we'd completely ignore the value [1]. The purpose of the target platform version is to specify which bindings to choose: "net8.0-ios17.0" would mean that the developer wants packages that have bindings for iOS 17.0 (and earlier iOS versions, but not later iOS versions). So saying "net8.0-ios11.0" would technically mean that the developer would want our bindings for iOS 11.0 (and earlier iOS versions, but not later iOS versions). The problem is that we don't ship any such thing... we shipped iOS 17.0 bindings in .NET 8, and that's it, you can't choose to build with something that does *not* have bindings for iOS 17.0. This will change with multi-targeting: we'll support *some* matrix of bindings. For instance, we might want to support the OS version we shipped initial support in any given .NET release + the latest OS version. For example, we might want to support both of these: * net8.0-ios17.0 * net8.0-ios17.2 This means that the target platform version (17.0/17.2) can't keep staying ignored. There was an somewhat related issue with the `SdkSupportedTargetPlatformVersion`, where we're now able to distinguish between old versions we no longer support and new versions that limits the valid values for TargetPlatformVersion (see 74d83ca). We've already taken advantage of this to properly annotate every version, even in .NET 8 (in a future service update), because the dotnet/sdk change required to understand the new annotations (and ignore old versions in the `SdkSupportedTargetPlatformVersion` item group) won't be shipped until .NET 9, so this won't be a breaking change in .NET 8. However, we'd still like to give customers a heads up that their project files need to change, so this PR adds a warning (that tells developers what to do), and then in .NET 9 we'll make the warning an error instead. Side note: Android is also making an invalid target platform version an error in .NET 9: dotnet/android#8569. [1]: We'd ignore the value for executable projects. It did have an effect for library projects that were packed into NuGets: the target platform version would be stored in the NuGet.
…19901) We've used to ignore the target platform version (the "17.0" part in "net8.0-ios17.0") since our initial .NET relaese - customers could specify any valid OS version between the minimum and maximum versions, and we'd completely ignore the value [1]. The purpose of the target platform version is to specify which bindings to choose: "net8.0-ios17.0" would mean that the developer wants packages that have bindings for iOS 17.0 (and earlier iOS versions, but not later iOS versions). So saying "net8.0-ios11.0" would technically mean that the developer would want our bindings for iOS 11.0 (and earlier iOS versions, but not later iOS versions). The problem is that we don't ship any such thing... we shipped iOS 17.0 bindings in .NET 8, and that's it, you can't choose to build with something that does *not* have bindings for iOS 17.0. This will change with multi-targeting: we'll support *some* matrix of bindings. For instance, we might want to support the OS version we shipped initial support in any given .NET release + the latest OS version. For example, we might want to support both of these: * net8.0-ios17.0 * net8.0-ios17.2 This means that the target platform version (17.0/17.2) can't keep staying ignored. There was an somewhat related issue with the `SdkSupportedTargetPlatformVersion`, where we're now able to distinguish between old versions we no longer support and new versions that limits the valid values for TargetPlatformVersion (see 74d83ca). We've already taken advantage of this to properly annotate every version, even in .NET 8 (in a future service update), because the dotnet/sdk change required to understand the new annotations (and ignore old versions in the `SdkSupportedTargetPlatformVersion` item group) won't be shipped until .NET 9, so this won't be a breaking change in .NET 8. However, we'd still like to give customers a heads up that their project files need to change, so this PR adds a warning (that tells developers what to do), and then in .NET 9 we'll make the warning an error instead. Side note: Android is also making an invalid target platform version an error in .NET 9: dotnet/android#8569. [1]: We'd ignore the value for executable projects. It did have an effect for library projects that were packed into NuGets: the target platform version would be stored in the NuGet.
…API levels Fixes: dotnet#8331 Context: dotnet#8569 Context: dotnet/sdk@25b360d Building for `net9.0-android33` would give a poor error message: error NETSDK1181: Error getting pack version: Pack 'Microsoft.Android.Ref.33' was not present in workload manifests. C:\Program Files\dotnet\sdk\8.0.100-preview.7.23376.3\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.FrameworkReferenceResolution.targets To solve this, you would either change 33 to 34, or just remove the number to rely on the default value. To make this easier: * Automatically switch to 34 if the user specifies 33 or less. * Opt into `%(DefineConstantsOnly)=true` for API levels 21-33 (and not the latest), which allows the .NET SDK to emit the *better* .NET SDK error message instead. So now users will get: (_CheckForInvalidTargetPlatformVersion target) -> dotnet/sdk/9.0.100-alpha.1.23628.5/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.TargetFrameworkInference.targets(243,5): error NETSDK1140: 33.0 is not a valid TargetPlatformVersion for Android. Valid versions include: 34.0 I added a test for this scenario.
|
Closing in favor of: #8777 |
…API levels (#8777) Fixes: #8331 Context: #8569 Context: dotnet/sdk@25b360d Building for `net9.0-android33` would give a poor error message: error NETSDK1181: Error getting pack version: Pack 'Microsoft.Android.Ref.33' was not present in workload manifests. C:\Program Files\dotnet\sdk\8.0.100-preview.7.23376.3\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.FrameworkReferenceResolution.targets To solve this, you would either change 33 to 34, or just remove the number to rely on the default value. To make this easier: * Automatically switch to 34 if the user specifies 33 or less. * Opt into `%(DefineConstantsOnly)=true` for API levels 21-33 (and not the latest), which allows the .NET SDK to emit the *better* .NET SDK error message instead. So now users will get: (_CheckForInvalidTargetPlatformVersion target) -> dotnet/sdk/9.0.100-alpha.1.23628.5/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.TargetFrameworkInference.targets(243,5): error NETSDK1140: 33.0 is not a valid TargetPlatformVersion for Android. Valid versions include: 34.0 I added a test for this scenario.
Fixes: #8331
Building a
net8.0-android33project, currently fails with:To solve this, you would either change 33 to 34, or just remove the number to rely on the default value.
To make this easier:
Automatically switch to 34 if the user specifies 33 or less.
Emit the new
XA1038warning message.This allows these projects to build with a reasonable warning message.
The only concern down the road: if we ever support
net8.0-android35alongsidenet8.0-android34, then we'd need to slightly adjust the logic here.