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

[Xamarin.Android.Build.Tasks] Try a case insensitive lookup as a backup for Legacy Designer fixup #8376

Merged
merged 3 commits into from
Oct 10, 2023

Conversation

dellis1972
Copy link
Contributor

We have a very odd situation regarding the NuGet Xamarin.CommunityToolkit.MauiCompat.

The NuGet package ships with a file .net/__res_name_case_map.txt in the
Xamarin.CommunityToolkit.MauiCompat.aar which contains the
case map changes for the AndroidResource items. The purpose of this file is to allow
the user to use Camel case (or any case) names in C# code, while the android resource
compilation is left as all lowercase. This is because android requires lowercase resource
items.

The contents of this file are as follows

Resources/Layout/CameraFragment.axml;layout/camerafragment.xml

As you can see from this camerafragment is remaped to CameraFragment. With this
change we would expect to see code in C# which uses CameraFragment. This is true
as per https://github.com/xamarin/XamarinCommunityToolkit/blob/5a6062f3c3543acda3c36ca4683cd8fc7fe86ba7/src/CommunityToolkit/Xamarin.CommunityToolkit/Views/CameraView/Android/CameraFragment.android.cs#L129.

We can see that the code in the NuGet is using the CameraFragment casing.
However when we look at the IL that comes from the NuGet package we see the following

ikdasm ~/.nuget/packages/xamarin.communitytoolkit.mauicompat/2.0.2-preview1013/lib/net6.0-android31.0/Xamarin.CommunityToolkit.MauiCompat.dll| grep Layout::camerafragment
      IL_0131:  stsfld     int32 Xamarin.CommunityToolkit.MauiCompat.Resource/Layout::camerafragment
    IL_0001:  ldsfld     int32 Xamarin.CommunityToolkit.MauiCompat.Resource/Layout::camerafragment

For some reason as part of the maui release the following script was run on the code
https://github.com/xamarin/XamarinCommunityToolkit/blob/5a6062f3c3543acda3c36ca4683cd8fc7fe86ba7/MauiCompat.sh#L561.

This force changes the case of the CameraFragment to camerafragment across the whole project.
But the NuGet still includes the .aar with the case map changes (which are no longer needed).
I'm not sure why this happens but it results in the following build error when trying to consume this package
under .net8.0-android with the new Resource Designer.

XA8000
Could not find Android Resource ‘@layout/camerafragment’. Please update @(AndroidResource) to add the missing resource.

This is because we are looking for the lowercase version of the resource in the Designer assembly property list, but because
of the remap file. We only have the Camel case version.

The current workaround is to fallback to a case insensitive lookup... My first thought was to always use a case insensitive
lookup. But there are cases where users have two files with different cases. This is possible on systems like Linux and MacOS.

@jonpryor
Copy link
Member

@dellis1972 wrote:

My first thought was to always use a case insensitive lookup. But there are cases where users have two files with different cases. This is possible on systems like Linux and MacOS.

