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.

Sign up for GitHub

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

Jump to bottom

[stable] Fix #2973 #2974

Open
wants to merge 4 commits into
base: stable
Choose a base branch
from
+51 −13
Open

[stable] Fix #2973 #2974

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
030f2a2
Add test case for issue #2973
kinke Oct 31, 2024
6d54e20
Fix #2973
kinke Oct 29, 2024
cef47ae
Tentatively fix similar issue for SCM deps
kinke Oct 29, 2024
41002a4
[take 2 - fix PackageManager instead]
kinke Nov 1, 2024
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
9 changes: 5 additions & 4 deletions source/dub/project.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,8 @@ class Project {
(VersionRange range) {
// See `dub.recipe.selection : SelectedDependency.fromYAML`
assert(range.isExactVersion());
return m_packageManager.getPackage(dep.name, vspec.version_);
auto tmp = m_packageManager.getPackage(basename, vspec.version_);
return resolveSubPackage(tmp, subname, true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay fine if it's always been like that.

},
);
} else if (m_dependencies.canFind!(d => PackageName(d.name).main == basename)) {
Expand All @@ -581,10 +582,10 @@ class Project {
{
if (!vspec.repository.empty) {
p = m_packageManager.loadSCMPackage(basename, vspec.repository);
resolveSubPackage(p, subname, false);
enforce(p !is null,
"Unable to fetch '%s@%s' using git - does the repository and version exists?".format(
dep.name, vspec.repository));
"Unable to fetch '%s@%s' using git - does the repository and version exist?".format(
basename, vspec.repository));
p = resolveSubPackage(p, subname, false);
} else if (!vspec.path.empty && is_desired) {
NativePath path = vspec.path;
if (!path.absolute) path = pack.path ~ path;
Expand Down
2 changes: 1 addition & 1 deletion source/dub/test/dependencies.d
View file
Open in desktop
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
View file
Open in desktop
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