From 8923c11b04ef52bbf238955cefdb9995c5b5ed38 Mon Sep 17 00:00:00 2001 From: Dean Ellis Date: Wed, 4 Sep 2019 15:22:49 +0100 Subject: [PATCH] [Xamarin.Android.Build.Tasks] Response file support for AOT (#2062) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/609244 Fixes: https://developercommunity.visualstudio.com/content/problem/554407/vs2019-xamarinandroid-aot-command.html Fixes: https://github.com/xamarin/xamarin-android/issues/3407 Our AOT system has issues when a user uses non-ASCII characters and spaces on Windows. We used [`GetShortPathName()`][0] to get the old DOS 8.3 short names of paths to get around paths having spaces and unicode characters. However, short path name generation can be [disabled][1], in which case `GetShortPathName()` will return the long path name. Consequently, builds can fail when spaces appear in unexpected places: [aot-compiler stderr] Cannot open assembly 'Files': No such file or directory. Short path name generation can be disabled on a drive-by-drive basis, and our Azure DevOps build machines have short path name generation disabled on the `E:` drive used for builds. We really need to support paths with spaces and unicode characters. Rework the way we provide the `--aot` argument to the cross compilers so that it actually works in those scenarios. The first thing was how the arguments were parsed. `mono` uses the built-in system command line parser [`CommandLineToArgv()`][2] to parse arguments. Given the following `--aot` argument --aot=outfile="c:\Sandbox\repo\bin\BuildAotApplicationAndBundle AndÜmläüts_x86_True_True\obj\Release\libaot-Mono.Android.dll.so",asmwriter,mtriple=i686-linux-android,tool-prefix=C:\ProgramData\Microsoft\AndroidNDK\android-ndk-r13b\toolchains\x86-4.9\prebuilt\windows\bin\i686-linux-android-,ld-flags=C:\ProgramData\Microsoft\AndroidNDK\android-ndk-r13b\toolchains\x86-4.9\prebuilt\windows\lib\gcc\i686-linux-android\4.9.x\libgcc.a;C:\ProgramData\Microsoft\AndroidNDK\android-ndk-r13b\platforms\android-16\arch-x86\usr\lib\libc.so;C:\ProgramData\Microsoft\AndroidNDK\android-ndk-r13b\platforms\android-16\arch-x86\usr\lib\libm.so,temp-path="c:\Sandbox\repo\BuildAotApplicationAndBundle AndÜmläüts_x86_True_True\obj\Release\temp" This ends up as the following arguments, one per-line [0]: --aot=outfile="c:\Sandbox\repo\bin\BuildAotApplicationAndBundle [1]: AndÜmläüts_x86_True_True\obj\Release\libaot-Mono.Android.dll.so" [2]: ,asmwriter,mtriple=i686-linux-android,tool-prefix=C:\ProgramData\Microsoft\AndroidNDK\android-ndk-r13b\toolchains\x86-4.9\prebuilt\windows\bin\i686-linux-android-,ld-flags=C:\ProgramData\Microsoft\AndroidNDK\android-ndk-r13b\toolchains\x86-4.9\prebuilt\windows\lib\gcc\i686-linux-android\4.9.x\libgcc.a;C:\ProgramData\Microsoft\AndroidNDK\android-ndk-r13b\platforms\android-16\arch-x86\usr\lib\libc.so;C:\ProgramData\Microsoft\AndroidNDK\android-ndk-r13b\platforms\android-16\arch-x86\usr\lib\libm.so,temp-path="c:\Sandbox\repo\BuildAotApplicationAndBundle [3]: AndÜmläüts_x86_True_True\obj\Release\temp" As you can see the parameters have been split wherever there is a space. The solution to this is to double quote the ENTIRE argument and remove any quotes within the parameter list like: "--aot=outfile=c:\Sandbox\repo\bin\BuildAotApplicationAndBundle AndÜmläüts_x86_True_True\obj\Release\libaot-Mono.Android.dll.so,asmwriter,mtriple=i686-linux-android,tool-prefix=C:\ProgramData\Microsoft\AndroidNDK\android-ndk-r13b\toolchains\x86-4.9\prebuilt\windows\bin\i686-linux-android-,ld-flags=C:\ProgramData\Microsoft\AndroidNDK\android-ndk-r13b\toolchains\x86-4.9\prebuilt\windows\lib\gcc\i686-linux-android\4.9.x\libgcc.a;C:\ProgramData\Microsoft\AndroidNDK\android-ndk-r13b\platforms\android-16\arch-x86\usr\lib\libc.so;C:\ProgramData\Microsoft\AndroidNDK\android-ndk-r13b\platforms\android-16\arch-x86\usr\lib\libm.so,temp-path=c:\Sandbox\repo\BuildAotApplicationAndBundle AndÜmläüts_x86_True_True\obj\Release\temp" This allows Windows (and mac) to parse the parameters correctly as one value. There is another issue however. With the new argument line above, if the `temp-path=` path has a space in it then `mono` still has issues with that path. The good news is that we can use some domain knowledge to reduce not only the paths which need spaces but also the overall length of the argument. Because we know the cross compiler will be executed within `WorkingDirectory` we can shorten any path which is within that directory structure. `WorkingDirectory` is set to the directory of the projects `.csproj` file. So the following: E:\Some Project\My Project\obj\Release\aot\System.dll\ will become obj\Release\aot\System.dll\ This will fix the issue with the `temp-path=` argument. We end up with something like this "--aot=outfile=obj\Release\libaot-Mono.Android.dll.so,asmwriter,mtriple=i686-linux-android,tool-prefix=C:\ProgramData\Microsoft\AndroidNDK\android-ndk-r13b\toolchains\x86-4.9\prebuilt\windows\bin\i686-linux-android-,ld-flags=C:\ProgramData\Microsoft\AndroidNDK\android-ndk-r13b\toolchains\x86-4.9\prebuilt\windows\lib\gcc\i686-linux-android\4.9.x\libgcc.a;C:\ProgramData\Microsoft\AndroidNDK\android-ndk-r13b\platforms\android-16\arch-x86\usr\lib\libc.so;C:\ProgramData\Microsoft\AndroidNDK\android-ndk-r13b\platforms\android-16\arch-x86\usr\lib\libm.so,temp-path=obj\Release\temp" However we might also still start hitting the [command line length limit][3] on Windows. This is currently 8191 characters on Windows XP and later. So depending on where a user creates the project we might end up with a "too long" command line. To work around this issue we can make use of the new `mono --response=` argument. Instead of passing all the arguments to the cross compiler we can instead write them to a file and pass the path to that file as the `--response=` argument. This will reduce our command line length to be within an acceptable range unless the user creates a project in a very very deep directory structure. The final call will be like: ...\cross-arm --response="c:\Sandbox\repo\bin\BuildAotApplicationAndBundle AndÜmläüts_x86_True_True\obj\Release\Mono.Android.dll\response.txt" which works perfectly. This commit also updates the `BuildTest.BuildAotApplication()` and `BuildTest.BuildAotApplicationAndBundle()` unit tests to use paths with both spaces and non-ASCII characters to ensure we support both of those scenarios. [0]: https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getshortpathnamew [1]: https://support.microsoft.com/en-us/help/121007/how-to-disable-8-3-file-name-creation-on-ntfs-partitions [2]: https://docs.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-commandlinetoargvw [3]: https://support.microsoft.com/en-us/help/830473/command-prompt-cmd-exe-command-line-string-limitation --- src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs | 2 + .../Tasks/Aapt2.cs | 2 + src/Xamarin.Android.Build.Tasks/Tasks/Aot.cs | 71 +++++++++++-------- .../Xamarin.Android.Build.Tests/BuildTest.cs | 10 +-- 4 files changed, 50 insertions(+), 35 deletions(-) diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs b/src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs index 445c04f1a5b..26cf0411498 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs @@ -4,6 +4,7 @@ using System.Diagnostics; using System.IO; using System.Linq; +using System.Text; using System.Threading; using System.Xml; using System.Xml.Linq; @@ -113,6 +114,7 @@ bool RunAapt (string commandLine, IList output) UseShellExecute = false, RedirectStandardOutput = true, RedirectStandardError = true, + StandardOutputEncoding = Encoding.UTF8, CreateNoWindow = true, WindowStyle = ProcessWindowStyle.Hidden, WorkingDirectory = WorkingDirectory, diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/Aapt2.cs b/src/Xamarin.Android.Build.Tasks/Tasks/Aapt2.cs index 26e5a5a1c4d..889926d8987 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/Aapt2.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/Aapt2.cs @@ -7,6 +7,7 @@ using System.Threading; using System.Xml; using System.Xml.Linq; +using System.Text; using Microsoft.Build.Utilities; using Microsoft.Build.Framework; using System.Text.RegularExpressions; @@ -53,6 +54,7 @@ protected bool RunAapt (string commandLine, IList output) UseShellExecute = false, RedirectStandardOutput = true, RedirectStandardError = true, + StandardOutputEncoding = Encoding.UTF8, CreateNoWindow = true, WindowStyle = ProcessWindowStyle.Hidden, WorkingDirectory = WorkingDirectory, diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/Aot.cs b/src/Xamarin.Android.Build.Tasks/Tasks/Aot.cs index 96023b75a05..3a2ca9405cb 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/Aot.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/Aot.cs @@ -5,6 +5,7 @@ using System.IO; using System.Linq; using System.Reflection; +using System.Text; using System.Threading; using Microsoft.Build.Framework; using Microsoft.Build.Utilities; @@ -174,14 +175,6 @@ static string GetNdkToolchainLibraryDir (string binDir, AndroidTargetArch arch) return GetNdkToolchainLibraryDir (binDir, NdkUtil.GetArchDirName (arch)); } - static string GetShortPath (string path) - { - if (Environment.OSVersion.Platform != PlatformID.Win32NT) - return QuoteFileName (path); - var shortPath = KernelEx.GetShortPathName (Path.GetDirectoryName (path)); - return Path.Combine (shortPath, Path.GetFileName (path)); - } - static string QuoteFileName(string fileName) { var builder = new CommandLineBuilder(); @@ -283,12 +276,14 @@ bool RunParallelAotCompiler (List nativeLibs) return; } - if (!RunAotCompiler (config.AssembliesPath, config.AotCompiler, config.AotOptions, config.AssemblyPath)) { + if (!RunAotCompiler (config.AssembliesPath, config.AotCompiler, config.AotOptions, config.AssemblyPath, config.ResponseFile)) { LogCodedError ("XA3001", "Could not AOT the assembly: {0}", Path.GetFileName (config.AssemblyPath)); Cancel (); return; } + File.Delete (config.ResponseFile); + lock (nativeLibs) nativeLibs.Add (config.OutputFile); } @@ -362,6 +357,11 @@ IEnumerable GetAotConfigs () if (!Directory.Exists (outdir)) Directory.CreateDirectory (outdir); + // dont use a full path if the outdir is withing the WorkingDirectory. + if (outdir.StartsWith (WorkingDirectory, StringComparison.InvariantCultureIgnoreCase)) { + outdir = outdir.Replace (WorkingDirectory + Path.DirectorySeparatorChar, string.Empty); + } + int level = 0; string toolPrefix = EnableLLVM ? NdkUtil.GetNdkToolPrefix (AndroidNdkDirectory, arch, level = GetNdkApiLevel (AndroidNdkDirectory, AndroidApiLevel, arch)) @@ -389,19 +389,19 @@ IEnumerable GetAotConfigs () var libs = new List(); if (NdkUtil.UsingClangNDK) { - libs.Add ($"-L{GetShortPath (toolchainLibDir)}"); - libs.Add ($"-L{GetShortPath (androidLibPath)}"); + libs.Add ($"-L{toolchainLibDir}"); + libs.Add ($"-L{androidLibPath}"); if (arch == AndroidTargetArch.Arm) { // Needed for -lunwind to work string compilerLibDir = Path.Combine (toolchainPath, "..", "sysroot", "usr", "lib", NdkUtil.GetArchDirName (arch)); - libs.Add ($"-L{GetShortPath (compilerLibDir)}"); + libs.Add ($"-L{compilerLibDir}"); } } - libs.Add (GetShortPath (Path.Combine (toolchainLibDir, "libgcc.a"))); - libs.Add (GetShortPath (Path.Combine (androidLibPath, "libc.so"))); - libs.Add (GetShortPath (Path.Combine (androidLibPath, "libm.so"))); + libs.Add (Path.Combine (toolchainLibDir, "libgcc.a")); + libs.Add (Path.Combine (androidLibPath, "libc.so")); + libs.Add (Path.Combine (androidLibPath, "libm.so")); ldFlags = string.Join(";", libs); } @@ -423,25 +423,28 @@ IEnumerable GetAotConfigs () aotOptions.Add ("profile-only"); foreach (var p in Profiles) { var fp = Path.GetFullPath (p.ItemSpec); - aotOptions.Add ($"profile={GetShortPath (fp)}"); + aotOptions.Add ($"profile={fp}"); } } if (!string.IsNullOrEmpty (AotAdditionalArguments)) aotOptions.Add (AotAdditionalArguments); if (sequencePointsMode == SequencePointsMode.Offline) - aotOptions.Add ("msym-dir=" + GetShortPath (outdir)); + aotOptions.Add ($"msym-dir={outdir}"); if (AotMode != AotMode.Normal) aotOptions.Add (AotMode.ToString ().ToLowerInvariant ()); - aotOptions.Add ("outfile=" + GetShortPath (outputFile)); + aotOptions.Add ($"outfile={outputFile}"); aotOptions.Add ("asmwriter"); - aotOptions.Add ("mtriple=" + mtriple); - aotOptions.Add ("tool-prefix=" + GetShortPath (toolPrefix)); - aotOptions.Add ("ld-flags=" + ldFlags); - aotOptions.Add ("llvm-path=" + GetShortPath (sdkBinDirectory)); - aotOptions.Add ("temp-path=" + GetShortPath (tempDir)); + aotOptions.Add ($"mtriple={mtriple}"); + aotOptions.Add ($"tool-prefix={toolPrefix}"); + aotOptions.Add ($"ld-flags={ldFlags}"); + aotOptions.Add ($"llvm-path={sdkBinDirectory}"); + aotOptions.Add ($"temp-path={tempDir}"); - string aotOptionsStr = (EnableLLVM ? "--llvm " : "") + "--aot=" + string.Join (",", aotOptions); + // we need to quote the entire --aot arguments here to make sure it is parsed + // on windows as one argument. Otherwise it will be split up into multiple + // values, which wont work. + string aotOptionsStr = (EnableLLVM ? "--llvm " : "") + $"\"--aot={string.Join (",", aotOptions)}\""; if (!string.IsNullOrEmpty (ExtraAotOptions)) { aotOptionsStr += (aotOptions.Count > 0 ? "," : "") + ExtraAotOptions; @@ -461,23 +464,29 @@ IEnumerable GetAotConfigs () } var assembliesPath = Path.GetFullPath (Path.GetDirectoryName (resolvedPath)); - var assemblyPath = QuoteFileName (Path.GetFullPath (resolvedPath)); + var assemblyPath = Path.GetFullPath (resolvedPath); - yield return new Config (assembliesPath, QuoteFileName (aotCompiler), aotOptionsStr, assemblyPath, outputFile); + yield return new Config (assembliesPath, aotCompiler, aotOptionsStr, assemblyPath, outputFile, Path.Combine (tempDir, "response.txt")); } } } - bool RunAotCompiler (string assembliesPath, string aotCompiler, string aotOptions, string assembly) + bool RunAotCompiler (string assembliesPath, string aotCompiler, string aotOptions, string assembly, string responseFile) { var stdout_completed = new ManualResetEvent (false); var stderr_completed = new ManualResetEvent (false); + + using (var sw = new StreamWriter (responseFile, append: false, encoding: new UTF8Encoding (encoderShouldEmitUTF8Identifier: false))) { + sw.WriteLine (aotOptions + " " + QuoteFileName (assembly)); + } + var psi = new ProcessStartInfo () { - FileName = aotCompiler, - Arguments = aotOptions + " " + assembly, + FileName = QuoteFileName (aotCompiler), + Arguments = $"--response={QuoteFileName (responseFile)}", UseShellExecute = false, RedirectStandardOutput = true, RedirectStandardError = true, + StandardOutputEncoding = Encoding.UTF8, CreateNoWindow=true, WindowStyle=ProcessWindowStyle.Hidden, WorkingDirectory = WorkingDirectory, @@ -537,16 +546,18 @@ struct Config { public string AotOptions { get; } public string AssemblyPath { get; } public string OutputFile { get; } + public string ResponseFile { get; } public bool Valid { get; private set; } - public Config (string assembliesPath, string aotCompiler, string aotOptions, string assemblyPath, string outputFile) + public Config (string assembliesPath, string aotCompiler, string aotOptions, string assemblyPath, string outputFile, string responseFile) { AssembliesPath = assembliesPath; AotCompiler = aotCompiler; AotOptions = aotOptions; AssemblyPath = assemblyPath; OutputFile = outputFile; + ResponseFile = responseFile; Valid = true; } 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 9447731df22..12ab4048706 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 @@ -732,9 +732,9 @@ public void BuildMkBundleApplicationReleaseAllAbi () [Test] [TestCaseSource (nameof (AotChecks))] [Category ("Minor")] - public void BuildAotApplication (string supportedAbis, bool enableLLVM, bool expectedResult) + public void BuildAotApplicationAndÜmläüts (string supportedAbis, bool enableLLVM, bool expectedResult) { - var path = Path.Combine ("temp", string.Format ("BuildAotApplication_{0}_{1}_{2}", supportedAbis, enableLLVM, expectedResult)); + var path = Path.Combine ("temp", string.Format ("BuildAotApplication AndÜmläüts_{0}_{1}_{2}", supportedAbis, enableLLVM, expectedResult)); var proj = new XamarinAndroidApplicationProject () { IsRelease = true, BundleAssemblies = false, @@ -768,7 +768,7 @@ public void BuildAotApplication (string supportedAbis, bool enableLLVM, bool exp // LLVM passes a direct path to libc.so, and we need to use the libc.so // which corresponds to the *minimum* SDK version specified in AndroidManifest.xml // Since we overrode minSdkVersion=16, that means we should use libc.so from android-16. - var rightLibc = new Regex (@"^\s*\[AOT\].*cross-.*--llvm.*,ld-flags=.*android-16.arch-.*.usr.lib.libc\.so", RegexOptions.Multiline); + var rightLibc = new Regex (@"\s*\[aot-compiler stdout].*android-16.arch-.*.usr.lib.libc\.so", RegexOptions.Multiline); var m = rightLibc.Match (string.Join ("\n",b.LastBuildOutput)); Assert.IsTrue (m.Success, "AOT+LLVM should use libc.so from minSdkVersion!"); } @@ -803,9 +803,9 @@ public void BuildAotApplication (string supportedAbis, bool enableLLVM, bool exp [Test] [TestCaseSource (nameof (AotChecks))] [Category ("Minor")] - public void BuildAotApplicationAndBundle (string supportedAbis, bool enableLLVM, bool expectedResult) + public void BuildAotApplicationAndBundleAndÜmläüts (string supportedAbis, bool enableLLVM, bool expectedResult) { - var path = Path.Combine ("temp", string.Format ("BuildAotApplicationAndBundle_{0}_{1}_{2}", supportedAbis, enableLLVM, expectedResult)); + var path = Path.Combine ("temp", string.Format ("BuildAotApplicationAndBundle AndÜmläüts_{0}_{1}_{2}", supportedAbis, enableLLVM, expectedResult)); var proj = new XamarinAndroidApplicationProject () { IsRelease = true, BundleAssemblies = true,