(macOS is case-insensitive by default, like Windows, so you can't have two files with different cases unless you create a new filesystem, e.g. APFS Case Sensitive.)

I think we may be overthinking this? For file-backed resources, we always lowercase the filename before passing on to aapt2/etc., because those tools require lowercase filenames. (This doesn't apply to resources declared within files, such as colors or strings.)

It thus should not be possible to have two file-based Android resources which differ only in case.

@dellis1972: does __res_name_case_map.txt only contain file-based resources, not all resources?

If __res_name_case_map.txt only deals with file-based resources, then I think your original thought was correct: always use case-insensitive (for file-based resources). If it applies to all resources…ugh.

Though I suspect that FixBody() is used for all resource types (colors, strings, ids, etc.), not just file-based resources, in which case we do need the "check for case-sensitive first, then fallback" approach.

@dellis1972
Copy link
Contributor Author

__res_name_case_map.txt only contains the files which have case changes.
But I think the fallback approach will be the safest option in this case.

@dellis1972 dellis1972 force-pushed the resourcestuff branch 3 times, most recently from a4127be to c3fb7cc Compare October 4, 2023 09:40
@dellis1972 dellis1972 marked this pull request as ready for review October 6, 2023 10:40
output.Add (key, property.GetMethod);
caseInsensitiveLookup.Add (key, property.GetMethod);
Copy link
Member

@jonpryor jonpryor Oct 9, 2023

Choose a reason for hiding this comment

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

This could be a problem, as .Add() will throw if a "duplicate key" already exists:

var d = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
d.Add ("A", "a");
d.Add ("a", "a");
// throws System.ArgumentException: An item with the same key has already been added. Key: a

Either we should use caseInsensitiveLookup.TryAdd(key, property.GetMethod) (.NET Standard 2.1), or we should use the indexer caseInsensitiveLookup[key] = property.GetMethod, though using the indexer means that caseInsensitiveLookup will always contain the last case-insensitive value encountered rather than the first (which might be fine?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well we only use .netstandard 2.0 so I guess that makes the decision for us.

@jonpryor
Copy link
Member

jonpryor commented Oct 9, 2023

[Xamarin.Android.Build.Tasks] case-insensitive Legacy Designer fixup (#8376)

Context: 628d6cb869c872c34d17b014724a43e8d451fb93
Context: dc3ccf28cdbe9f8c0a705400b83c11a85c81a980

We have a very odd situation with the
[`Xamarin.CommunityToolkit.MauiCompat` NuGet package][0]:

The NuGet package ships with a file `.net/__res_name_case_map.txt`
(628d6cb8) in `Xamarin.CommunityToolkit.MauiCompat.aar` which contains
the case mapchanges for the `@(AndroidResource)` items.  The purpose
of this file is to allow the user to use PascalCase (or any case) names
in C# code, while the Android Resource compilation is left as all
lowercase.  This is because Android requires that file-backed resource
names consist of only lowercase letters.

The contents of this file includes:

	Resources/Layout/CameraFragment.axml;layout/camerafragment.xml

which tells us that the Android `R.layout.camerafragment` resource id
should be "bound" as `Resouce.Layout.CameraFragment`, which allows
[`CameraFragment.android.cs` to build][1]:

	public override AView? OnCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) =>
	    inflater.Inflate(Resource.Layout.CameraFragment, null);

However, when we look at the IL that comes from the NuGet package we
see the following

	ikdasm ~/.nuget/packages/xamarin.communitytoolkit.mauicompat/2.0.2-preview1013/lib/net6.0-android31.0/Xamarin.CommunityToolkit.MauiCompat.dll| grep Layout::camerafragment
	
	    IL_0131:  stsfld     int32 Xamarin.CommunityToolkit.MauiCompat.Resource/Layout::camerafragment
	    IL_0001:  ldsfld     int32 Xamarin.CommunityToolkit.MauiCompat.Resource/Layout::camerafragment

For some reason as part of the maui release [the following script][2]
was run on the code:

	sed -i '' 's/MauiCompat.Resource.Layout.CameraFragment/MauiCompat.Resource.Layout.camerafragment/g' ./src/CommunityToolkit/Xamarin.CommunityToolkit.MauiCompat/**/CameraFragment.android.cs

This force changes the case of `CameraFragment` to `camerafragment`.
However, the NuGet still includes the `.aar` with the case map
changes, which are now inconsistent. 

I'm not sure why this happens but it results in the following build
error when trying to consume this package under net8.0-android with
the new Resource Designer Assembly (dc3ccf28):

	error XA8000: Could not find Android Resource ‘@layout/camerafragment’. Please update @(AndroidResource) to add the missing resource.

This is because we are looking for the lowercase version of the
resource in the Designer assembly property list, but because of the
remap file we only know about the PascalCase version. 

As a workaround, fallback to a *case-insensitive* lookup.

[0]: https://www.nuget.org/packages/Xamarin.CommunityToolkit.MauiCompat/
[1]: https://github.com/xamarin/XamarinCommunityToolkit/blob/5a6062f3c3543acda3c36ca4683cd8fc7fe86ba7/src/CommunityToolkit/Xamarin.CommunityToolkit/Views/CameraView/Android/CameraFragment.android.cs#L129
[2]: https://github.com/xamarin/XamarinCommunityToolkit/blob/5a6062f3c3543acda3c36ca4683cd8fc7fe86ba7/MauiCompat.sh#L561

@jonpryor jonpryor merged commit 94451e6 into xamarin:main Oct 10, 2023
1 check passed
@dellis1972 dellis1972 deleted the resourcestuff branch October 11, 2023 09:48
grendello added a commit to grendello/xamarin-android that referenced this pull request Oct 11, 2023
* main:
  [Xamarin.Android.Build.Tasks] case-insensitive Legacy Designer fixup (xamarin#8376)
  [build] Update docs and remove `xabuild` mentions (xamarin#8406)
  Bump to xamarin/java.interop/main@ed63d890 (xamarin#8407)
jonathanpeppers pushed a commit that referenced this pull request Oct 13, 2023
…8376)

Context: 628d6cb
Context: dc3ccf2

We have a very odd situation with the
[`Xamarin.CommunityToolkit.MauiCompat` NuGet package][0]:

The NuGet package ships with a file `.net/__res_name_case_map.txt`
(628d6cb) in `Xamarin.CommunityToolkit.MauiCompat.aar` which contains
the case map changes for the `@(AndroidResource)` items.  The purpose
of this file is to allow the user to use PascalCase (or any case) names
in C# code, while the Android Resource compilation is left as all
lowercase.  This is because Android requires that file-backed resource
names consist of only lowercase letters.

The contents of this file includes:

	Resources/Layout/CameraFragment.axml;layout/camerafragment.xml

which tells us that the Android `R.layout.camerafragment` resource id
should be "bound" as `Resouce.Layout.CameraFragment`, which allows
[`CameraFragment.android.cs` to build][1]:

	public override AView? OnCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) =>
	    inflater.Inflate(Resource.Layout.CameraFragment, null);

However, when we look at the IL that comes from the NuGet package we
see the only lowercase `camerafragment`:

	ikdasm ~/.nuget/packages/xamarin.communitytoolkit.mauicompat/2.0.2-preview1013/lib/net6.0-android31.0/Xamarin.CommunityToolkit.MauiCompat.dll| grep Layout::camerafragment
	…
	    IL_0131:  stsfld     int32 Xamarin.CommunityToolkit.MauiCompat.Resource/Layout::camerafragment
	…
	    IL_0001:  ldsfld     int32 Xamarin.CommunityToolkit.MauiCompat.Resource/Layout::camerafragment

For some reason as part of the XamarinCommunityToolkit build
[the following script][2] was run on the code:

	sed -i '' 's/MauiCompat.Resource.Layout.CameraFragment/MauiCompat.Resource.Layout.camerafragment/g' ./src/CommunityToolkit/Xamarin.CommunityToolkit.MauiCompat/**/CameraFragment.android.cs

This changes `Resource.Layout.CameraFragment` to
`Resource.Layout.camerafragment`, apparently because *something* is
renaming [`CameraFragment.axml`][3] to `camerafragment.axml` *after*
`__res_name_case_map.txt` is generated, but before
`CameraFragment.android.cs` is compiled.
The NuGet still includes the `.aar` with the case map changes, which
are now inconsistent with the assembly contents.

I'm not sure why XamarinCommunityToolkit does all this.

The result of the above XamarinCommunityToolkit build and packaging
behavior is that it interacts badly with the new net8.0-android
Resource Designer Assembly (dc3ccf2) work, with a build-time error:

	error XA8000: Could not find Android Resource ‘@layout/camerafragment’. Please update @(AndroidResource) to add the missing resource.

This is because we are looking for the lowercase version of the
resource in the Designer assembly property list, but because of the
remap file we only know about the PascalCase version. 

As a workaround, fallback to a *case-insensitive* resource name
lookup if a case-sensitive lookup fails.

[0]: https://www.nuget.org/packages/Xamarin.CommunityToolkit.MauiCompat/
[1]: https://github.com/xamarin/XamarinCommunityToolkit/blob/5a6062f3c3543acda3c36ca4683cd8fc7fe86ba7/src/CommunityToolkit/Xamarin.CommunityToolkit/Views/CameraView/Android/CameraFragment.android.cs#L129
[2]: https://github.com/xamarin/XamarinCommunityToolkit/blob/5a6062f3c3543acda3c36ca4683cd8fc7fe86ba7/MauiCompat.sh#L561
[3]: https://github.com/xamarin/XamarinCommunityToolkit/blob/5a6062f3c3543acda3c36ca4683cd8fc7fe86ba7/src/CommunityToolkit/Xamarin.CommunityToolkit/Resources/Layout/CameraFragment.axml
jonathanpeppers pushed a commit that referenced this pull request Oct 17, 2023
…8376)

Context: 628d6cb
Context: dc3ccf2

We have a very odd situation with the
[`Xamarin.CommunityToolkit.MauiCompat` NuGet package][0]:

The NuGet package ships with a file `.net/__res_name_case_map.txt`
(628d6cb) in `Xamarin.CommunityToolkit.MauiCompat.aar` which contains
the case map changes for the `@(AndroidResource)` items.  The purpose
of this file is to allow the user to use PascalCase (or any case) names
in C# code, while the Android Resource compilation is left as all
lowercase.  This is because Android requires that file-backed resource
names consist of only lowercase letters.

The contents of this file includes:

	Resources/Layout/CameraFragment.axml;layout/camerafragment.xml

which tells us that the Android `R.layout.camerafragment` resource id
should be "bound" as `Resouce.Layout.CameraFragment`, which allows
[`CameraFragment.android.cs` to build][1]:

	public override AView? OnCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) =>
	    inflater.Inflate(Resource.Layout.CameraFragment, null);

However, when we look at the IL that comes from the NuGet package we
see the only lowercase `camerafragment`:

	ikdasm ~/.nuget/packages/xamarin.communitytoolkit.mauicompat/2.0.2-preview1013/lib/net6.0-android31.0/Xamarin.CommunityToolkit.MauiCompat.dll| grep Layout::camerafragment
	…
	    IL_0131:  stsfld     int32 Xamarin.CommunityToolkit.MauiCompat.Resource/Layout::camerafragment
	…
	    IL_0001:  ldsfld     int32 Xamarin.CommunityToolkit.MauiCompat.Resource/Layout::camerafragment

For some reason as part of the XamarinCommunityToolkit build
[the following script][2] was run on the code:

	sed -i '' 's/MauiCompat.Resource.Layout.CameraFragment/MauiCompat.Resource.Layout.camerafragment/g' ./src/CommunityToolkit/Xamarin.CommunityToolkit.MauiCompat/**/CameraFragment.android.cs

This changes `Resource.Layout.CameraFragment` to
`Resource.Layout.camerafragment`, apparently because *something* is
renaming [`CameraFragment.axml`][3] to `camerafragment.axml` *after*
`__res_name_case_map.txt` is generated, but before
`CameraFragment.android.cs` is compiled.
The NuGet still includes the `.aar` with the case map changes, which
are now inconsistent with the assembly contents.

I'm not sure why XamarinCommunityToolkit does all this.

The result of the above XamarinCommunityToolkit build and packaging
behavior is that it interacts badly with the new net8.0-android
Resource Designer Assembly (dc3ccf2) work, with a build-time error:

	error XA8000: Could not find Android Resource ‘@layout/camerafragment’. Please update @(AndroidResource) to add the missing resource.

This is because we are looking for the lowercase version of the
resource in the Designer assembly property list, but because of the
remap file we only know about the PascalCase version. 

As a workaround, fallback to a *case-insensitive* resource name
lookup if a case-sensitive lookup fails.

[0]: https://www.nuget.org/packages/Xamarin.CommunityToolkit.MauiCompat/
[1]: https://github.com/xamarin/XamarinCommunityToolkit/blob/5a6062f3c3543acda3c36ca4683cd8fc7fe86ba7/src/CommunityToolkit/Xamarin.CommunityToolkit/Views/CameraView/Android/CameraFragment.android.cs#L129
[2]: https://github.com/xamarin/XamarinCommunityToolkit/blob/5a6062f3c3543acda3c36ca4683cd8fc7fe86ba7/MauiCompat.sh#L561
[3]: https://github.com/xamarin/XamarinCommunityToolkit/blob/5a6062f3c3543acda3c36ca4683cd8fc7fe86ba7/src/CommunityToolkit/Xamarin.CommunityToolkit/Resources/Layout/CameraFragment.axml
@github-actions github-actions bot locked and limited conversation to collaborators Jan 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants