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] fix for utf8 chars and custom views #4726

Merged
merged 1 commit into from
May 27, 2020

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented May 26, 2020

Fixes: https://developercommunity.visualstudio.com/content/problem/955972/the-calltarget-task-returned-false-but-did-not-log.html

Android layout files with utf8 characters and custom views fail to
build with:

error APT2258: not well-formed (invalid token).

In be47b3e, I inadvertently changed the <ConvertCustomView/>
MSBuild task to save changes using Encoding.Default.
XDocumentExtensions.SaveIfChanged should just use
MonoAndroidHelper.UTF8withoutBOM by default.

The other places that use this method, will not be affected by using
UTF-8 without a BOM. I also fixed two other places in the build that
use Encoding.Default.

I updated two tests to reproduce this issue.

@jonpryor
Copy link
Member

Should we audit the codebase for other uses of Encoding.Default?

% git grep Encoding.Default
build-tools/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/Adb.cs:			si.StandardOutputEncoding = Encoding.Default;
build-tools/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/Adb.cs:			si.StandardErrorEncoding = Encoding.Default;
build-tools/xa-prep-tasks/Xamarin.Android.BuildTools.PrepTasks/GitCommitInfo.cs:				StandardOutputEncoding = Encoding.Default,
build-tools/xa-prep-tasks/Xamarin.Android.BuildTools.PrepTasks/GitCommitInfo.cs:				StandardErrorEncoding = Encoding.Default,
build-tools/xaprepare/xaprepare/Application/ProcessRunner.cs:		public Encoding StandardOutputEncoding                               { get; set; } = Encoding.Default;
build-tools/xaprepare/xaprepare/Application/ProcessRunner.cs:		public Encoding StandardErrorEncoding                                { get; set; } = Encoding.Default;
build-tools/xaprepare/xaprepare/Application/ProcessStandardStreamWrapper.cs:	    public override Encoding Encoding => Encoding.Default;
build-tools/xaprepare/xaprepare/ToolRunners/MSBuildRunner.OutputSink.cs:			public override Encoding Encoding => Encoding.Default;
build-tools/xaprepare/xaprepare/ToolRunners/NuGetRunner.OutputSink.cs:			public override Encoding Encoding => Encoding.Default;
build-tools/xaprepare/xaprepare/ToolRunners/SevenZipRunner.OutputSink.cs:			public override Encoding Encoding => Encoding.Default;
build-tools/xaprepare/xaprepare/ToolRunners/SnRunner.OutputSink.cs:			public override Encoding Encoding => Encoding.Default;
build-tools/xaprepare/xaprepare/ToolRunners/TarRunner.OutputSink.cs:			public override Encoding Encoding => Encoding.Default;
build-tools/xaprepare/xaprepare/ToolRunners/ToolRunner.OutputSink.cs:			public override Encoding Encoding => Encoding.Default;
src/Mono.Android/map.csv:E,10,android/media/AudioFormat.ENCODING_DEFAULT,1,Android.Media.Encoding,Default,keep,
src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs:			using (var acw_map = MemoryStreamPool.Shared.CreateStreamWriter (Encoding.Default)) {
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildAssetsTest.cs:						Assert.AreEqual ("bar", Encoding.Default.GetString (memory.ToArray ()));
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/FilesTests.cs:		Stream NewStream (string contents) => new MemoryStream (Encoding.Default.GetBytes (contents));
src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs:			using (var writer = MemoryStreamPool.Shared.CreateStreamWriter (Encoding.Default)) {
src/Xamarin.Android.Build.Tasks/Utilities/XDocumentExtensions.cs:			using (var sw = MemoryStreamPool.Shared.CreateStreamWriter (Encoding.Default))
src/Xamarin.Android.NUnitLite/NUnitLite/Runner/ConsoleWriter.cs:            get { return System.Text.Encoding.Default; }
src/Xamarin.Android.NUnitLite/NUnitLite/Runner/DebugWriter.cs:            get { return System.Text.Encoding.Default; }
src/Xamarin.Android.NUnitLite/NUnitLite/Runner/OutputWriters/NUnit2XmlOutputWriter.cs:        //    char[] encodedChars = System.Text.Encoding.Default.GetChars(System.Text.Encoding.Default.GetBytes(encodedString));
src/Xamarin.Android.NUnitLite/NUnitLite/Runner/OutputWriters/NUnit2XmlOutputWriter.cs:        //    return System.Text.Encoding.Default.GetString(System.Text.Encoding.Default.GetBytes(encodedChars));
src/Xamarin.Android.NUnitLite/NUnitLite/Runner/TcpWriter.cs:            get { return System.Text.Encoding.Default; }

Some of these are around Process.Standard*Encoding, and likely shouldn't be changed, but the others are possibly worrying or need review.

@jonathanpeppers
Copy link
Member Author

I removed two that seem problematic during builds.

@grendello might want to look at the usage in xaprepare or NUnitLite?

Copy link
Contributor

@brendanzagaeski brendanzagaeski left a comment

Choose a reason for hiding this comment

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

While drafting release notes, I realized a way to produce the XARLC7000 symptom. What surprised me at first glance was that the BuildApplicationWithSpacesInPath test didn't yet hit it. For the context of this PR, maybe it's worth adding a library project to that test so it will cover a path that can hit this scenario? Or maybe BuildAMassiveApp could start adding some coverage of non-ASCII characters too? (For the future, it might be useful to add accented characters to a few other tests too.)

Candidate change for BuildApplicationWithSpacesInPath:

diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs
index 107c50e6..b15a86d0 100644
--- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs
+++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs
@@ -2583,11 +2583,16 @@ public class Test
 		[Category ("SmokeTests")]
 		public void BuildApplicationWithSpacesInPath ([Values (true, false)] bool enableMultiDex, [Values ("dx", "d8")] string dexTool, [Values ("", "proguard", "r8")] string linkTool)
 		{
+			var lib = new XamarinAndroidLibraryProject {
+				IsRelease = true,
+				ProjectName = "Library1"
+			};
 			var proj = new XamarinAndroidApplicationProject () {
 				IsRelease = true,
 				AotAssemblies = true,
 				DexTool = dexTool,
 				LinkTool = linkTool,
+				References = { new BuildItem ("ProjectReference", $"..\\BuildReleaseAppWithA InItAndÜmläütsLibrary1({enableMultiDex}{dexTool}{linkTool})\\Library1.csproj") }
 			};
 			proj.OtherBuildItems.Add (new BuildItem ("AndroidJavaLibrary", "Hello (World).jar") { BinaryContent = () => Convert.FromBase64String (@"
 UEsDBBQACAgIAMl8lUsAAAAAAAAAAAAAAAAJAAQATUVUQS1JTkYv/soAAAMAUEsHCAAAAAACAAAAA
@@ -2617,7 +2622,9 @@ AAMMAAABzYW1wbGUvSGVsbG8uY2xhc3NQSwUGAAAAAAMAAwC9AAAA1gEAAAAA") });
 </Project>
 ",
 			});
+			using (var libb = CreateDllBuilder (Path.Combine ("temp", $"BuildReleaseAppWithA InItAndÜmläütsLibrary1({enableMultiDex}{dexTool}{linkTool})")))
 			using (var b = CreateApkBuilder (Path.Combine ("temp", $"BuildReleaseAppWithA InItAndÜmläüts({enableMultiDex}{dexTool}{linkTool})"))) {
+				libb.Build (lib);
 				if (dexTool == "d8" && linkTool == "proguard") {
 					b.ThrowOnBuildFailure = false;
 					Assert.IsFalse (b.Build (proj), "Build should have failed.");

This fails when SaveIfChanged() is using Encoding.Default and passes after applying the changes in this PR.

Draft release notes

As this PR is finalized for merge, when you get a chance, you can add a release note for it to Documentation/release-notes/4726.md as part of the PR. Here's a rough first draft. Thanks!

#### Application and library build and deployment

- [Developer Community 955972](https://developercommunity.visualstudio.com/content/problem/955972/the-calltarget-task-returned-false-but-did-not-log.html)
  Starting in Xamarin.Android 10.3, _error APT2258: not well-formed (invalid
  token)_ prevented using accented characters or other non-ASCII UTF8
  characters in Android layout files that also contained custom views.
- [Developer Community 955972](https://developercommunity.visualstudio.com/content/problem/955972/the-calltarget-task-returned-false-but-did-not-log.html)
  Starting in Xamarin.Android 10.3, errors similar to _error XARLC7000:
  System.Xml.XmlException: Invalid character in the given encoding_ could
  prevent successful builds for certain project configurations, such as
  projects located in directory paths that contained accented characters or
  other non-ASCII UTF8 characters.

For this special case, I listed the item twice because users could separately hit the XARLC7000 symptom or the APT2258 symptom without hitting the other one.

@grendello
Copy link
Contributor

I removed two that seem problematic during builds.

@grendello might want to look at the usage in xaprepare or NUnitLite?

xaprepare is fine, just checked. We use Encoding.Default only when reading process input. NUnitLite likewise.

@dellis1972
Copy link
Contributor

@jonathanpeppers I think we will need this on 16.7 I just did some tests on Windows with accented paths and files like libraryprojectimports.cache no longer work correctly. We get the following error

System.Xml.XmlException: Invalid character in the given encoding. Line 1, position 39.   at System.Xml.XmlTextReaderImpl.Throw(Exception e)   at System.Xml.XmlTextReaderImpl.Throw(String res, String arg)   at System.Xml.XmlTextReaderImpl.InvalidCharRecovery(Int32& bytesCount, Int32& charsCount)   at System.Xml.XmlTextReaderImpl.GetChars(Int32 maxCharsCount)   at System.Xml.XmlTextReaderImpl.ReadData()   at System.Xml.XmlTextReaderImpl.ParseText(Int32& startPos, Int32& endPos, Int32& outOrChars)   at System.Xml.XmlTextReaderImpl.FinishPartialValue()   at System.Xml.XmlTextReaderImpl.get_Value()   at System.Xml.Linq.XContainer.ReadContentFrom(XmlReader r)   at System.Xml.Linq.XContainer.ReadContentFrom(XmlReader r, LoadOptions o)   at System.Xml.Linq.XDocument.Load(XmlReader reader, LoadOptions options)   at System.Xml.Linq.XDocument.Load(String uri, LoadOptions options)   at System.Xml.Linq.XDocument.Load(String uri)   at Xamarin.Android.Tasks.ReadLibraryProjectImportsCache.RunTask()   at Xamarin.Android.Tasks.AndroidTask.Execute() | App1 |   |   |  

For some reason it is also missing the xml declaration at the top that might be related, but I'm not sure.

Fixes: https://feedback.devdiv.io/955972

Android layout files with utf8 characters and custom views fail to
build with:

    error APT2258: not well-formed (invalid token).

In be47b3e, I inadvertently changed the `<ConvertCustomView/>`
MSBuild task to save changes using `Encoding.Default`.
`XDocumentExtensions.SaveIfChanged` should just use
`MonoAndroidHelper.UTF8withoutBOM` by default.

The other places that use this method, will not be affected by using
UTF-8 without a BOM. I also fixed two other places in the build that
use `Encoding.Default`.

I updated two tests to reproduce this issue.
@jonpryor
Copy link
Member

jonpryor commented May 27, 2020

@jonpryor jonpryor merged commit 72bb668 into dotnet:master May 27, 2020
jonpryor pushed a commit that referenced this pull request May 27, 2020
)

Fixes: https://feedback.devdiv.io/955972

Context: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1084873
Context: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1130421

Android layout files with UTF-8 characters and custom views fail to
build with:

	error APT2258: not well-formed (invalid token).

In be47b3e, I inadvertently changed the `<ConvertCustomView/>`
MSBuild task to save changes using [`Encoding.Default`][0], which
defaults to the "ANSI" codepage on Windows, and thus cannot store
the entirety of Unicode characters.

`XDocumentExtensions.SaveIfChanged()` should instead use
`MonoAndroidHelper.UTF8withoutBOM` by default.

I updated two tests to reproduce this issue.

Review other uses of `MemoryStreamPool.Shared.CreateStreamWriter()`
and `Encoding.Default` in general to ensure that we're not using
`Encoding.Default` where we shouldn't be.

[0]: https://docs.microsoft.com/en-us/dotnet/api/system.text.encoding.default?view=netcore-3.1
@jonathanpeppers jonathanpeppers deleted the urf8-customview branch May 27, 2020 16:39
jonpryor pushed a commit that referenced this pull request May 28, 2020
)

Fixes: https://feedback.devdiv.io/955972

Context: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1084873
Context: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1130421

Android layout files with UTF-8 characters and custom views fail to
build with:

	error APT2258: not well-formed (invalid token).

In be47b3e, I inadvertently changed the `<ConvertCustomView/>`
MSBuild task to save changes using [`Encoding.Default`][0], which
defaults to the "ANSI" codepage on Windows, and thus cannot store
the entirety of Unicode characters.

`XDocumentExtensions.SaveIfChanged()` should instead use
`MonoAndroidHelper.UTF8withoutBOM` by default.

I updated two tests to reproduce this issue.

Review other uses of `MemoryStreamPool.Shared.CreateStreamWriter()`
and `Encoding.Default` in general to ensure that we're not using
`Encoding.Default` where we shouldn't be.

[0]: https://docs.microsoft.com/en-us/dotnet/api/system.text.encoding.default?view=netcore-3.1
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 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.

5 participants