Ship the .aar with the NuGet for android #2465
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/azp run |
|
Commenter does not have sufficient privileges for PR 2465 in repo mono/SkiaSharp |
dellis1972
changed the title
|
@mattleibow can we get this in for .NET 8 customers? |
mattleibow
changed the title
mattleibow
approved these changes
Jun 8, 2023
| @@ -106,6 +106,7 @@ Please visit https://go.microsoft.com/fwlink/?linkid=868517 to view the release | |||
| <file src="lib/net6.0-android/SkiaSharp.Views.Android.dll" target="lib/net6.0-android30.0/SkiaSharp.Views.Android.dll" /> | |||
| <file src="lib/net6.0-android/SkiaSharp.Views.Android.pdb" target="lib/net6.0-android30.0/SkiaSharp.Views.Android.pdb" /> | |||
| <file src="lib/net6.0-android/SkiaSharp.Views.Android.xml" target="lib/net6.0-android30.0/SkiaSharp.Views.Android.xml" /> | |||
| <file src="lib/net6.0-android/SkiaSharp.Views.Android.aar" target="lib/net6.0-android30.0/SkiaSharp.Views.Android.aar" /> | |||
There was a problem hiding this comment.
This packs the file in the NuGet package.
| @@ -187,11 +187,13 @@ internal partial class VersionConstants { | |||
| <ItemGroup Condition=" '$(PackagingLocation)' == '' "> | |||
| <_CopyItems Include="$(TargetPath)" Dest="nuget\lib\$(PackagingPlatform)\$(TargetFileName)" /> | |||
| <_CopyItems Include="$(TargetDir)$(TargetName).xml" Dest="nuget\lib\$(PackagingPlatform)\$(TargetName).xml" Condition=" Exists('$(TargetDir)$(TargetName).xml') " /> | |||
| <_CopyItems Include="$(TargetDir)$(TargetName).aar" Dest="nuget\lib\$(PackagingPlatform)\$(TargetName).aar" Condition=" Exists('$(TargetDir)$(TargetName).aar') " /> | |||
There was a problem hiding this comment.
This copies from the bin directory into the artifacts directory.
mattleibow
approved these changes
Jun 8, 2023
jonpryor
pushed a commit
to dotnet/android
that referenced
this pull request
Jun 9, 2023
Context: mono/SkiaSharp#2444 Context: mono/SkiaSharp#2465 Context: fcd7cf8 Context: dc3ccf2 Context: 3ec1b15 In Classic Xamarin.Android, `@(AndroidResource)` files were embedded into assemblies as `@(EmbeddedResource)` entries. While convenient, this slowed down build times (as every assembly needed to be checked to see if it had embedded resources), and this functionality was removed for .NET 6 in fcd7cf8. The replacement was to use `.aar` files, typically located in the same directory as the assembly. Unfortunately, it was easy to incorrectly migrate from the old system to the new system, which was the case for the [SkiaSharp.Views (v2.88.3) NuGet Package][0], which *uses* an [`@attr/ignorePixelScaling`][1] resource, but did not package it. In .NET 6 and 7, this was "fine" *so long as* that resource value was never used. If it *was* used, then the results were undefined: a "garbage" resource id value would be used, which would either be invalid or refer to a *different* resource altogether! .NET 8 introduces the Resource Designer Assembly (dc3ccf2), which involves *rewriting non-.NET 8 assemblies* to use the Resource Designer Assembly, which in turn requires that all resources exist. Which brings us to mono/SkiaSharp#2444: because the `SkiaSharp.Views` NuGet package references but did not redistribute the `@attr/ignorePixelScaling` resource, the assembly rewriter would *skip over* the missing resource name, resulting in an invalid assembly. This in turn would result in a *runtime crash*: System.BadImageFormatException: 'Invalid field token 0x040000a1' We decided that instead of ignoring missing resource fields, we should error out. Update `FixLegacyResourceDesignerStep` to report an error if we cannot find the required Resource Designer Assembly property which matches a "legacy" field. In Debug builds, this will result in an XA8000 error: error XA8000: Could not find Android Resource '@attr/ignorePixelScaling'. Please update @(AndroidResource) to add the missing resource. For Release builds, this instead results in an IL8000 error: error IL8000: Could not find Android Resource '@attr/ignorePixelScaling'. Please update @(AndroidResource) to add the missing resource. Note that this only moves a runtime error to a compile-time error. The fix for mono/SkiaSharp#2444 is mono/SkiaSharp#2465, which adds `SkiaSharp.Views.Android.aar` to the `SkiaSharp.Views` NuGet package. This should be fixed in the (forthcoming) SkaSharp.Views 2.88.4. [A *workaround* for mono/SkiaSharp#2444][3] is to define the `@attr/ignorePixelScaling` resource within your app project. Additionally, we found another problem while investigating mono/SkiaSharp#2444 around resource name case sensitivity. Not all Android Resource names need to be lowercase; generally only those that are based on *filenames* need to be lowercase, e.g. `@layout/main`. Resources which are defined within XML containers, such as [string resources][2], may contain uppercase letters. While looking at the output for the test app, it turns out the final assembly does NOT include the expected `anim` based resources. Looking at the IL we found that the properties were NOT being fixed up to use the correct casing, e.g. we have `enterfromright` instead of `EnterFromRight`. Further investigation tracked the issue down to the `RTxtParser`; This class uses the `case_map.txt` file for lookup. The `case_map.txt` file contains entries such as: animation/enterfromleft;animation/EnterFromLeft animation/enterfromright;animation/EnterFromRight these allow us to map the "android" lower cased items to the C# cased items. The problem was we were not mapping `anim` to `animation`. There is a method called `ResourceParser.GetNestedTypeName()` which is used to map the android resource type to the managed resource type. For example it maps `anim` to `Animation` and `bool` to `Boolean`. This was not being called when we tried to map the resource. As a result we never found the correct cased entry. This would result in the final Resource Designer Assembly containing the lower cased "android" naming of the property and not the correctly cased one which the C#/IL would be using; the fields were never fixed up. The fix is to make sure we call `ResourceParser.GetNestedTypeName()` when calculating the final property name in the `RTxtParser`. [0]: https://www.nuget.org/packages/SkiaSharp/2.88.3 [1]: https://github.com/mono/SkiaSharp/blob/99c2437b1055dc6eb8ee25e90e5ad4afa695aa89/source/SkiaSharp.Views/SkiaSharp.Views.Android/Resources/values/attrs.xml#L4 [2]: https://developer.android.com/guide/topics/resources/string-resource [3]: mono/SkiaSharp#2444 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of Change
Bugs Fixed
API Changes
None.
Behavioral Changes
None.
Required skia PR
None.
PR Checklist