Skip to content

Commit

Permalink
[Xamarin.Android.Build.Tasks] Response file support for AOT (#2062)
Browse files Browse the repository at this point in the history 
Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/609244
Fixes: https://developercommunity.visualstudio.com/content/problem/554407/vs2019-xamarinandroid-aot-command.html
Fixes: #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
  • Loading branch information
@dellis1972 @jonpryor
dellis1972 authored and jonpryor committed Sep 4, 2019
1 parent 97f345a commit 8923c11
Show file tree
Hide file tree
Showing 4 changed files with 50 additions and 35 deletions.
2 changes: 2 additions & 0 deletions src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -113,6 +114,7 @@ bool RunAapt (string commandLine, IList<OutputLine> output)
UseShellExecute = false,
RedirectStandardOutput = true,
RedirectStandardError = true,
StandardOutputEncoding = Encoding.UTF8,
CreateNoWindow = true,
WindowStyle = ProcessWindowStyle.Hidden,
WorkingDirectory = WorkingDirectory,
Expand Down
2 changes: 2 additions & 0 deletions src/Xamarin.Android.Build.Tasks/Tasks/Aapt2.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -53,6 +54,7 @@ protected bool RunAapt (string commandLine, IList<OutputLine> output)
UseShellExecute = false,
RedirectStandardOutput = true,
RedirectStandardError = true,
StandardOutputEncoding = Encoding.UTF8,
CreateNoWindow = true,
WindowStyle = ProcessWindowStyle.Hidden,
WorkingDirectory = WorkingDirectory,
Expand Down
71 changes: 41 additions & 30 deletions src/Xamarin.Android.Build.Tasks/Tasks/Aot.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -283,12 +276,14 @@ bool RunParallelAotCompiler (List<string> 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);
}
Expand Down Expand Up @@ -362,6 +357,11 @@ IEnumerable<Config> 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))
Expand Down Expand Up @@ -389,19 +389,19 @@ IEnumerable<Config> GetAotConfigs ()

var libs = new List<string>();
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);
}
Expand All @@ -423,25 +423,28 @@ IEnumerable<Config> 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;
Expand All @@ -461,23 +464,29 @@ IEnumerable<Config> 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,
Expand Down Expand Up @@ -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;
}

Expand Down
10 changes: 5 additions & 5 deletions src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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!");
}
Expand Down Expand Up @@ -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,
Expand Down

0 comments on commit 8923c11

Please sign in to comment.