Ignore --arch switch if it doesn't affect the build

This change has two consequences, one it can improve caching as
specifying `--arch=<default_arch>` will lead to the same build id as
not specifying it at all but, more importantly, it improves the
usability of external tools that rely on dub generated files.

Take as an example the meson build system. It supports for a project
to depend on a dub build library. The internal code calls `dub
describe --build=... --compiler=... --arch=...` and gathers the cache
artifact file from that. Because of always specifying `--arch` the
package will always need to be rebuilt when used by meson. This is
not intuitive to someone who doesn't know how the --arch switch is
used internally in dub.

This has come up as an issue in meson's CI system. The CI system is
setup to build test images to be used in a container, periodically,
which include all the needed dependencies and common files that are
shared across test runs to improve test times. For dub this implies
that dub packages that are needed during the tests are fetched + built
during the building of the CI images, not when running tests.

However, the packages were built with `dub build --compiler=... <pkg>`
which would log to stdout that it built <pkg> debug configuration with
<comp> on x86_64. During the tests meson would fail to find the built
packages because of the `--arch` switch it used internally and it
recommended that the package should be built with `--compiler=<comp>
--build=debug --arch=x86_64` witch is exactly what dub reported the
package was built with. This has lead to frustrating debugging and the
install scripts calling dub like `dub run --yes dub-build-deep --
<pkg> --arch=x86_64 --compiler=<comp> --build=debug` because nobody
can figure out what flags are actually needed.

Signed-off-by: Andrei Horodniceanu <[email protected]>

source/dub/compilers/compiler.d

@@ -171,20 +171,24 @@ interface Compiler {
 			format("%s failed with exit code %s.", args[0], status));
 	}
 
+	/** Default compiler arguments for performing a probe. They should
+		be the D compiler equivalent of "don't output executables"
+	*/
+	protected string[] defaultProbeArgs() const;
+
 	/** Compiles platform probe file with the specified compiler and parses its output.
 		Params:
 			compiler_binary =	binary to invoke compiler with
-			args			=	arguments for the probe compilation
-			arch_override	=	special handler for x86_mscoff
+			arch_flags      =   compiler specific flags derived from the user's arch override
 	*/
-	protected final BuildPlatform probePlatform(string compiler_binary, string[] args, string arch_override)
+	protected final BuildPlatform probePlatform(string compiler_binary, string[] arch_flags)
 	{
 		import dub.compilers.utils : generatePlatformProbeFile, readPlatformSDLProbe;
 		import std.string : format, strip;
 
-		NativePath fil = generatePlatformProbeFile();
+		immutable fileArg = generatePlatformProbeFile().toNativeString;
 
-		auto result = execute(compiler_binary ~ args ~ fil.toNativeString());
+		auto result = execute(compiler_binary ~ defaultProbeArgs ~ arch_flags ~ fileArg);
 		enforce!CompilerInvocationException(result.status == 0,
 				format("Failed to invoke the compiler %s to determine the build platform: %s",
 				compiler_binary, result.output));
@@ -200,22 +204,20 @@ interface Compiler {
 			build_platform.compilerVersion = ver;
 		}
 
-		// Skip the following check for LDC, emitting a warning if the specified `-arch`
-		// cmdline option does not lead to the same string being found among
-		// `build_platform.architecture`, as it's brittle and doesn't work with triples.
-		if (build_platform.compiler != "ldc") {
-			if (arch_override.length && !build_platform.architecture.canFind(arch_override) &&
-				!(build_platform.compiler == "dmd" && arch_override.among("x86_omf", "x86_mscoff")) // Will be fixed in determinePlatform
-			) {
-				logWarn(`Failed to apply the selected architecture %s. Got %s.`,
-					arch_override, build_platform.architecture);
-			}
-		}
-
 		return build_platform;
 	}
+
 }
 
 private {
 	Compiler[] s_compilers;
 }
