-
Notifications
You must be signed in to change notification settings - Fork 564
Fix an Issue with new Resource Desginer Assembly #8066
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
Conversation
c301924 to
078cf5b
Compare
jonathanpeppers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, looks good to me -- we have a test that runs an app, verifying it no longer crashes. 👍
src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixLegacyResourceDesignerStep.cs
Outdated
Show resolved
Hide resolved
src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixLegacyResourceDesignerStep.cs
Outdated
Show resolved
Hide resolved
| #if ILLINK | ||
| Context.LogMessage (MessageContainer.CreateCustomErrorMessage (error, code, origin: new MessageOrigin ())); | ||
| #else // !ILLINK | ||
| Console.Error.WriteLine ($"error XA{code}: {error}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If ILLINK is not defined, we should be within Xamarin.Android.Build.Tasks.dll, right? In which case we should have a "real" MSBuild Task that can raise "real" MSBuild error messages, instead of going through Console.Error.WriteLine()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done in https://github.com/xamarin/xamarin-android/pull/8066/files#diff-6bfa8cb4f52234d33d427a9aa52682179ea4e86d986fa7b2c6c26504ffcd6d96R145 by overriding this method. The Console.Error.WriteLine here is just a backup.
| } | ||
| }, | ||
| "PackageSize": 2632010 | ||
| "PackageSize": 2685258 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not related to this PR, but why'd the package size increase so much?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like its
"assemblies/System.Private.CoreLib.dll": {
"Size": 516811
"Size": 536436
},
or at least a fair chunk of it will be
| } | ||
| }, | ||
| "PackageSize": 7930717 | ||
| "PackageSize": 7877469 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…and this one shrank. Nice! …but weird?
f6ae490 to
171926f
Compare
[Xamarin.Android.Build.Tasks] missing resource error handling (#8066)
Context: https://github.com/mono/SkiaSharp/issues/2444
Context: https://github.com/mono/SkiaSharp/pull/2465
Context: fcd7cf8f4200ccae92e333c920613bfec260973b
Context: dc3ccf28cdbe9f8c0a705400b83c11a85c81a980
Context: 3ec1b159b8125cfaea1d774da94b323f08e2432b
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 fcd7cf8f. 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], 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 (dc3ccf28), 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 throw an exception if we
cannot find the required Resource Designer Assembly property which
matches a "legacy" field.
In Debug builds, this will result in the build-time 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]: https://github.com/mono/SkiaSharp/issues/2444#issuecomment-1564090245 |
* main: LEGO: Merge pull request 8119 [build] Bump `$(XABuildToolsVersion)`=34 (dotnet#8118) [Xamarin.Android.Build.Tasks] missing resource error handling (dotnet#8066) [CI] Allow MSBuild test stages to run in the megapipeline (dotnet#8033) [ci] Fix nightly test environment provisioning (dotnet#8113)
Context mono/SkiaSharp#2444.
So while investigating the issue above we found there is an issue with the new Resource Designer Assembly implementation. The problem in the issue linked was that the SkiaSharp.Views Nuget package was not shipping the .aar file for its net8.0-android platform. As a result the
ignorePixelScalingfield in the ShiaSharp.Views assembly was not found in the new Resource Designer Assembly. This results in the field NOT being changed to use the Resource Designer Assembly Property (because it does not exist). It is this which results in the runtime errorWe decided that instead of ignoring missing resource fields, we should error out. This PR includes an update to the
FixLegacyResourceDesignerStepwhich will throw and exception if we cannot find the required property to match a field. The new error will be the following for Debug buildsor for Release builds
It also however, highlighted a problem with the Resource Designer Assembly. Looking at the output for the test app it turns out the final assembly does NOT include the expected
animbased resources.Looking at the IL we found that the properties were NOT being fixed up to use the correct casing. So we have
enterfromrightinstead ofEnterFromRight. Further investigation tracked the issue down to theRTxtParser. This class uses thecase_map.txtfile to lookup. Thecase_map.txtfile contains entries such asthese allow use to map the "android" lower cased items to the C# cased items.
The problem was we were not mapping
animtoanimation. There is a method calledResourceParser.GetNestedTypeNamewhich is used to map the android resource type to themanaged resource type. For example it maps
animtoAnimationandbooltoBoolean.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. So the fields were never
fixed up.
So in this case the fix is to make sure we call
ResourceParser.GetNestedTypeNamewhen calculating the final property name in theRTxtParser.