[stable] Fix #2973: [Reg] error when depending on a subpackages that is already resolved#2974

kinke · 2024-10-29T16:10:02Z

No description provided.

github-actions · 2024-10-29T16:11:34Z

✅ 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=62s
+executable size=5297464 bin/dub
+rough build time=61s

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.6.gcdce19f0: 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=61s

kinke · 2024-10-29T16:20:13Z

source/dub/project.d

 						assert(range.isExactVersion());
-						return m_packageManager.getPackage(dep.name, vspec.version_);
+						auto tmp = m_packageManager.getPackage(basename, vspec.version_);
+						return resolveSubPackage(tmp, subname, true);


This fixes the testcase in #2973. Not sure if this is the right fix though; PackageManager.getPackage() returning the parent package when given a subpackage name doesn't make a lot of sense IMO.

Yeah it doesn't make a lot of sense but it's always been this way. It was one of the things I wanted to fix when I introduced PackageName but never got around to it.
For dependency resolution we always store references to parent package, not subpackage, because all subpackages should resolve to the same parent. I would need to inspect the code to see if that is really the right fix.

Okay fine if it's always been like that.

kinke · 2024-10-31T14:36:22Z

source/dub/test/dependencies.d

    assert(dub.project.getDependency("b", true), "Missing 'b' dependency");
    assert(dub.project.getDependency("c", true), "Missing 'c' dependency");
-    assert(dub.project.getDependency("c", true), "Missing 'd' dependency");
+    assert(dub.project.getDependency("d", true), "Missing 'd' dependency");


Just happened to notice this while browsing the tests.

Geod24

Thanks! By when do you need this reviewed ?

Geod24 · 2024-10-31T17:20:52Z

source/dub/project.d

 						assert(range.isExactVersion());
-						return m_packageManager.getPackage(dep.name, vspec.version_);
+						auto tmp = m_packageManager.getPackage(basename, vspec.version_);
+						return resolveSubPackage(tmp, subname, true);


Yeah it doesn't make a lot of sense but it's always been this way. It was one of the things I wanted to fix when I introduced PackageName but never got around to it.
For dependency resolution we always store references to parent package, not subpackage, because all subpackages should resolve to the same parent. I would need to inspect the code to see if that is really the right fix.

kinke · 2024-10-31T17:48:40Z

By when do you need this reviewed ?

I assume you've already reviewed this, and if you haven't, just expand the diff context - all other branches do the subpackage resolution (after getting the base package). :) - This is IMO a major release blocker, so v1.39 final should definitely have this.

kinke · 2024-10-31T18:12:23Z

source/dub/test/subpackages.d

+            `{ "fileVersion": 1, "versions": { "b": "1.0.0" } }`);
+        root.writePackageFile("b", "1.0.0",
+            `{ "name": "b", "version": "1.0.0", "subPackages": [ { "name": "a" } ] }`);
+    });


Btw thx again for the test framework, sooo much better than the old tests...

I wish we had a similar filesystem abstract in Phobos to make this kind of unittests the norm. It is so much more expressive.
I really need to finish it though, there's an issue with Windows at the moment which prevents mimicking the real world properly.

Geod24 · 2024-10-31T18:57:41Z

I assume you've already reviewed this, and if you haven't, just expand the diff context - all other branches do the subpackage resolution (after getting the base package). :) - This is IMO a major release blocker, so v1.39 final should definitely have this.

Do you know what introduced the regression ? I am fairly sure it used to work ?

kinke · 2024-10-31T20:47:15Z

Do you know what introduced the regression ? I am fairly sure it used to work ?

Oh, I thought this was all revamped after v1.38, but no, the 'problematic' line is the same as was in v1.38.0, which works for the original test case. Hmm...

kinke · 2024-10-31T21:00:49Z

Hmm, this looks suspicious:

dub/source/dub/packagemanager.d

Lines 1740 to 1747 in 7a50f4c

    
           logDiagnostic("Lazily loading package %s:%s from %s", name.main, vers, path); 
        
           auto p = mgr.load(path); 
        
           enforce( 
        
           	p.version_ == vers, 
        
           	format("Package %s located in %s has a different version than its path: Got %s, expected %s", 
        
           		name, path, p.version_, vers)); 
        
           mgr.addPackages(this.fromPath, p); 
        
           return p;

It returns the package from the path, which is always going to be a parent package.

kinke · 2024-11-01T03:36:52Z

source/dub/packagemanager.d

+		}
+
+		return null;
+	}


I've totally reworked the fix, tackling the PackageManager API instead and resolving the subpackage in the lazy-loading getPackage() case (that was the regression AFAICT), as well as newly in loadSCMPackage().

With more context now, I see that resolveSubPackage() is actually to be avoided, as it results in a full packages scan, killing the lazy-loading advantages. It's still used if the dub.selections.json contains a path-based selection, so with these current changes, we still get a full scan whenever depending on a subpackage of a path-selected parent package.

Right, anything that calls getPackageIterator needs to go / be reworked.

kinke changed the base branch from master to stable October 29, 2024 16:11

kinke force-pushed the fix2973 branch from e6d545d to 21f1ba2 Compare October 29, 2024 16:18

kinke commented Oct 29, 2024

View reviewed changes

kinke changed the title ~~Fix #2973~~ [stable] Fix #2973 Oct 29, 2024

kinke force-pushed the fix2973 branch from 7ddb815 to 3b6e697 Compare October 29, 2024 22:12

Add test case for issue dlang#2973

030f2a2

kinke force-pushed the fix2973 branch from 3b6e697 to 030f2a2 Compare October 31, 2024 14:18

kinke added 2 commits October 31, 2024 15:19

Fix dlang#2973

6d54e20

Tentatively fix similar issue for SCM deps

cef47ae

kinke commented Oct 31, 2024

View reviewed changes

kinke marked this pull request as ready for review October 31, 2024 14:37

kinke requested a review from Geod24 October 31, 2024 15:03

Geod24 reviewed Oct 31, 2024

View reviewed changes

kinke commented Oct 31, 2024

View reviewed changes

kinke commented Nov 1, 2024

View reviewed changes

kinke force-pushed the fix2973 branch 2 times, most recently from 9b6c23b to 32ade85 Compare November 1, 2024 18:18

[take 2 - fix PackageManager instead]

624f2af

kinke force-pushed the fix2973 branch from 32ade85 to 624f2af Compare November 2, 2024 22:17

Geod24 approved these changes Nov 4, 2024

View reviewed changes

Geod24 changed the title ~~[stable] Fix #2973~~ [stable] Fix #2973: [Reg] error when depending on a subpackages that is already resolved Nov 4, 2024

Geod24 merged commit ec809a5 into dlang:stable Nov 4, 2024

kinke deleted the fix2973 branch November 4, 2024 13:35

kinke mentioned this pull request Nov 4, 2024

Serious regression in stable/v1.39 wrt. 'Non-optional dependency not found in dependency tree!?' #2973

Closed

Uh oh!

Conversation

kinke commented Oct 29, 2024

Uh oh!

github-actions bot commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Geod24 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinke commented Oct 31, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Geod24 commented Oct 31, 2024

Uh oh!

kinke commented Oct 31, 2024

Uh oh!

kinke commented Oct 31, 2024

Uh oh!

kinke Nov 1, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

github-actions bot commented Oct 29, 2024 •

edited

Loading

kinke Nov 1, 2024 •

edited

Loading