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

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

Merged
merged 4 commits into from
Nov 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 35 additions & 10 deletions source/dub/packagemanager.d
Original file line number Diff line number Diff line change
Expand Up @@ -510,23 +510,26 @@ class PackageManager {

// Before doing a git clone, let's see if the package exists locally
if (this.fs.existsDirectory(destination)) {
bool isMatch(Package p) {
return p.name == name.toString() && p.basePackage.path == destination;
}

// It exists, check if we already loaded it.
// Either we loaded it on refresh and it's in PlacementLocation.user,
// or we just added it and it's in m_internal.
foreach (p; this.m_internal.fromPath)
if (p.path == destination)
if (isMatch(p))
return p;
if (this.m_repositories.length)
foreach (p; this.m_repositories[PlacementLocation.user].fromPath)
if (p.path == destination)
if (isMatch(p))
return p;
} else if (!this.gitClone(repo.remote, gitReference, destination))
return null;

Package result = this.load(destination);
if (result !is null)
this.addPackages(this.m_internal.fromPath, result);
return result;
Package p = this.load(destination);
if (p is null) return null;
return this.addPackagesAndResolveSubPackage(this.m_internal.fromPath, p, name);
}

/**
Expand Down Expand Up @@ -1293,7 +1296,7 @@ symlink_exit:
return serialized;
}

/// Adds the package and scans for sub-packages.
/// Adds the package and its sub-packages.
protected void addPackages(ref Package[] dst_repos, Package pack)
{
// Add the main package.
Expand Down Expand Up @@ -1323,6 +1326,29 @@ symlink_exit:
}
}
}

/// Adds the package and its sub-packages, and returns the added package matching
/// the specified name (of the package itself or a sub-package).
/// Returns null if the sub-package doesn't exist.
private Package addPackagesAndResolveSubPackage(ref Package[] dst_repos, Package pack,
in PackageName nameToResolve)
in(pack.name == nameToResolve.main.toString(),
"nameToResolve must be the added package or one of its sub-packages")
{
this.addPackages(dst_repos, pack);

if (nameToResolve.sub.empty)
return pack;

// available sub-packages have been appended
foreach_reverse (sp; dst_repos) {
if (sp.parentPackage is pack && sp.name == nameToResolve.toString())
return sp;
}

logDiagnostic("Sub-package %s not found in parent package", nameToResolve);
return null;
}
Copy link
Contributor Author

@kinke kinke Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

deprecated(OverrideDepMsg)
Expand Down Expand Up @@ -1742,9 +1768,8 @@ package struct Location {
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;
name.main, path, p.version_, vers));
return mgr.addPackagesAndResolveSubPackage(this.fromPath, p, name);
}

/**
Expand Down
8 changes: 3 additions & 5 deletions source/dub/project.d
Original file line number Diff line number Diff line change
Expand Up @@ -557,8 +557,7 @@ class Project {
return resolveSubPackage(tmp, subname, true);
},
(Repository repo) {
auto tmp = m_packageManager.loadSCMPackage(basename, repo);
return resolveSubPackage(tmp, subname, true);
return m_packageManager.loadSCMPackage(dep.name, repo);
},
(VersionRange range) {
// See `dub.recipe.selection : SelectedDependency.fromYAML`
Expand All @@ -580,10 +579,9 @@ class Project {
if (p is null)
{
if (!vspec.repository.empty) {
p = m_packageManager.loadSCMPackage(basename, vspec.repository);
resolveSubPackage(p, subname, false);
p = m_packageManager.loadSCMPackage(dep.name, vspec.repository);
enforce(p !is null,
"Unable to fetch '%s@%s' using git - does the repository and version exists?".format(
"Unable to fetch '%s@%s' using git - does the repository and version exist?".format(
dep.name, vspec.repository));
} else if (!vspec.path.empty && is_desired) {
NativePath path = vspec.path;
Expand Down
2 changes: 1 addition & 1 deletion source/dub/test/dependencies.d
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ version "1.0.0"`, PackageFormat.sdl);
assert(dub.project.hasAllDependencies(), "project has missing dependencies");
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");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just happened to notice this while browsing the tests.

assert(dub.project.getDependency("no", true) is null, "Returned unexpected dependency");
}

Expand Down
17 changes: 17 additions & 0 deletions source/dub/test/subpackages.d
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,20 @@ unittest

assert(!dub.packageManager().getPackage(PackageName("b:b"), Version("1.1.0")));
}

// https://github.com/dlang/dub/issues/2973
unittest
{
scope dub = new TestDub((scope Filesystem root) {
root.writeFile(TestDub.ProjectPath ~ "dub.json",
`{ "name": "a", "dependencies": { "b:a": "~>1.0" } }`);
root.writeFile(TestDub.ProjectPath ~ "dub.selections.json",
`{ "fileVersion": 1, "versions": { "b": "1.0.0" } }`);
root.writePackageFile("b", "1.0.0",
`{ "name": "b", "version": "1.0.0", "subPackages": [ { "name": "a" } ] }`);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

dub.loadPackage();

assert(dub.project.hasAllDependencies(), "project has missing dependencies");
assert(dub.project.getDependency("b:a", true), "Missing 'b:a' dependency");
}
Loading