dub icon indicating copy to clipboard operation
dub copied to clipboard

Look up `dub.selections.json` files in parent directories too

Open kinke opened this issue 2 years ago • 17 comments

In case the root package directory doesn't contain a dub.selections.json file, dub now looks in parent directories too and potentially uses the first (deepest) one it finds - iff that JSON file contains an optional new "inheritable": true flag.

This allows using a 'central' dub.selections.json file for a repository containing multiple dub projects, making it automatically apply to all builds in that source tree if located in the repository root directory (unless a local dub.selections.json overrides it).

Such an inherited selections file is never mutated when running dub for a nested project, i.e., changes are always saved to a local dub.selections.json file.

kinke avatar Jun 17 '22 16:06 kinke

Ouch, looks like the testsuite fails due to dub's own dub.selections.json in its repo root...

kinke avatar Jun 17 '22 16:06 kinke

I don't think this is a good behavior to add and it might confuse tools assuming that dub.selections.json is the truth for your packages.

There is already dub add-local, are you looking for that behavior, but controllable from the filesystem? I think a dedicated file for that would make more sense.

Additionally if we were to change dub.selections.json (semantics) here, I think a version bump in the file would be in order.

WebFreak001 avatar Jun 20 '22 15:06 WebFreak001

I understand this is a pretty big breaking change and might not land that easily, possibly having to resort to some environment variable to control it.

The obvious usage is for us at Symmetry, for mono/big repos with a big dub.selections.json in its root, containing all deps of all nested projects. There, the behavior should be automatic (dub build/test as before), no dub add-local etc. stuff.

kinke avatar Jun 20 '22 15:06 kinke

I think it's probably fine to add the feature, but I'm not sure yet on the semantics. If we just inherit from parent directories that would be nice for sub-packages of projects, which want to read from the dub.selections.json anyway and not need to manually dub upgrade all the sub-packages.

I think it might be best to have some kind of "inheritSelections": true property in the dub.json/sdl, but I'm not yet fully set on that thought.

What do you think @s-ludwig ?

WebFreak001 avatar Jun 20 '22 15:06 WebFreak001

Such an 'inherited' selections file is never mutated when running dub for a nested project, i.e., changes are always saved to a local dub.selections.json file.

Why is that? Seems to make it more complicated. Instead I would either error out or change parent dub.selections.json, possibly interactively asking for the latter.

skoppe avatar Jul 12 '22 13:07 skoppe

Why is that? Seems to make it more complicated. Instead I would either error out or change parent dub.selections.json, possibly interactively asking for the latter.

The number one reason is implementation complexity - the current changes are trivial and don't require any keeping-around of the used path and re-mapping relative paths.

Then I don't know enough about dub upgrade to say OTOH what happens if you'd invoke dub upgrade in some tiny nested project, using a single of 100 dependencies specified in the inherited selections file, and whether that would make any sense. With the current proposal, you'd end up with a local dub.selections.json and can diff/upstream the changes into the inherited selections file manually.

I don't see why that makes it more complicated. The expected use case is a big, complete master dub.selections.json containing all deps, where a normal dub build/test doesn't generate a local dub.selections.json (tested in the added test).

kinke avatar Jul 12 '22 13:07 kinke

ok I think it makes sense to have only one dub.selections.json - but I think it should not check the parent dub.selections.json files (e.g. to avoid issues such as with the test runner here), but instead only use the parent package dub.selections.json, if it's a subpackage.

When running dub upgrade inside a subpackage, it should probably still keep its behavior of saving its own dub.selections.json, although I'm unsure which package we would choose at that point.

Also note Sönke Ludwig added support for dub upgrade -s which upgrades the root dub.selections.json as well as all sub-packages. We might need to disable the warning on that or offer a utility to delete subpackage dub.selections.json files?

CC @s-ludwig

WebFreak001 avatar Oct 15 '22 13:10 WebFreak001

To quote:

The general idea with sub packages is that they are logically completely separate packages, with the exception of being tied to the same version. Everything in addition to that would lead to another dimension of opt-in/opt-out directives that become necessary to override the default inheritance behavior.

I think we're ought to either stick with this principle and properly document it, OR re-evaluate what is people's natural expectation / need around subpackages. It's confusing even to maintainers when subpackages should be used vs configurations.

Geod24 avatar Oct 16 '22 09:10 Geod24

How do other languages solves these issues? Would be interesting to check if Rust have these same issues and implement them in a similar way to what we plan.

nordlow avatar Oct 27 '22 09:10 nordlow

Apart from merging with master what more work needs to happen in this MR, @kinke?

nordlow avatar Oct 27 '22 09:10 nordlow

Such an 'inherited' selections file is never mutated when running dub for a nested project, i.e., changes are always saved to a local dub.selections.json file.

What is meant by "changes are always saved to a local dub.selections.json file.", @kinke? I interpret "Such an 'inherited' selections file is never mutated when running dub for a nested project" as no dub.selections.json will ever be written in such cases but then you say there will be a dub.selections.json saved; I don't understand.

nordlow avatar Oct 27 '22 09:10 nordlow

Apart from merging with master what more work needs to happen in this MR?

Well, it's a pretty big breaking change and might not be always desirable. So maybe make it opt-in via cmdline flag or extending the dub.selections.json file (inheritable: true or so - but that would still lead to more file lookups).

What is meant by "changes are always saved to a local dub.selections.json file."?

If you run e.g. dub upgrade in a nested project, changes are saved to a dub.selections.json file in that dir, not to the inherited file from some parent dir.

kinke avatar Oct 27 '22 11:10 kinke

✅ PR OK, no changes in deprecations or warnings

Total deprecations: 0

Total warnings: 0

Build statistics:

 statistics (-before, +after)
-executable size=5347864 bin/dub
-rough build time=63s
+executable size=5356056 bin/dub
+rough build time=62s
Full build output
DUB version 1.36.0, built on Mar  3 2024
LDC - the LLVM D compiler (1.37.0):
  based on DMD v2.107.1 and LLVM 17.0.6
  built with LDC - the LLVM D compiler (1.37.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.37.0/x64/ldc2-1.37.0-linux-x86_64/bin/ldc2 for x86_64.
    Building dub 1.37.0-rc.1+commit.19.gdcac276d: building configuration [application]
     Linking dub
STAT:statistics (-before, +after)
STAT:executable size=5356056 bin/dub
STAT:rough build time=62s

github-actions[bot] avatar Aug 19 '23 13:08 github-actions[bot]

Rebased and made opt-in via a new optional inheritable: true in the dub.selections.json file to be inherited by nested projects.

I've left the fileVersion at 1 for now; I guess we'd have to bump it. But maybe only for files containing inheritable: true, so that the whole D ecosystem doesn't have to be adapted.

kinke avatar Aug 19 '23 13:08 kinke

I think keeping fileVersion at 1 is fine here, since it's not backwards incompatible

WebFreak001 avatar Aug 19 '23 13:08 WebFreak001

Ping.

kinke avatar Sep 06 '23 14:09 kinke

Given the opt-in nature now with having to set inheritability in the parent selections file, I don't see any reason to not merget his. Objections?

atilaneves avatar May 10 '24 14:05 atilaneves