From 87b5beed56a46db8b6193f72054ba99163a38969 Mon Sep 17 00:00:00 2001 From: Andrei Horodniceanu Date: Thu, 19 Sep 2024 18:44:34 +0300 Subject: [PATCH] Ignore --arch switch if it doesn't affect the build This change has two consequences, one it can improve caching as specifying `--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=... ` which would log to stdout that it built debug configuration with 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= --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 -- --arch=x86_64 --compiler= --build=debug` because nobody can figure out what flags are actually needed. Signed-off-by: Andrei Horodniceanu --- source/dub/compilers/compiler.d | 36 ++++++++------- source/dub/compilers/dmd.d | 47 +++++++++++--------- source/dub/compilers/gdc.d | 18 +++++--- source/dub/compilers/ldc.d | 24 ++++++---- test/ignore-useless-arch-switch/.no_test | 0 test/ignore-useless-arch-switch/dub.sdl | 2 + test/ignore-useless-arch-switch/source/app.d | 40 +++++++++++++++++ 7 files changed, 116 insertions(+), 51 deletions(-) create mode 100644 test/ignore-useless-arch-switch/.no_test create mode 100644 test/ignore-useless-arch-switch/dub.sdl create mode 100644 test/ignore-useless-arch-switch/source/app.d diff --git a/source/dub/compilers/compiler.d b/source/dub/compilers/compiler.d index 4dba3dbb9..5a15b333a 100644 --- a/source/dub/compilers/compiler.d +++ b/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); + } +} diff --git a/source/dub/compilers/dmd.d b/source/dub/compilers/dmd.d index 587079d01..ff7f64a51 100644 --- a/source/dub/compilers/dmd.d +++ b/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"]; + } } diff --git a/source/dub/compilers/gdc.d b/source/dub/compilers/gdc.d index 3df8eda6b..b9f6c98a4 100644 --- a/source/dub/compilers/gdc.d +++ b/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; } diff --git a/source/dub/compilers/ldc.d b/source/dub/compilers/ldc.d index 0ca58c29e..eeab904e9 100644 --- a/source/dub/compilers/ldc.d +++ b/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"]; + } } diff --git a/test/ignore-useless-arch-switch/.no_test b/test/ignore-useless-arch-switch/.no_test new file mode 100644 index 000000000..e69de29bb diff --git a/test/ignore-useless-arch-switch/dub.sdl b/test/ignore-useless-arch-switch/dub.sdl new file mode 100644 index 000000000..919a41f95 --- /dev/null +++ b/test/ignore-useless-arch-switch/dub.sdl @@ -0,0 +1,2 @@ +name "ignore-useless-arch-switch" +targetType "executable" diff --git a/test/ignore-useless-arch-switch/source/app.d b/test/ignore-useless-arch-switch/source/app.d new file mode 100644 index 000000000..b48449c7d --- /dev/null +++ b/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"); +}