+
+/// Adds the given flags to the build settings if desired, otherwise informs the user
+package void maybeAddArchFlags(ref BuildSettings settings, bool keep_arch, string[] arch_flags, string arch_override) {
+	if (keep_arch)
+		settings.addDFlags(arch_flags);
+	else if (arch_override.length) {
+		logDebug("Ignoring arch_override '%s' for better caching because it doesn't affect the build", arch_override);
+	}
+}

source/dub/compilers/dmd.d

@@ -110,34 +110,37 @@ config    /etc/dmd.conf
 	{
 		// Set basic arch flags for the probe - might be revised based on the exact value + compiler version
 		string[] arch_flags;
-		if (arch_override.length)
-			arch_flags = [ arch_override != "x86_64" ? "-m32" : "-m64" ];
-		else
-		{
-			// Don't use Optlink by default on Windows
-			version (Windows) {
-				const is64bit = isWow64();
-				if (!is64bit.isNull)
-					arch_flags = [ is64bit.get ? "-m64" : "-m32" ];
-			}
-		}
-
-		BuildPlatform bp = probePlatform(
-			compiler_binary,
-			arch_flags ~ ["-quiet", "-c", "-o-", "-v"],
-			arch_override
-		);
-
 		switch (arch_override) {
 			default: throw new UnsupportedArchitectureException(arch_override);
-			case "": break;
+			case "":
+				// Don't use Optlink by default on Windows
+				version (Windows) {
+					const is64bit = isWow64();
+					if (!is64bit.isNull)
+						arch_flags = [ is64bit.get ? "-m64" : "-m32" ];
+				}
+				break;
 			// DMD 2.099  made MsCOFF the default, and DMD v2.109 removed OMF
 			// support. Default everything to MsCOFF, people wanting to use OMF
 			// should use an older DMD / dub.
 			case "x86", "x86_omf", "x86_mscoff": arch_flags = ["-m32"]; break;
 			case "x86_64": arch_flags = ["-m64"]; break;
 		}
-		settings.addDFlags(arch_flags);
+
+		auto bp = probePlatform(compiler_binary, arch_flags);
+
+		bool keep_arch;
+		if (arch_flags.length)
+			keep_arch = bp.architecture != probePlatform(compiler_binary, []).architecture;
+		settings.maybeAddArchFlags(keep_arch, arch_flags, arch_override);
+
+		if (arch_override.length
+			&& !bp.architecture.canFind(arch_override)
+			&& !arch_override.among("x86_omf", "x86_mscoff")
+			) {
+			logWarn(`Failed to apply the selected architecture %s. Got %s.`,
+					arch_override, bp.architecture);
+		}
 
 		return bp;
 	}
@@ -396,4 +399,8 @@ config    /etc/dmd.conf
 				    || arg.startsWith("-defaultlib=");
 		}
 	}
+
+	protected string[] defaultProbeArgs () const {
+		return ["-quiet", "-c", "-o-", "-v"];
+	}
 }

source/dub/compilers/gdc.d

@@ -76,13 +76,15 @@ class GDCCompiler : Compiler {
 			case "x86": arch_flags = ["-m32"]; break;
 			case "x86_64": arch_flags = ["-m64"]; break;
 		}
-		settings.addDFlags(arch_flags);
 
-		return probePlatform(
-			compiler_binary,
-			arch_flags ~ ["-fsyntax-only", "-v"],
-			arch_override
-		);
+		auto bp = probePlatform(compiler_binary, arch_flags);
+
+		bool keep_arch;
+		if (arch_flags.length)
+			keep_arch = bp.architecture != probePlatform(compiler_binary, []).architecture;
+		settings.maybeAddArchFlags(keep_arch, arch_flags, arch_override);
+
+		return bp;
 	}
 
 	void prepareBuildSettings(ref BuildSettings settings, const scope ref BuildPlatform platform, BuildSetting fields = BuildSetting.all) const
