Skip to content

Commit 1441c63

Browse files
committed
[WIP] Don't use quoting
Execute clang on Windows directly for NDK r19+, avoids trouble with having to quote spaces in the path so that NDK .cmd scripts don't fail.
1 parent f6722e5 commit 1441c63

File tree

3 files changed

+28
-16
lines changed

3 files changed

+28
-16
lines changed

src/Xamarin.Android.Build.Tasks/Utilities/NdkTools/NdkTools.cs

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -216,16 +216,18 @@ protected virtual string GetPlatformLibPath (AndroidTargetArch arch, int apiLeve
216216
{
217217
string? executablePath = null;
218218
if (IsWindows) {
219-
string toolDir = Path.GetDirectoryName (toolPath);
220-
executablePath = Path.Combine (toolDir, MonoAndroidHelper.GetExecutablePath (toolDir, Path.GetFileName (toolPath)));
221-
222-
if (String.Compare (Path.GetExtension (executablePath), ".cmd", StringComparison.Ordinal) == 0 && executablePath.IndexOf (' ') >= 0) {
223-
// NDK has CMD scripts to execute the compilers on Windows, but the scripts do not quote spaces in the command path properly, we need to do
224-
// it for them here or e.g. mkbundle will fail:
225-
//
226-
// [CC] "C:\a\_work\1\s\bin\TestRelease\temp\SDK Ümläüts\ndk\toolchains\llvm\prebuilt\windows-x86_64\bin\armv7a-linux-androideabi16-clang.CMD" -c -D__ANDROID_API__=16 -DANDROID -o obj\Release\bundles\armeabi-v7a\temp.o -I "C:\a\_work\1\s\bin\TestRelease\temp\SDK Ümläüts\ndk\sysroot\usr\include\arm-linux-androideabi" -I "C:\a\_work\1\s\bin\TestRelease\temp\SDK Ümläüts\ndk\sysroot\usr\include" obj\Release\bundles\armeabi-v7a\temp.c (TaskId:181)
227-
// [cc stderr] 'C:\a\_work\1\s\bin\TestRelease\temp\SDK' is not recognized as an internal or external command, (TaskId:181)
228-
executablePath = executablePath.Replace (" ", "\\ ");
219+
// We can't just use `File.Exists (toolPath)` since the Windows NDK contains extension-less
220+
// Unix shell scripts which would fail to work if an attempt to execute them would be made.
221+
//
222+
// Also, the NDK r19+ workaround (see NdkToolsWithClangWithPlatforms.ctor) will cause `toolPath`
223+
// here to end with .exe when looking for the compiler and we can save some time by not letting
224+
// `MonoAndroidHelper.GetExecutablePath` iterate over all %PATHEXT% extensions only to return the
225+
// original tool name
226+
if (Path.HasExtension (toolPath) && File.Exists (toolPath)) {
227+
executablePath = toolPath;
228+
} else {
229+
string toolDir = Path.GetDirectoryName (toolPath);
230+
executablePath = Path.Combine (toolDir, MonoAndroidHelper.GetExecutablePath (toolDir, Path.GetFileName (toolPath)));
229231
}
230232
} else if (File.Exists (toolPath)) {
231233
executablePath = toolPath;

src/Xamarin.Android.Build.Tasks/Utilities/NdkTools/WithClang.cs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@ abstract class NdkToolsWithClang : NdkTools
1010
{
1111
protected string UnifiedHeadersDirPath { get; }
1212

13+
// See NdkToolsWithClangWithPlatforms.ctor for explanation
14+
protected bool NeedClangWorkaround { get; set; }
15+
1316
protected NdkToolsWithClang (string androidNdkPath, NdkVersion version, TaskLoggingHelper? log)
1417
: base (androidNdkPath, version, log)
1518
{
@@ -46,7 +49,10 @@ public override string GetToolPath (NdkToolKind kind, AndroidTargetArch arch, in
4649
string toolName = GetToolName (kind);
4750

4851
if (kind == NdkToolKind.CompilerC || kind == NdkToolKind.CompilerCPlusPlus) {
49-
toolName = $"{GetCompilerTriple (arch)}{apiLevel}-{toolName}";
52+
if (!NeedClangWorkaround) {
53+
// See NdkToolsWithClangWithPlatforms.ctor for explanation
54+
toolName = $"{GetCompilerTriple (arch)}{apiLevel}-{toolName}";
55+
}
5056
} else {
5157
toolName = $"{GetArchTriple (arch)}-{toolName}";
5258
}

src/Xamarin.Android.Build.Tasks/Utilities/NdkTools/WithClangWithPlatforms.cs

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,11 @@ namespace Xamarin.Android.Tasks
77
{
88
class NdkToolsWithClangWithPlatforms : NdkToolsWithClang
99
{
10-
bool needClangWorkaround;
11-
1210
public NdkToolsWithClangWithPlatforms (string androidNdkPath, NdkVersion version, TaskLoggingHelper? log)
1311
: base (androidNdkPath, version, log)
1412
{
15-
needClangWorkaround = IsWindows && Version.Main.Major == 19 && Version.Main.Minor < 2;
16-
if (needClangWorkaround) {
13+
NeedClangWorkaround = IsWindows && Version.Main.Major >= 19;
14+
if (NeedClangWorkaround) {
1715
//
1816
// NDK r19 bug (fixed in r19c).
1917
//
@@ -38,13 +36,19 @@ public NdkToolsWithClangWithPlatforms (string androidNdkPath, NdkVersion version
3836
//
3937
// The code below tries to rectify the situation by special-casing the compiler tool handling to
4038
// return path to the actual .exe instead of the CMD.
39+
//
40+
// Despite the above issue having been fixed in NDK r19c, the CMD scripts in question have another issue -
41+
// they don't work well with paths that have spaces in them. That means we either need to quote the paths
42+
// ourselves, or invoke the compilers directly. The latter option seems to be the better one.
43+
//
4144
NdkToolNames[NdkToolKind.CompilerC] = "clang.exe";
45+
NdkToolNames[NdkToolKind.CompilerCPlusPlus] = "clang++.exe";
4246
}
4347
}
4448

4549
public override string GetCompilerTargetParameters (AndroidTargetArch arch, int apiLevel, bool forCPlusPlus = false)
4650
{
47-
if (!needClangWorkaround) {
51+
if (!NeedClangWorkaround) {
4852
return base.GetCompilerTargetParameters (arch, apiLevel, forCPlusPlus);
4953
}
5054

0 commit comments

Comments
 (0)