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

the-horo · 2024-08-22T05:21:04Z

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.

github-actions · 2024-08-22T05:21:16Z

✅ PR OK, no changes in deprecations or warnings

Total deprecations: 8

Total warnings: 0

Build statistics:

 statistics (-before, +after)
-executable size=5293368 bin/dub
-rough build time=64s
+executable size=5297464 bin/dub
+rough build time=62s

Full build output

DUB version 1.38.0, built on Jul  4 2024
LDC - the LLVM D compiler (1.39.0):
  based on DMD v2.109.1 and LLVM 18.1.6
  built with LDC - the LLVM D compiler (1.39.0)
  Default target: x86_64-unknown-linux-gnu
  Host CPU: znver3
  http://dlang.org - http://wiki.dlang.org/LDC


  Registered Targets:
    aarch64     - AArch64 (little endian)
    aarch64_32  - AArch64 (little endian ILP32)
    aarch64_be  - AArch64 (big endian)
    amdgcn      - AMD GCN GPUs
    arm         - ARM
    arm64       - ARM64 (little endian)
    arm64_32    - ARM64 (little endian ILP32)
    armeb       - ARM (big endian)
    avr         - Atmel AVR Microcontroller
    bpf         - BPF (host endian)
    bpfeb       - BPF (big endian)
    bpfel       - BPF (little endian)
    hexagon     - Hexagon
    lanai       - Lanai
    loongarch32 - 32-bit LoongArch
    loongarch64 - 64-bit LoongArch
    mips        - MIPS (32-bit big endian)
    mips64      - MIPS (64-bit big endian)
    mips64el    - MIPS (64-bit little endian)
    mipsel      - MIPS (32-bit little endian)
    msp430      - MSP430 [experimental]
    nvptx       - NVIDIA PTX 32-bit
    nvptx64     - NVIDIA PTX 64-bit
    ppc32       - PowerPC 32
    ppc32le     - PowerPC 32 LE
    ppc64       - PowerPC 64
    ppc64le     - PowerPC 64 LE
    r600        - AMD GPUs HD2XXX-HD6XXX
    riscv32     - 32-bit RISC-V
    riscv64     - 64-bit RISC-V
    sparc       - Sparc
    sparcel     - Sparc LE
    sparcv9     - Sparc V9
    spirv       - SPIR-V Logical
    spirv32     - SPIR-V 32-bit
    spirv64     - SPIR-V 64-bit
    systemz     - SystemZ
    thumb       - Thumb
    thumbeb     - Thumb (big endian)
    ve          - VE
    wasm32      - WebAssembly 32-bit
    wasm64      - WebAssembly 64-bit
    x86         - 32-bit X86: Pentium-Pro and above
    x86-64      - 64-bit X86: EM64T and AMD64
    xcore       - XCore
   Upgrading project in /home/runner/work/dub/dub/
    Starting Performing "release" build using /opt/hostedtoolcache/dc/ldc2-1.39.0/x64/ldc2-1.39.0-linux-x86_64/bin/ldc2 for x86_64.
    Building dub 1.39.0-beta.1+commit.11.g2f5d1a17: building configuration [application]
source/dub/internal/dyaml/composer.d(210,43): Deprecation: cannot access overlapped field `Event.implicit` with unsafe bit patterns in `@safe` code
source/dub/internal/dyaml/composer.d(232,43): Deprecation: cannot access overlapped field `Event.implicit` with unsafe bit patterns in `@safe` code
source/dub/internal/dyaml/composer.d(336,43): Deprecation: cannot access overlapped field `Event.implicit` with unsafe bit patterns in `@safe` code
source/dub/internal/dyaml/event.d(196,5): Deprecation: cannot access overlapped field `Event.explicitDocument` with unsafe bit patterns in `@safe` code
source/dub/internal/dyaml/event.d(214,5): Deprecation: cannot access overlapped field `Event.explicitDocument` with unsafe bit patterns in `@safe` code
source/dub/internal/dyaml/event.d(241,5): Deprecation: cannot access overlapped field `Event.implicit` with unsafe bit patterns in `@safe` code
source/dub/internal/dyaml/event.d(148,5): Deprecation: cannot access overlapped field `Event.implicit` with unsafe bit patterns in `@safe` code
source/dub/internal/dyaml/event.d(148,5): Deprecation: cannot access overlapped field `Event.implicit` with unsafe bit patterns in `@safe` code
     Linking dub
