Skip to content
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

Let user $DFLAGS override build settings as much as possible #2796

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

the-horo
Copy link
Contributor

This change is mostly about being able to build programs without -Werror or -w.

dub setting up so that warnings are errors by default can be nice for developers making their code more correct but it is useless for consumers. Currently, there is no way to disable that without modifying the project configuration to add allowWarnings to buildRequirements. Users should, at least, be able to add -wi to DFLAGS and have the preference respected.

This isn't so much a problem with dmd or ldc in my experience as they mostly report deprecations, the problem is with gdc where every deprecation is a warning and the -Werror makes builds fail where they otherwise should have succeeded with only some warnings.

Signed-off-by: Andrei Horodniceanu <a.horodniceanu@proton.me>
Copy link

github-actions bot commented Jan 13, 2024

✅ PR OK, no changes in deprecations or warnings

Total deprecations: 0

Total warnings: 0

Build statistics:

 statistics (-before, +after)
-executable size=5228376 bin/dub
-rough build time=62s
+executable size=5232472 bin/dub
+rough build time=61s
Full build output
DUB version 1.35.1, built on Jan  6 2024
LDC - the LLVM D compiler (1.36.0):
  based on DMD v2.106.1 and LLVM 17.0.6
  built with LDC - the LLVM D compiler (1.36.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
    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.36.0/x64/ldc2-1.36.0-linux-x86_64/bin/ldc2 for x86_64.
    Building dub 1.36.0-beta.1+commit.37.gebde196d: building configuration [application]
     Linking dub
STAT:statistics (-before, +after)
STAT:executable size=5232472 bin/dub
STAT:rough build time=61s

@Geod24
Copy link
Member

Geod24 commented Jan 13, 2024

Currently you can work around this by using dflags on dependencies. It isn't pretty but it works.
GDC should really treat deprecations and warnings differently though.

@the-horo
Copy link
Contributor Author

Currently you can work around this by using dflags on dependencies. It isn't pretty but it works.

Can you explain me how this solution would go as I didn't know you could do that?

@Geod24
Copy link
Member

Geod24 commented Jan 13, 2024

 "dependencies": {
        "vibe-d":           { "version": "~>0.9", "dflags" : [ "-preview=in", "-revert=dtorfields" ] }
    }

It will apply transitively, so eventcore, vibe-core, etc... will have those DFLAGS defined.
Not ideal but until we take the time to untangle how we want the build system to look like, this is the "best" way to do it.

@the-horo
Copy link
Contributor Author

 "dependencies": {
        "vibe-d":           { "version": "~>0.9", "dflags" : [ "-preview=in", "-revert=dtorfields" ] }
    }

It will apply transitively, so eventcore, vibe-core, etc... will have those DFLAGS defined. Not ideal but until we take the time to untangle how we want the build system to look like, this is the "best" way to do it.

My issue is more of the flags being put in the wrong place and overwriten rather then not applying transitively:

$ DFLAGS='-Wno-error' dub -v build --compiler=gdc
...
    Building dub 1.36.0-beta.1+commit.37.gf86ed3e9: building configuration [application]
gdc -Wno-error -o <cache>/dub -Werror -Wall -fversion=DubUseCurl -fversion=DubApplication ...

The same thing happens if I put dflags "-Wno-error" in dub.sdl.

There should be a way to have flags be passed later on the command line so that their values are respected, that's what I'm trying to fix.

@Geod24
Copy link
Member

Geod24 commented Jan 15, 2024

Have you tried "buildRequirements": [ "allowWarnings" ] ?

@PetarKirov
Copy link
Member

@Geod24 I think there is a common use case which Dub doesn't support very well. While developers working on a given project may find that adding "buildRequirements": [ "allowWarnings" ] to their dub.sdl/json may be the ideal approach, the same is not true for consumers of projects which ideally should not need to modify the source code of any of their dependencies. E.g. imagine a user of Linux distro that wants to rebuild their entire system for their exact CPU. Many of them would like the simplicity exporting CFLAGS=-march=native and hitting "rebuild all".

@the-horo
Copy link
Contributor Author

Have you tried "buildRequirements": [ "allowWarnings" ] ?

Yes, that fixes it, but my issue is not about having a way to disable it, it's having an easy-access way to do it. Editing the dub.* file I don't consider enough. You can also consider the second issue of having user flags be respected more, which I think is always nice.

@Geod24 I think there is a common use case which Dub doesn't support very well. While developers working on a given project may find that adding "buildRequirements": [ "allowWarnings" ] to their dub.sdl/json may be the ideal approach, the same is not true for consumers of projects which ideally should not need to modify the source code of any of their dependencies. E.g. imagine a user of Linux distro that wants to rebuild their entire system for their exact CPU. Many of them would like the simplicity exporting CFLAGS=-march=native and hitting "rebuild all".

I agree with this. Gentoo, for which I maintain D packages, does require disabling -Werror: https://devmanual.gentoo.org/ebuild-writing/common-mistakes/index.html#-werror-compiler-flag-not-removed.

@Geod24 Geod24 changed the title Let uesr $DFLAGS override build settings as much as possible Let user $DFLAGS override build settings as much as possible Jan 17, 2024
@Geod24
Copy link
Member

Geod24 commented Jan 17, 2024

@the-horo : I can see the use case being useful, I myself had my share of similar issues.
I'm starting to think the way we currently do DFLAGS does not make much sense, as it's its own build type. Anyway, looking at the code with a fresh pair of eyes, I think we should go ahead with it.

@Geod24 Geod24 merged commit 2ace859 into dlang:master Jan 17, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants