Special Char Support #6273

dellis1972 · 2021-09-08T12:30:50Z

Binutils 2.36 introduced code to deal extra long path names on
Windows which makes compilation (in our case using as) and linking
to break if the source/object files reside in a directory which has
special characters (in our case they were テスト):

Xamarin.Android.Common.targets(1887,3): error XA3006: Could not compile native assembly file: typemaps.armeabi-v7a.s [SmokeTestBuildWithSpecialCharactersFalse\テスト\テスト.csproj]
  [aarch64-linux-android-as.EXE stderr] Assembler messages: (TaskId:187)
  [aarch64-linux-android-as.EXE stderr] Fatal error: can't create typemaps.arm64-v8a.o: Invalid argument (TaskId:187)

Downgrade binutils version to 2.35.2 which doesn't contain the offending
code block and will work for us, with relative paths. It will still
break with full paths if the path contains special characters, but since
our use case involves only relative paths, this is fine.

There is also a bug in aapt2 [1] which stops it processing a directory which has
certain special characters on windows. This will only appear on projects which
have library resources, such as Xamarin.Forms. The problem only appears when
processing a directory into a flata archive. Oddly it works file when processing a
file directly. In order to work around this we now generate a resource only res.zip
file when we extract the library resources. This res.zip is then used during the
generation of the flata archive. Weirdly, this works where directory transversal does
not.

Additionally it seems to be slightly quicker

Before
581 ms Aapt2Compile 1 calls

After
366 ms Aapt2Compile 1 calls

So the time taken to generate the res.zip is offset by the quicker execution of
aapt2 compile.

Finally, java tooling such as zipalign and apksigner also failed with the
special characters. This was because we were using full paths to the files
we were signing/aligning. Switching over to use relative paths fixes this particular
problem.

Add a set of Unit tests to make sure we can handle the special characters.

[1] https://issuetracker.google.com/issues/188679588

jonpryor · 2021-09-09T01:14:36Z

I was going to comment that the test works, so seemingly there isn't a problem, right?

…but then I thought about it: tests/MSBuildDeviceIntegration only appears to be run on macOS!

https://github.com/xamarin/xamarin-android/blob/fe8c8514fa59446e49eba6b52fd23747ca7c131f/build-tools/automation/azure-pipelines.yaml#L901-L906

https://github.com/xamarin/xamarin-android/blob/fe8c8514fa59446e49eba6b52fd23747ca7c131f/build-tools/automation/azure-pipelines.yaml#L1030-L1048

I don't think any of tests/MSBuildDeviceIntegration is run on Windows, is it?

Which means we have no idea if non-ASCII paths work on Windows at all, at least not via CI, right?

dellis1972 · 2021-09-09T10:10:26Z

I'll see about adding the SmokeTest run of MSBuildDeviceIntegration to the windows smoke test run

dellis1972 · 2021-09-23T10:31:44Z

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidUpdateResourcesTest.cs

 					rawToDelete,
 				},
 			};
+			libProj.SetProperty ("Deterministic", "true");


@jonathanpeppers I'm not sure why I've had to add this to this particular test. It was failing without it.

Frankly, we should use $(Deterministic)=true everywhere…

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs

dellis1972 · 2021-09-23T14:27:58Z

Turns out @jonathanpeppers found AndroidStudio doesn't even let you create a project with these special characters. So I guess it doesn't work at all in Android Studio.

src/Xamarin.Android.Build.Tasks/Tasks/ResolveLibraryProjectImports.cs

src/Xamarin.Android.Build.Tasks/Tasks/Aapt2Compile.cs

jonpryor · 2021-10-06T16:57:51Z