STAT:statistics (-before, +after)
STAT:executable size=5297464 bin/dub
STAT:rough build time=62s

the-horo · 2024-08-22T05:23:01Z

Example of meson changes caused by this issue: https://github.com/mesonbuild/meson/pull/13550/files#diff-a0eb8ae9cfbf214701a150d556ecba59c3776cff6c5ba9c1f712a2d6a6616443

Geod24

Thanks for doing this. It is indeed a problem if dub behave differently when an --arch that matches the default is passed as opposed to not provided. I made a few comments about the approach but the underlying goal is 💯

Geod24 · 2024-09-18T12:36:56Z

source/dub/compilers/compiler.d

@@ -212,6 +214,23 @@ interface Compiler {
 			}
 		}

+		if (arch_flags.length) {
+			auto result_without_arch = execute(compiler_binary ~ args ~ fileArg);
+			with (result_without_arch) {


Please don't use with. We can just shorten the name here (res_noarch?)

Geod24 · 2024-09-18T12:45:34Z

source/dub/compilers/compiler.d

 	*/
-	protected final BuildPlatform probePlatform(string compiler_binary, string[] args, string arch_override)
+	protected final BuildPlatform probePlatform(string compiler_binary, string[] args, string[] arch_flags, string arch_override, out bool arch_override_does_something)


I think we should look into simplifying this. Having 3 parameters around arch is a red flag.

Let me think about this for a little bit, enough time has passed since I first wrote this and I changed my mind on some implementations details. I'll make the changes that I want and see if I can reduce the number of flags here in order not to have a garbage API.

Sorry for the delay - I can also try to take a look. At the end of the day, if this solves a problem you have, I would be happy to go ahead with it to avoid another great delay.

The only thing that I care for changing is modifying the algorithm to preserve -arch x86_64-unknown-linux-gnu like flags, so fully specifying the target, but try to strip flags like -arch x86_64 since those just map to -m64. I'm just a little worried that stripping the triple will lead to some obscure errors on weird setups but stripping -m32 and whatnot should always be safe.

I'm not sure I'll get it done by the end of the day and I'm not the one affected by this issue so I also don't care that much for getting this merged ASAP.

So I went with another approach. To determine if the platform changes I just call probePlatform twice in each compiler's determinePlatform.

I've also taken the warning about the architecture not applying from probePlatform and put it into dmd.d since it seems to be mostly relevant for x86_mscoff vs x86_omf, which gdc doesn't suffer from. This, along side moving the default probe compiler flags (-o- -v -c) into a separate function, reduced the number of parameters to probePlatform to only two: the compiler and the arch flags.

Like I've said that I wanted now full triples like x86_64-unknown-linux-gnu are not stripped.

The new code looks better, my only concern is calling probePlatform twice, do you know how much time that would add to the build time ? I assume the platform file is fairly easily parsed / analyzed but I never actually measured how much it takes on my system.

Did a quick check on my system, got min/max/med of 7/11/10 with LDC

The new code looks better, my only concern is calling probePlatform twice, do you know how much time that would add to the build time ? I assume the platform file is fairly easily parsed / analyzed but I never actually measured how much it takes on my system.

Using a stopwatch to time determinePlatform on my system gives, roughly:

compiler Without --arch With --arch

dmd 3.5ms 6.5ms

gdc 9ms 17ms

ldc 15ms 28ms

The old code also invoked the compiler twice to determine if the arch changed, it just did that in the probePlatform function in order to reuse the probe file. Timing that probe file generation gives me 0.038ms so, since it keeps the code cleaner I think it's better to simply recall probePlatform. I don't think we have a way to check if the architecture changes without reinvoking the compiler, maybe there's a way to do it simply by looking at compiler -v and guessing the default architecture from that but it sounds very brittle.

the-horo · 2024-09-22T12:58:57Z

source/dub/compilers/compiler.d

 		return build_platform;
 	}
+
+	/// Adds the given flags to the build settings if desired, otherwise informs the user
+	final void maybeAddArchFlags(bool keep_arch, ref BuildSettings settings, string[] arch_flags, string arch_override) {


Maybe this can become protected static instead of public final?

Or move it outside the class:

package void maybeAddArchFlags(ref BuildSettings settings, bool keep_arch, string[] arch_flags, string arch_override)

Reordering args to make it UFCS-friendly.

Geod24 · 2024-09-23T10:30:44Z

@the-horo : This is ready to be merged. LMK if you want to act on #2962 (comment) and maybe rebase on master.

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]>

the-horo · 2024-09-23T11:39:07Z

@the-horo : This is ready to be merged. LMK if you want to act on #2962 (comment) and maybe rebase on master.

Rebased and made the change

Geod24

Thanks!

Geod24 · 2024-09-24T10:46:22Z

BTW @the-horo any way I can reach you ? Are you on the Dlang Slack / an email I can use ?

the-horo · 2024-09-24T12:09:15Z

I'm not on, what I assume is, https://dlang.slack.com but I wouldn't mind joining. Email's fine otherwise, it's the same as the one I sign-off my commits with: [email protected]. Pings on github also work.

the-horo mentioned this pull request Aug 22, 2024

dependencies/dub: First try to describe local project mesonbuild/meson#13555

Open

Geod24 reviewed Sep 18, 2024

View reviewed changes

the-horo force-pushed the ignore-useless-arch branch 2 times, most recently from ff90dc1 to f0f8047 Compare September 19, 2024 15:59

the-horo commented Sep 22, 2024

View reviewed changes

the-horo force-pushed the ignore-useless-arch branch from f0f8047 to 5016ec5 Compare September 23, 2024 11:37

the-horo force-pushed the ignore-useless-arch branch from 5016ec5 to af5687e Compare September 23, 2024 11:38

Geod24 enabled auto-merge (rebase) September 23, 2024 12:09

Geod24 approved these changes Sep 23, 2024

View reviewed changes

Geod24 merged commit 87b5bee into dlang:master Sep 23, 2024
30 of 31 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

the-horo commented Aug 22, 2024

github-actions bot commented Aug 22, 2024 •

edited

Loading

the-horo commented Aug 22, 2024

Geod24 left a comment

Geod24 Sep 18, 2024

Geod24 Sep 18, 2024

the-horo Sep 18, 2024

Geod24 Sep 18, 2024

the-horo Sep 18, 2024

the-horo Sep 19, 2024

Geod24 Sep 22, 2024

Geod24 Sep 22, 2024 •

edited

Loading

the-horo Sep 22, 2024

the-horo Sep 22, 2024

Geod24 Sep 23, 2024

Geod24 commented Sep 23, 2024

the-horo commented Sep 23, 2024

Geod24 left a comment

Geod24 commented Sep 24, 2024

the-horo commented Sep 24, 2024

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

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

Conversation

the-horo commented Aug 22, 2024

github-actions bot commented Aug 22, 2024 • edited Loading

the-horo commented Aug 22, 2024

Geod24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Geod24 Sep 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Geod24 commented Sep 23, 2024

the-horo commented Sep 23, 2024

Geod24 left a comment

Choose a reason for hiding this comment

Geod24 commented Sep 24, 2024

the-horo commented Sep 24, 2024

github-actions bot commented Aug 22, 2024 •

edited

Loading

Geod24 Sep 22, 2024 •

edited

Loading