Skip to content

[One .NET] use API 31 "ref" pack with 32 "runtime" pack #6647

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 19, 2022
Merged

[One .NET] use API 31 "ref" pack with 32 "runtime" pack #6647

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions build-tools/create-packs/Directory.Build.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,6 @@
<_NuGetSources Include="$(OutputPath.TrimEnd('\'))" />
<_PreviewPacks Condition=" '$(AndroidLatestStableApiLevel)' != '$(AndroidLatestUnstableApiLevel)' " Include="$(XamarinAndroidSourcePath)bin\Build$(Configuration)\nuget-unsigned\Microsoft.Android.Ref.$(AndroidLatestUnstableApiLevel).*.nupkg" />
<_InstallArguments Include="android-aot" />
<!-- NOTE: temporary if $(AndroidDefaultTargetDotnetApiLevel) is not 32 -->
<_InstallArguments Include="android-32" />
<_InstallArguments Include="android-$(AndroidLatestUnstableApiLevel)" Condition=" '@(_PreviewPacks->Count())' != '0' " />
<_InstallArguments Include="--skip-manifest-update" />
<_InstallArguments Include="--verbosity diag" />
Expand Down
10 changes: 6 additions & 4 deletions build-tools/create-packs/Microsoft.Android.Sdk.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,10 +122,12 @@ WARNING: DO NOT MODIFY this file unless you are knowledgeable about MSBuild and
<XamarinAndroidVersion>$(AndroidPackVersionLong)</XamarinAndroidVersion>
</PropertyGroup>
<PropertyGroup Condition=" '%24(TargetPlatformVersion)' == '31.0' ">
<_AndroidRuntimePackId>31</_AndroidRuntimePackId>
<_AndroidRuntimePackVersion>31.0.101-preview.11.117</_AndroidRuntimePackVersion>
<_AndroidTargetingPackId>31</_AndroidTargetingPackId>
<_AndroidTargetingPackVersion>31.0.101-preview.11.117</_AndroidTargetingPackVersion>
</PropertyGroup>
<PropertyGroup>
<_AndroidTargetingPackId Condition=" '%24(_AndroidTargetingPackId)' == '' ">$(AndroidLatestStableApiLevel)</_AndroidTargetingPackId>
<_AndroidTargetingPackVersion Condition=" '%24(_AndroidTargetingPackVersion)' == '' ">**FromWorkload**</_AndroidTargetingPackVersion>
<_AndroidRuntimePackId Condition=" '%24(_AndroidRuntimePackId)' == '' ">$(AndroidLatestStableApiLevel)</_AndroidRuntimePackId>
<_AndroidRuntimePackVersion Condition=" '%24(_AndroidRuntimePackVersion)' == '' ">**FromWorkload**</_AndroidRuntimePackVersion>
Copy link
Member

@pjcollins pjcollins Jan 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Do we still need to use these _AndroidRuntimePackId _AndroidRuntimePackVersion based conditions if we aren't setting them anywhere anymore? Though I guess it doesn't hurt to keep them if we would want an easy way to override these for whatever reason.

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 left $(_AndroidRuntimePackId) and $(_AndroidRuntimePackVersion) as private properties. They might be useful to be able to put them in a project file for debugging something down the road.

</PropertyGroup>
Expand All @@ -135,8 +137,8 @@ WARNING: DO NOT MODIFY this file unless you are knowledgeable about MSBuild and
TargetFramework="$(_AndroidNETAppTargetFramework)"
RuntimeFrameworkName="Microsoft.Android"
LatestRuntimeFrameworkVersion="%24(_AndroidRuntimePackVersion)"
TargetingPackName="Microsoft.Android.Ref.%24(_AndroidRuntimePackId)"
TargetingPackVersion="%24(_AndroidRuntimePackVersion)"
TargetingPackName="Microsoft.Android.Ref.%24(_AndroidTargetingPackId)"
TargetingPackVersion="%24(_AndroidTargetingPackVersion)"
RuntimePackNamePatterns="Microsoft.Android.Runtime.%24(_AndroidRuntimePackId).**RID**"
Copy link
Member

Choose a reason for hiding this comment

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

