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

kinke · 2022-06-17T16:07:16Z

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 · 2022-06-17T16:13:09Z

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

WebFreak001 · 2022-06-20T15:30:10Z

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.

kinke · 2022-06-20T15:42:59Z

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.

WebFreak001 · 2022-06-20T15:50:22Z

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 ?

skoppe · 2022-07-12T13:30:51Z

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.

kinke · 2022-07-12T13:46:05Z

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).

WebFreak001 · 2022-10-15T13:09:15Z

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

Geod24 · 2022-10-16T09:14:55Z

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.

nordlow · 2022-10-27T09:31:23Z

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 · 2022-10-27T09:47:00Z

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

nordlow · 2022-10-27T09:49:23Z

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.

kinke · 2022-10-27T11:46:44Z

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.

github-actions · 2023-08-19T13:08:14Z

✅ PR OK, no changes in deprecations or warnings

Total deprecations: 0

Total warnings: 0

Build statistics:

 statistics (-before, +after)
-executable size=5259464 bin/dub
+executable size=5263616 bin/dub
 rough build time=64s

Full build output

DUB version 1.37.0, built on May 11 2024
LDC - the LLVM D compiler (1.38.0):
  based on DMD v2.108.1 and LLVM 18.1.5
  built with LDC - the LLVM D compiler (1.38.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.38.0/x64/ldc2-1.38.0-linux-x86_64/bin/ldc2 for x86_64.
    Building dub 1.38.1-rc.1+commit.69.g1a764d45: building configuration [application]
     Linking dub
STAT:statistics (-before, +after)
STAT:executable size=5263616 bin/dub
STAT:rough build time=64s

source/dub/project.d

In case the root package directory doesn't contain a `dub.selections.json` file, look in parent directories too and load the first (deepest) one. 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.

…m applying to test projects]

kinke · 2023-08-19T13:43:13Z

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.

WebFreak001 · 2023-08-19T13:59:28Z

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

kinke · 2023-09-06T14:39:27Z

Ping.

As e.g. reggae needs to know which dub.selections.json file was used.

atilaneves · 2024-05-10T14:20:35Z

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?

kinke · 2024-06-12T14:01:09Z

Updated yet again to resolve merge conflicts. The change is better integrated now; I got rid of the earlier new extra helper (lookupAndParseSelectionsFile() or so), and instead adapted the new (in master) PackageManager.readSelections() method, incl. making that public. The reason is that reggae needs to know the path to the used dub.selections.json, so that it can add a proper dependency in the generated build scripts, leading to a reggae re-run whenever a used dub.selections.json has changed.

…ent-dir Conflicts: source/dub/project.d

iff => if and only if

Geod24 · 2024-06-12T20:51:58Z

source/dub/packagemanager.d

+struct SelectionsFileLookupResult {
+	NativePath absolutePath; // to dub.selections.json
+	SelectionsFile selectionsFile;
+}
+


Every public symbol should have DDOC, even if simple

I think we could fold this into SelectionsFile, can't we ? In which case we can disregard (1).

would be doable after a quick glance, but would need a changed signature for the public SelectionsFile.fromYAML() factory function (unused in dub AFAICT), adding a path parameter.

fromYAML is a hook used by Configy so changing the signature is not going to work out so well.

Yeah I figured, so I went with option 1.

Geod24 · 2024-06-12T20:53:41Z

source/dub/packagemanager.d

+	 *   might use an unsupported version (see `SelectionsFile` documentation).
 	 */