src/Xamarin.Android.Build.Tasks/Tasks/Aapt2Compile.cs

 			if (isDirectory) {
-				cmd.Add ("--dir");
+				cmd.Add (isArchive ? "--zip" : "--dir");
 				cmd.Add (GetFullPath (fileOrDirectory).TrimEnd ('\\'));


Considering that this PR is in the context of "Chinese character set support", and that this is a command-line argument to aapt2, should this really use GetFullPath(fileOrDirectory) at all anymore?

I'm kinda surprised that this works at all, in that the PRs are green…

It needs to use get full path because aapt2 does not work in the project directory, but in a temp directory. This is to get around problems where the aapt2 daemons would lock the project directory while VS was running. So ALL the paths we pass to aapt2 need to be full paths.

The reason this works is because passing a zip file means aapt2 does not try to traverse the directory, which is where the buggy code is in aapt2.

jonpryor · 2021-10-06T16:58:17Z

src/Xamarin.Android.Build.Tasks/Tasks/AndroidApkSigner.cs

 				cmd.AppendSwitch (AdditionalArguments);

-			cmd.AppendSwitchIfNotNull (" ", Path.GetFullPath (ApkToSign));
+			cmd.AppendSwitchIfNotNull (" ", ApkToSign);


…especially considering that we're removing Path.GetFullPath() here…

src/Xamarin.Android.Build.Tasks/Tasks/ResolveLibraryProjectImports.cs

jonpryor · 2021-10-06T17:01:59Z

src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets

 	</PropertyGroup>
 	<AndroidSignPackage Condition=" '$(AndroidUseApkSigner)' != 'true' "
-		UnsignedApk="%(ApkAbiFilesIntermediate.FullPath)"
+		UnsignedApk="%(ApkAbiFilesIntermediate.Identity)"


If we're using %(Identity), why not instead use @(ApkAbiFilesIntermediate)?

Ditto subsequent changes as well. Why prefer %(Foo.Identity) over @(Foo)?

So AndroidSignPackage and ApkSigner need to use %(Foo.Identity) so that MSbuild will batch the calls. These tools can only sign one package at a time. If we provided @(Foo) and we had multiple apps (like we do when we split on abi) we would send the following to the signing tool

fooo.apk;foo2.apk

which is not what we want. What we want is for MSbuild to call AndroidSignPackage for each apk, and we do that via batching.

src/Xamarin.Android.Build.Tasks/Xamarin.Android.EmbeddedResource.targets

dellis1972 · 2021-10-12T08:46:49Z

/azp run

azure-pipelines · 2021-10-12T08:47:15Z

Azure Pipelines successfully started running 1 pipeline(s).

Binutils 2.36 introduced [code][0] to deal extra long path names on Windows which makes compilation (in our case using `as`) and linking to break if the source/object files reside in a directory which has special characters (in our case they were `テスト`): Xamarin.Android.Common.targets(1887,3): error XA3006: Could not compile native assembly file: typemaps.armeabi-v7a.s [SmokeTestBuildWithSpecialCharactersFalse\テスト\テスト.csproj] [aarch64-linux-android-as.EXE stderr] Assembler messages: (TaskId:187) [aarch64-linux-android-as.EXE stderr] Fatal error: can't create typemaps.arm64-v8a.o: Invalid argument (TaskId:187) Downgrade binutils version to 2.35.2 which doesn't contain the offending code block and will work for us, with relative paths. It will **still** break with full paths if the path contains special characters, but since our use case involves only relative paths, this is fine. There is also a bug in aapt2 [1] which stops it processing a directory which has certain special characters on windows. This will only appear on projects which have library resources, such as Xamarin.Forms. The problem only appears when processing a directory into a flata archive. Oddly it works file when processing a file directly. In order to work around this we now generate a resource only `res.zip` file when we extract the library resources. This `res.zip` is then used during the generation of the `flata` archive. Weirdly, this works where directory transversal does not. Additionally it seems to be slightly quicker Before `581 ms Aapt2Compile 1 calls` After `366 ms Aapt2Compile 1 calls` So the time taken to generate the `res.zip` is offset by the quicker execution of `aapt2 compile`. Finally, `java` tooling such as `zipalign` and `apksigner` also failed with the special characters. This was because we were using full paths to the files we were signing/aligning. Switching over to use relative paths fixes this particular problem. Add a set of Unit tests to make sure we can handle the special characters. [0]: https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=bfd/bfdio.c;h=463b3879c52ba6beac47190f8eb0810b0c330e65;hb=HEAD#l124 [1] https://issuetracker.google.com/issues/188679588

jonathanpeppers · 2021-10-15T13:25:40Z

src/Xamarin.Android.Build.Tasks/Tasks/ResolveLibraryProjectImports.cs

+					CreateResourceArchive (resDir, resDirArchive);
 					var skipProcessing = aarFile.GetMetadata (AndroidSkipResourceProcessing);
 					if (string.IsNullOrEmpty (skipProcessing)) {
 						skipProcessing = "True";
 					}
 					resolvedResourceDirectories.Add (new TaskItem (Path.GetFullPath (resDir), new Dictionary<string, string> {
 						{ OriginalFile, aarFullPath },
 						{ AndroidSkipResourceProcessing, skipProcessing },
+						{ ResourceDirectoryArchive, Path.GetFullPath (resDirArchive)},


For .aar files, can we simply set their existing path to %(ResourceDirectoryArchive) and not create a new .zip file?

I think this will help performance quite a bit for AndroidX, which is mostly .aar files now.

I tried that. aapt2 errors because there are files like classes.jar etc in there. It only seems to like files which ONLY contain res files.

jonathanpeppers · 2021-10-15T14:49:17Z

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs

+		[Test]
+		[Category ("SmokeTests")]
+		public void SmokeTestBuildWithSpecialCharacters ([Values (false, true)] bool forms)
+		{
+			var testName = "テスト";
+
+			var rootPath = Path.Combine (Root, "temp", TestName);
+			var proj = forms ?
+				new XamarinFormsAndroidApplicationProject () :
+				new XamarinAndroidApplicationProject ();


Should we put a test like this in MSBuildDeviceIntegration, to make sure it deploys and the app launches?

I'm wondering if something Fast Deployment might break, and we didn't catch it yet.

The problem, as outlined earlier, is that the MSBuildDeviceIntegration tests only run on macOS, which doesn't have a "non-ASCII characters" problem in the first place.

What we want/need is for MSBuildDevice integration to run on Windows. The problem is we tried that in an earlier iteration of #6297, and hit #6297 (comment)

Instead, it (later) insta-fails, because of virtualization:

[emulator stdout] emulator: ERROR: x86_64 emulation currently requires hardware acceleration! (TaskId:5) C:\a\_work\1\s\build-tools\scripts\TestApks.targets(65,5): error : Emulator failed to start: emulator: ERROR: x86_64 emulation currently requires hardware acceleration! [C:\a\_work\1\s\tests\Mono.Android-Tests\Mono.Android-Tests.csproj] [emulator stdout] CPU acceleration status: Hyper-V detected and Windows Hypervisor Platform is available. Please ensure both the "Hyper-V" and "Windows Hypervisor Platform" features enabled in "Turn Windows features on or off". (TaskId:5) [emulator stdout] More info on configuring VM acceleration on Windows: (TaskId:5) [emulator stdout] https://developer.android.com/studio/run/emulator-acceleration#vm-windows (TaskId:5) [emulator stdout] General information on acceleration: https://developer.android.com/studio/run/emulator-acceleration. (TaskId:5)

@pjcollins had additional followup: #6297 (comment)

Investigate Helix machine pools for Android emulator testing.

Revisit physical Windows machine usage. One possible target would be the shared pool we used to run QA tests on. Some of these machines have physical Android devices attached, but could still potentially be used: …

See if we can somehow enable virtualization on our 1ES Windows pool.

In the meantime, we're going to need to rely on manual testing for Windows (ugh).

This suggests an "alternate half step": instead of requiring "full" MSBuildDeviceIntegration tests, would building the project with /t:SignAndroidPackage /p:Configuration=Release be a meaningfully useful intermediate step? This wouldn't test fast deployment -- or any deployment, for that matter -- but would hit the "binutils doesn't like non-ASCII" codepath, plus aapt, plus lots of other parts of the build system.

Should we add a "build (no device attached!) test" around the SignAndroidPackage target and non-ASCII directories?

This test already does that. A basic release build produces the apk etc, so it will go through all the bin utils stuff, and aapt2. It runs on windows and Mac.

We do have a version of the same test which runs on Device but that is only gonna run on Mac because the emulator cannot run on the build machines we use for windows.

jonpryor · 2021-10-18T21:28:40Z

[Xamarin.Android.Build.Tasks] Special Char Support (#6273)

What would we like?  The ability for Xamarin.Android projects to
reside in directories containing non-ASCII characters.

The problem(s)?  For starters, many Android SDK utilities don't
support non-ASCII characters when running on Windows, e.g.:

  * https://issuetracker.google.com/issues/188679588

Android Studio (kinda/sorta) "enforces" this, showing a warning
message when **Save location** contains non-ASCII characters:

![Android Studio New Project dialog with non-ASCII characters](https://user-images.githubusercontent.com/184788/134550831-b5c5335a-11ca-4296-b30c-cb54e3302917.png)

> Save location: /tmp/テスト
> …
> ⚠️ Your project location contains non-ASCII characters.

These are not particularly encouraging foundations.

Improve support for non-ASCII characters by updating various parts
of our build system to:

 1. Prefer *relatives* paths when possible, as we can ensure
    ASCII-only paths this way, and

 2. "Otherwise" change *how* we invoke Android tools to avoid their
    use of full paths and directory traversal.

Additionally, we've long used GNU Binutils to build `libxamarin-app.so`
(commit decfbccf, fc3f0282, others), and we discovered that
Binutils 2.36 introduced [code][0] to deal extra long path names on
Windows which makes compilation (in our case using `as`) and linking
to break if the source/object files reside in a directory which has
non-ASCII characters (in our case they were `テスト`):

	Xamarin.Android.Common.targets(1887,3): error XA3006: Could not compile native assembly file: typemaps.armeabi-v7a.s [SmokeTestBuildWithSpecialCharactersFalse\テスト\テスト.csproj]
	  [aarch64-linux-android-as.EXE stderr] Assembler messages: (TaskId:187)
	  [aarch64-linux-android-as.EXE stderr] Fatal error: can't create typemaps.arm64-v8a.o: Invalid argument (TaskId:187)

Downgrade to Binutils 2.35.2, which doesn't contain the offending
code block and will work for us, so long as we use relative paths.
It will **still** break with full paths if the path contains special
characters, but since our use case involves only relative paths,
this is fine.

There is also a bug in [`aapt2`][1] which stops it processing a
directory which has certain special characters on Windows.  This will
only appear on projects which have library resources, such as
Xamarin.Forms.  The problem only appears when processing a directory
into a `.flata` archive.  Oddly it works fine when processing a file
directly.  In order to work around this we now generate a resource
only `res.zip` file when we extract the library resources.
`res.zip` is then used during the generation of the `.flata` archive.
Weirdly, this works where directory transversal does not.

Additionally it seems to be slightly quicker during incremental builds:

  * Before

        581 ms  Aapt2Compile                               1 calls

  * After

        366 ms  Aapt2Compile                               1 calls

So the time taken to generate the `res.zip` is offset by the quicker
execution of `aapt2 compile`.

Finally, Java tooling such as `zipalign` and `apksigner` also failed
with the special characters.  This was because we were using full
paths to the files we were signing/aligning. Switching over to use
relative paths fixes this particular problem.

Add a set of Unit tests to make sure we can handle the special characters.

[0]: https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=bfd/bfdio.c;h=463b3879c52ba6beac47190f8eb0810b0c330e65;hb=HEAD#l124
[1]: https://issuetracker.google.com/issues/188679588

dellis1972 requested a review from jonpryor as a code owner September 8, 2021 12:30

dellis1972 marked this pull request as draft September 8, 2021 12:31

dellis1972 force-pushed the specialcharacters branch 3 times, most recently from 29b21fb to 04ea048 Compare September 10, 2021 12:21

grendello mentioned this pull request Sep 10, 2021

Downgrade binutils to 2.35.2 #6278

Closed

dellis1972 force-pushed the specialcharacters branch 2 times, most recently from 53a1483 to 74ed2b3 Compare September 22, 2021 16:19

dellis1972 commented Sep 23, 2021

View reviewed changes

jonathanpeppers reviewed Sep 23, 2021

View reviewed changes

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs Show resolved Hide resolved

jonathanpeppers reviewed Sep 23, 2021

View reviewed changes

src/Xamarin.Android.Build.Tasks/Tasks/ResolveLibraryProjectImports.cs Show resolved Hide resolved

dellis1972 force-pushed the specialcharacters branch 2 times, most recently from 0850448 to 80a4cdc Compare October 4, 2021 13:47

dellis1972 changed the title ~~Special Char Unit Test [WIP]~~ Special Char Unit Test Oct 5, 2021

dellis1972 changed the title ~~Special Char Unit Test~~ Special Char Support Oct 5, 2021

dellis1972 marked this pull request as ready for review October 5, 2021 10:29

jonpryor reviewed Oct 6, 2021

View reviewed changes

src/Xamarin.Android.Build.Tasks/Tasks/Aapt2Compile.cs Outdated Show resolved Hide resolved

jonpryor reviewed Oct 6, 2021

View reviewed changes

src/Xamarin.Android.Build.Tasks/Tasks/Aapt2Compile.cs Outdated Show resolved Hide resolved

jonpryor reviewed Oct 6, 2021

View reviewed changes

src/Xamarin.Android.Build.Tasks/Tasks/ResolveLibraryProjectImports.cs Outdated Show resolved Hide resolved

jonpryor reviewed Oct 6, 2021

View reviewed changes

src/Xamarin.Android.Build.Tasks/Xamarin.Android.EmbeddedResource.targets Outdated Show resolved Hide resolved

dellis1972 force-pushed the specialcharacters branch from 80a4cdc to 5c62d13 Compare October 8, 2021 10:56

dellis1972 force-pushed the specialcharacters branch from bbb563f to e730adf Compare October 12, 2021 09:04

dellis1972 force-pushed the specialcharacters branch from e730adf to 5d9ef9a Compare October 14, 2021 09:25

dellis1972 requested review from grendello, jonathanpeppers and jonpryor October 15, 2021 09:53

jonathanpeppers reviewed Oct 15, 2021

View reviewed changes

grendello approved these changes Oct 15, 2021

View reviewed changes

jonathanpeppers reviewed Oct 15, 2021

View reviewed changes

jonathanpeppers approved these changes Oct 18, 2021

View reviewed changes

jonpryor merged commit fd5f31c into dotnet:main Oct 18, 2021

dellis1972 deleted the specialcharacters branch October 19, 2021 10:16

dellis1972 mentioned this pull request Apr 25, 2022

Android - APT2000 build error when project path contains non-ASCII symbols #6925

Open

github-actions bot locked and limited conversation to collaborators Jan 24, 2024

Special Char Support #6273

Special Char Support #6273

Uh oh!

Conversation

dellis1972 commented Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonpryor commented Sep 9, 2021

Uh oh!

dellis1972 commented Sep 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dellis1972 commented Sep 23, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dellis1972 commented Oct 12, 2021

Uh oh!

azure-pipelines bot commented Oct 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonpryor commented Oct 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dellis1972 commented Sep 8, 2021 •

edited

Loading