-
Notifications
You must be signed in to change notification settings - Fork 533
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] Response file support for AOT #2062
Conversation
@jonpryor according to @akoeplinger |
b244a42
to
b53a5d8
Compare
b53a5d8
to
c75de5b
Compare
0e31d14
to
4363ba4
Compare
e57ae38
to
be371a9
Compare
949cf56
to
85c6ce4
Compare
Have we tested this code with paths containing spaces -- or $DEITY forbid -- ümlauts? |
42fb802
to
1a62e69
Compare
ok it looks like the
|
1a62e69
to
14a422c
Compare
Does AOT currently support ümlauts, without the response file? If so, we'll need to wait for a mono-side fix for encoding purposes and sanity. If not, we could remove the AOT+ümlauts test and merge, with the restriction that AOT only works with ASCII paths. |
@jonpryor ok so we currently handle non-ascii characters for AOT without any problems. So if we merged this it would be a breaking change for Windows. It seems to work without any problems on Mac :/ So it looks like this is not handling the case with unicode characters (on windows). I'm not sure how to fix that. |
ok some hopefully useful info
This then turns into the following after it has been read from the file. This is the result of a
|
Is this with
The above stackoverflow answer makes it sound like |
Its the result of the following code
So its just a normal |
Added mono/mono#13082 and mono/mono#13081 |
9db409c
to
3454de1
Compare
This PR has been updated to only add the Unit test. We are backing out the AOT response file changes until such time as mono properly processed the paths within the response file. I'll be raising an issue on mono as soon as I can get a good repo. |
453d8b4
to
bcab81d
Compare
bcab81d
to
cf191a6
Compare
cf191a6
to
46c16e6
Compare
Sigh, So Windows tests pass locally..
from the following path
and guess what. Its because GetShortPath is disabled on that E: drive it seems 🤦♂ |
|
||
string aotOptionsStr = (EnableLLVM ? "--llvm " : "") + "--aot=" + string.Join (",", aotOptions); | ||
string aotOptionsStr = (EnableLLVM ? "--llvm " : "") + "\"--aot=" + string.Join (",", aotOptions) + "\""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the important line. All of the options inside the --aot=
argument do not need quotes as long as the entire argument is wrapped in quotes. e.g
"--aot=foo=bar bar,doh,cat=true"
@@ -361,6 +357,10 @@ IEnumerable<Config> GetAotConfigs () | |||
if (!Directory.Exists (outdir)) | |||
Directory.CreateDirectory (outdir); | |||
|
|||
if (outdir.StartsWith (WorkingDirectory, StringComparison.InvariantCultureIgnoreCase)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bit is needed because some of the aot options still don't like spaces. However all of the files we create are in the IntermediateOuputPath. So we can strip off the WorkingDirectory
part of the path since it won't be need (as the process is started in the WorkingDirectory
.
The WorkingDirectory
is normally the root of the project where the csproj is, and that is the part which will generally contain spaces and non-ascii characters.
@@ -173,14 +174,6 @@ static string GetNdkToolchainLibraryDir (string binDir, AndroidTargetArch arch) | |||
return GetNdkToolchainLibraryDir (binDir, NdkUtil.GetArchDirName (arch)); | |||
} | |||
|
|||
static string GetShortPath (string path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bye bye :D
fc048c8
to
ec3d04b
Compare
Test failure is |
Our AOT system has issues when a user uses non ASCII characters and spaces on Windows. We use `GetShortPath` to get the old DOS 8.3 short names of paths to get around paths having spaces and unicode characters. However on the latest versions of windows `GetShortPath` seems to be supported only on the main system drive (C:). Many of our developers and CI will be building on other drives. So we really need to support paths with spaces and unicode. This commit reworks the way we provide the `--aot` pargument to the cross compilers so that it actually works in those senarios. The first thing was how the arguments were parsed. `mono` uses the build in system command line parser `GetCommandLineW` 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 --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" As you can see the parameters have been split where there is a space. The solution to this is to double quote the ENTIRE argument and remove any quotes within the parameter list like so "--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 paremters correctly as one block. There is another issue however. With the new argument line above if the `temp=` path has a space in it `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=` argument. So 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 still start hitting the command line length limit on windows. This is currently 246 characters. So depending on where a user creates the project we might end up with a long command line. To work around this issue we can make use of the new `--response=` argument in `mono`. 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. So the final call will be something 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 `BuildAotApplication` and `BuildAotApplicationAndBundle` unit tests to use paths with both spaces and non-ascii characters to make sure we support both of those senarios.
ec3d04b
to
ce5a293
Compare
Commit message for squash-and-merge:
|
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
Our AOT system has issues when a user uses
non ASCII characters and spaces on Windows.
We use
GetShortPath
to get the old DOS8.3 short names of paths to get around
paths having spaces and unicode characters.
However on the latest versions of windows
GetShortPath
seems to be supported onlyon the main system drive (C:). Many of our
developers and CI will be building on other
drives. So we really need to support paths
with spaces and unicode.
This commit reworks the way we provide the
--aot
pargument to the cross compilers sothat it actually works in those senarios.
The first thing was how the arguments were
parsed.
mono
uses the build in systemcommand line parser
GetCommandLineW
to parsearguments. Given the following
--aot
argumentThis ends up as the following arguments
As you can see the parameters have been split
where there is a space. The solution to this is
to double quote the ENTIRE argument and remove
any quotes within the parameter list like so
This allows windows (and mac) to parse the paremters
correctly as one block.
There is another issue however. With the new argument
line above if the
temp-path=
path has a space in itmono
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 withinthat directory structure.
WorkingDirectory
is set to thedirectory of the projects csproj file. So the following
will become
This will fix the issue with the
temp-path=
argument. So we end upwith something like this
However we might still start hitting the command line length limit
on windows. This is currently 246 characters. So depending on where
a user creates the project we might end up with a long command line.
To work around this issue we can make use of the new
--response=
argument in
mono
. Instead of passing all the arguments to thecross compiler we can instead write them to a file and pass the
path to that file as the
--response=
argument. This will reduceour command line length to be within an acceptable range unless
the user creates a project in a very very deep directory structure.
So the final call will be something like
Which works perfectly.
This commit also updates the
BuildAotApplication
andBuildAotApplicationAndBundle
unit tests to use paths withboth spaces and non-ascii characters to make sure we support
both of those senarios.