-	package final Nullable!SelectionsFile readSelections(in Package project) {
+	final Nullable!SelectionsFileLookupResult readSelections(in NativePath absProjectPath)


If we make it public, I'm not sure it should be final anymore.

Non-final now.

source/dub/packagemanager.d

source/dub/project.d

Geod24 · 2024-06-12T21:00:20Z

test/selections-from-parent-dir.sh

+#!/usr/bin/env bash
+
+. $(dirname "${BASH_SOURCE[0]}")/common.sh
+cd "${CURR_DIR}/selections-from-parent-dir/pkg2" || die "Could not cd."
+
+"$DUB" build --nodeps
+
+if [ -f dub.selections.json ]; then
+    die "Shouldn't create a dub.selections.json in pkg2 subdir."
+fi


Any chance you could convert this to the new test framework ? E.g. see source/dub/test/other.d.

I did; pretty cool. 👍 - dub test -c library-nonet fails for some rather bizarre reason (the CI problem), dub test alone works fine.

object.Exception@source/dub/packagemanager.d(408): No package file found in /dub/project/pkg2/../pkg1/, expected one of dub.json/dub.sdl/package.json

Solved; was a problem of the mocked file system (no entries for . and .., so needs extra normalization; might be required in other places too).

changelog/selections_from_parent_dir.dd

Geod24

Looks good! Made a few suggestions. I thing there's nothing blocking the merge (although I would like to see the points addressed, especially the ones about inheritable property and absolute path, as that could trigger an assert easily) but can do it myself if you just want to go agead.

source/dub/project.d

Geod24 · 2024-06-17T10:13:30Z

source/dub/packagemanager.d

+		// check for dub.selections.json in root project dir first, then walk up its
+		// parent directories and look for inheritable dub.selections.json files
+		for (NativePath dir = absProjectPath; !dir.empty; dir = dir.hasParentPath ? dir.parentPath : NativePath.init) {
+			const path = dir ~ "dub.selections.json";
+			if (!this.existsFile(path))
+				continue;


Logically this would be better represented as something that returns a path that is then acted on. The code as-is does not mean to be changed (it works, that's what matters) but a loop that ends up with an unconditional break is not a loop (or IOW, contains too much code).

Geod24 · 2024-06-17T10:17:15Z

source/dub/project.d

-		auto selected = mgr.readSelections(pack);
-		// Parsing error that will be displayed to the user or just no selections
-		if (selected.isNull())
+		auto lookupResult = mgr.readSelections(pack.path);


How do we know here that the path is absolute ?
Loops like we should just change the signature of the function ?

source/dub/recipe/selection.d

Geod24 · 2024-06-17T10:22:39Z

source/dub/packagemanager.d

+/// A SelectionsFile associated with its file-system path.
+struct SelectionsFileLookupResult {
+	/// The absolute path to the dub.selections.json file
+	/// (potentially inherited from a parent directory of the root package).
+	NativePath absolutePath;
+	/// The parsed dub.selections.json file.
+	SelectionsFile selectionsFile;
+}
+


I would like to fold this into SelectionsFile, but I know it's not currently possible. All things considered, it might be a good thing to add to Configy, so I'll have a look at it myself. It should not block this being merged.

Geod24 · 2024-06-17T10:22:55Z

source/dub/test/selections_from_parent_dir.d

@@ -0,0 +1,113 @@
+module dub.test.selections_from_parent_dir;


Module doc please

Geod24 · 2024-06-17T10:26:15Z

source/dub/test/selections_from_parent_dir.d

+    assert(dub.fs.existsFile(nestedPath));
+    assert(dub.fs.readFile(path) == dubSelectionsJsonContent,
+        "Inherited dub.selections.json modified after dub upgrade!");
+    const nestedContent = cast(string) dub.fs.readFile(nestedPath);


Geod24 · 2024-06-17T10:31:03Z

source/dub/test/selections_from_parent_dir.d

+    // no selections for root/a/b/
+    {
+        const result = dub.packageManager.readSelections(root_a_b);
+        assert(result.isNull());
+    }
+
+    // local selections for root/a/
+    {
+        const result = dub.packageManager.readSelections(root_a);
+        assert(!result.isNull());
+        assert(result.get().absolutePath == root_a ~ "dub.selections.json");
+        assert(!result.get().selectionsFile.inheritable);
+    }
+
+    // local selections for root/
+    {
+        const result = dub.packageManager.readSelections(root);
+        assert(!result.isNull());
+        assert(result.get().absolutePath == root ~ "dub.selections.json");
+        assert(result.get().selectionsFile.inheritable);
+    }
+
+    // after removing non-inheritable root/a/dub.selections.json: inherited root selections for root/a/b/
+    {
+        dub.fs.removeFile(root_a ~ "dub.selections.json");
+
+        const result = dub.packageManager.readSelections(root_a_b);
+        assert(!result.isNull());
+        assert(result.get().absolutePath == root ~ "dub.selections.json");
+        assert(result.get().selectionsFile.inheritable);
+    }


Maybe those tests should check the version too ? IIUC before the rm hasAlldependencies should be false, and it should be true afterwards. You can use TestDub.newTest to easily test this.

Geod24 · 2024-06-17T14:24:33Z

@kinke : I took the liberty to push the requested fixes, as I know you are away and I'm sure you'd like this fixed sooner than later.

Geod24 · 2024-06-17T14:25:32Z

This is a scheduled macOS-11 brownout. The macOS-11 environment is deprecated and will be removed on June 28th, 2024.

Well...

Geod24 · 2024-06-17T14:50:20Z

../../../.dub/packages/vibe-core/2.7.1/vibe-core/source/vibe/core/path.d(72,8): Error: none of the overloads of `endsWithSlash` are callable using argument types `(bool) const`
../../../.dub/packages/vibe-core/2.7.1/vibe-core/source/vibe/core/path.d(510,17):        Candidates are: `vibe.core.path.GenericPath!(PosixPathFormat).GenericPath.endsWithSlash() const`
../../../.dub/packages/vibe-core/2.7.1/vibe-core/source/vibe/core/path.d(512,17):                        `vibe.core.path.GenericPath!(PosixPathFormat).GenericPath.endsWithSlash(bool v)`
source/dub/project.d(127,60): Error: template instance `vibe.core.path.relativeTo!(const(GenericPath!(PosixPathFormat)))` error instantiating

Well looks like vibe-core has a bug too, but we can work around that.

Geod24 · 2024-06-18T12:15:45Z

None of the workaround is pretty though, so I opened vibe-d/vibe-core#400

…arent-dir

kinke · 2024-07-01T11:27:28Z

Thanks Mathias!

kinke requested a review from Geod24 June 17, 2022 16:07

kinke force-pushed the selections-from-parent-dir branch from 5384fac to 8b1f3a9 Compare June 17, 2022 16:09

kinke marked this pull request as draft June 17, 2022 16:12

kinke force-pushed the selections-from-parent-dir branch from ed59793 to 979905f Compare June 20, 2022 15:17

kinke force-pushed the selections-from-parent-dir branch from 979905f to 1ee1bfc Compare August 19, 2023 13:08

WebFreak001 reviewed Aug 19, 2023

View reviewed changes

source/dub/project.d Outdated Show resolved Hide resolved

kinke added 3 commits August 19, 2023 15:24

[add test/dub.selections.json to prevent root dub.selections.json fro…

f5d76e7

…m applying to test projects]

[make it work with vibe-core's NativePath too]

2b5580a

kinke force-pushed the selections-from-parent-dir branch from 1ee1bfc to 6e088c8 Compare August 19, 2023 13:25

kinke marked this pull request as ready for review August 19, 2023 13:41

kinke force-pushed the selections-from-parent-dir branch from 955aed5 to b81c967 Compare January 30, 2024 13:45

kinke and others added 2 commits January 31, 2024 16:15

Add optional inheritable flag to dub.selections.json

c803a29

[extract public lookupAndParseSelectionsFile() helper]

ee9e36c

As e.g. reggae needs to know which dub.selections.json file was used.

kinke force-pushed the selections-from-parent-dir branch 2 times, most recently from 3a0451d to b5a6b8d Compare June 12, 2024 13:15

Merge remote-tracking branch 'origin/master' into selections-from-par…

db1139a

…ent-dir Conflicts: source/dub/project.d

kinke force-pushed the selections-from-parent-dir branch from b5a6b8d to db1139a Compare June 12, 2024 14:31

Geod24 reviewed Jun 12, 2024

View reviewed changes

kinke added 4 commits June 13, 2024 16:09

[address review nits]

d33ede2

[migrate test to new framework]

13d252e

[add dub upgrade test]

1edca8d

[modernize unittest to get rid of new deprecation]

4b6638f

kinke force-pushed the selections-from-parent-dir branch from c23d3c2 to 4b6638f Compare June 13, 2024 17:18

kinke added 2 commits June 13, 2024 20:00

[fix an unrelated missing path-normalize call]

f20534c

[add unittest wrt. breaking the inheritance chain]

12212b2

Geod24 approved these changes Jun 17, 2024

View reviewed changes

Geod24 added 3 commits June 17, 2024 14:11

Make loadSelections accept NativePath

c94e824

Refactor inheritable selections lookup loop

546042d

Improve tests

08dcd47

Geod24 enabled auto-merge (squash) June 17, 2024 14:24

Geod24 mentioned this pull request Jun 18, 2024

Make relativeTo work with const paths vibe-d/vibe-core#400

Merged

Merge remote-tracking branch 'upstream/master' into selections-from-p…

cd770ed

…arent-dir

Geod24 force-pushed the selections-from-parent-dir branch from bb236b9 to cd770ed Compare June 18, 2024 13:37

Geod24 merged commit 6c0bfc7 into dlang:master Jun 18, 2024

kinke deleted the selections-from-parent-dir branch July 1, 2024 11:27

		@@ -0,0 +1,113 @@
		module dub.test.selections_from_parent_dir;

Uh oh!

Look up dub.selections.json files in parent directories too #2272

Look up dub.selections.json files in parent directories too #2272

Uh oh!

Conversation

kinke commented Jun 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kinke commented Jun 17, 2022

Uh oh!

WebFreak001 commented Jun 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kinke commented Jun 20, 2022

Uh oh!

WebFreak001 commented Jun 20, 2022

Uh oh!

skoppe commented Jul 12, 2022

Uh oh!

kinke commented Jul 12, 2022

Uh oh!

WebFreak001 commented Oct 15, 2022

Uh oh!

Geod24 commented Oct 16, 2022

Uh oh!

nordlow commented Oct 27, 2022

Uh oh!

nordlow commented Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nordlow commented Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kinke commented Oct 27, 2022

Uh oh!

github-actions bot commented Aug 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kinke commented Aug 19, 2023

Uh oh!

WebFreak001 commented Aug 19, 2023

Uh oh!

kinke commented Sep 6, 2023

Uh oh!

atilaneves commented May 10, 2024

Uh oh!

kinke commented Jun 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinke Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Geod24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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

kinke commented Jun 17, 2022 •

edited

Loading

WebFreak001 commented Jun 20, 2022 •

edited

Loading

nordlow commented Oct 27, 2022 •

edited

Loading

nordlow commented Oct 27, 2022 •

edited

Loading

github-actions bot commented Aug 19, 2023 •

edited

Loading

kinke Jun 13, 2024 •

edited

Loading