I'm somewhat concerned about the approach to compile and run against different API levels. This reminds me of a very old issue we ran into when updating the shared runtime to always include the latest Mono.Android.dll - https://github.com/xamarin/androidtools/commit/0b7bcd77f1519d2928d9c8e619610ee2afe44786.

Perhaps that particular point of failure is no longer an issue in API 31+, as I think default interface methods in Java may have helped with this case? I can't think of any other scenarios that might break with this approach, but maybe @jonpryor has some additional thoughts here.

Copy link
Member

Choose a reason for hiding this comment

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

Historical context: xamarin/androidtools@0b7bcd7 fixes Xamarin Bugzilla 22074, in which a System.TypeLoadException was thrown from a Debug-configuration app when the shared runtime was used, and the app was built with a $(TargetFrameworkVersion) "sufficiently lower than" the $(TargetFrameworkVersion) included in the shared runtime, all because of android.database.Cursor, my favorite type ever. (Google kept adding methods to Cursor, even before Java interface default methods were supported, and it was a perfectly acceptable change…to Java, but not C#. See also API Design: Interfaces versus Abstract Classes.)

xamarin/androidtools@0b7bcd7 (2014-Aug) predates FixAbstractMethodsStep in xamarin/monodroid@73579e210e (2015-Nov); see also f96fcf9, 0c4c033, etc. If FixAbstractMethodStep had existed, we could have instead "fixed up" the to-be-fast-deployed assemblies against the latest API level, which would also have addressed the scenario in xamarin/androidtools@0b7bcd7 while continuing to include the latest Mono.Android.dll in the shared runtime package. xamarin/androidtools@0b7bcd7 also predates C#8 default interface members.

Today, with .NET 6, this shouldn't be a concern: we can now rely on C#8 default interface members, thus preserving API and ABI compatibility from lower API levels through higher API levels -- which means we could possibly consider dropping FixAbstractMethodsStep in .NET 6? -- and preserving API and ABI compatibility going forward is now one of our tenants.

RuntimePackRuntimeIdentifiers="@(_AndroidNETAppRuntimePackRids, '%3B')"
Profile="Android"
Expand Down
33 changes: 3 additions & 30 deletions src/Xamarin.Android.Build.Tasks/Microsoft.NET.Sdk.Android/WorkloadManifest.in.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,26 +6,15 @@
"packs": [
"Microsoft.Android.Sdk",
"Microsoft.Android.Ref.31",
"Microsoft.Android.Runtime.31.android-arm",
"Microsoft.Android.Runtime.31.android-arm64",
"Microsoft.Android.Runtime.31.android-x86",
"Microsoft.Android.Runtime.31.android-x64",
"Microsoft.Android.Templates"
],
"platforms": [ "win-x64", "linux-x64", "osx-x64", "osx-arm64" ],
"extends" : [ "microsoft-net-runtime-android" ]
},
"android-32": {
"description": "Support for Android API-32.",
"packs": [
"Microsoft.Android.Ref.32",
"Microsoft.Android.Runtime.32.android-arm",
"Microsoft.Android.Runtime.32.android-arm64",
"Microsoft.Android.Runtime.32.android-x86",
"Microsoft.Android.Runtime.32.android-x64"
"Microsoft.Android.Runtime.32.android-x64",
"Microsoft.Android.Templates"
],
"platforms": [ "win-x64", "linux-x64", "osx-x64", "osx-arm64" ],
"extends" : [ "android" ]
"extends" : [ "microsoft-net-runtime-android" ]
},
"android-aot": {
"description": ".NET SDK Workload for building Android applications with AOT support.",
Expand All @@ -49,22 +38,6 @@
"kind": "framework",
"version": "31.0.101-preview.11.117"
},
"Microsoft.Android.Runtime.31.android-arm": {
"kind": "framework",
"version": "31.0.101-preview.11.117"
},
"Microsoft.Android.Runtime.31.android-arm64": {
"kind": "framework",
"version": "31.0.101-preview.11.117"
},
"Microsoft.Android.Runtime.31.android-x86": {
"kind": "framework",
"version": "31.0.101-preview.11.117"
},
"Microsoft.Android.Runtime.31.android-x64": {
"kind": "framework",
"version": "31.0.101-preview.11.117"
},
"Microsoft.Android.Ref.32": {
"kind": "framework",
"version": "@WORKLOAD_VERSION@"
Expand Down