@@ -266,6 +268,10 @@ class GDCCompiler : Compiler {
 
 		return  dflags;
 	}
+
+	protected string[] defaultProbeArgs () const {
+		return ["-fsyntax-only", "-v"];
+	}
 }
 
 private string extractTarget(const string[] args) { auto i = args.countUntil("-o"); return i >= 0 ? args[i+1] : null; }

source/dub/compilers/ldc.d

@@ -79,6 +79,7 @@ config    /etc/ldc2.conf (x86_64-pc-linux-gnu)
 	BuildPlatform determinePlatform(ref BuildSettings settings, string compiler_binary, string arch_override)
 	{
 		string[] arch_flags;
+		bool arch_override_is_triple = false;
 		switch (arch_override) {
 			case "": break;
 			case "x86": arch_flags = ["-march=x86"]; break;
@@ -87,19 +88,22 @@ config    /etc/ldc2.conf (x86_64-pc-linux-gnu)
 			case "aarch64": arch_flags = ["-march=aarch64"]; break;
 			case "powerpc64": arch_flags = ["-march=powerpc64"]; break;
 			default:
-				if (arch_override.canFind('-'))
+				if (arch_override.canFind('-')) {
+					arch_override_is_triple = true;
 					arch_flags = ["-mtriple="~arch_override];
-				else
+				} else
 					throw new UnsupportedArchitectureException(arch_override);
 				break;
 		}
-		settings.addDFlags(arch_flags);
 
-		return probePlatform(
-			compiler_binary,
-			arch_flags ~ ["-c", "-o-", "-v"],
-			arch_override
-		);
+		auto bp = probePlatform(compiler_binary, arch_flags);
+
+		bool keep_arch = arch_override_is_triple;
+		if (!keep_arch && arch_flags.length)
+			keep_arch = bp.architecture != probePlatform(compiler_binary, []).architecture;
+		settings.maybeAddArchFlags(keep_arch, arch_flags, arch_override);
+
+		return bp;
 	}
 
 	void prepareBuildSettings(ref BuildSettings settings, const scope ref BuildPlatform platform, BuildSetting fields = BuildSetting.all) const
@@ -335,4 +339,8 @@ config    /etc/ldc2.conf (x86_64-pc-linux-gnu)
 				    || arg.startsWith("-mtriple=");
 		}
 	}
+
+	protected string[] defaultProbeArgs () const {
+		return ["-c", "-o-", "-v"];
+	}
 }

test/ignore-useless-arch-switch/dub.sdl

@@ -0,0 +1,2 @@
+name "ignore-useless-arch-switch"
+targetType "executable"

test/ignore-useless-arch-switch/source/app.d

@@ -0,0 +1,40 @@
+import std.json;
+import std.path;
+import std.process;
+import std.stdio;
+
+string getCacheFile (in string[] program) {
+	auto p = execute(program);
+	with (p) {
+		if (status != 0) {
+			assert(false, "Failed to invoke dub describe: " ~ output);
+		}
+		return output.parseJSON["targets"][0]["cacheArtifactPath"].str;
+	}
+}
+
+void main()
+{
+	version (X86_64)
+		string archArg = "x86_64";
+	else version (X86)
+		string archArg = "x86";
+	else {
+		string archArg;
+		writeln("Skipping because of unsupported architecture");
+		return;
+	}
+
+	const describeProgram = [
+		environment["DUB"],
+		"describe",
+		"--compiler=" ~ environment["DC"],
+		"--root=" ~ __FILE_FULL_PATH__.dirName.dirName,
+	];
+	immutable plainCacheFile = describeProgram.getCacheFile;
+
+	const describeWithArch = describeProgram ~ [ "--arch=" ~ archArg ];
+	immutable archCacheFile = describeWithArch.getCacheFile;
+
+	assert(plainCacheFile == archCacheFile, "--arch shouldn't have modified the cache file");
+}