Reimplement dependency resolution as a recursive process #1513

s-ludwig · 2018-07-10T08:26:58Z

The new approach minimizes the dependency version graph while descending, making it much less likely to run into pathological cases. It also is a lot easier to understand and reason about than the previous iterative approach.

The downside is dynamic and stack memory use, where the latter could be optimized using a COW approach. If stack overflow should turn out to become an issue in large dependency graphs, the algorithm could be converted to use a heap based stack.

Fixes #1300.
Fixes #1508.

Looks related, but appears to be fixed already: #1484
#1001 appears to have been fixed already (gives a proper error message)
#1345 also appears to have been fixed, this time externally by fixing the faulty dependencies

dlang-bot · 2018-07-10T08:26:59Z

Thanks for your pull request, @s-ludwig!

WebFreak001 · 2018-07-10T09:04:41Z

Are there also changes in performance by this?

WebFreak001 · 2018-07-10T09:06:56Z

test/expected-issue1037-output

-	b depends on ~>1.0.2
+Conflicting dependencies to gitcompatibledubpackage:
+  b >=0.0.0 @/home/runner/dub/test/issue1037-better-dependency-messages/b depends on gitcompatibledubpackage ~>1.0.2
+  issue1037-better-dependency-messages ~master depends on gitcompatibledubpackage 1.0.1


love the new details in this message

WebFreak001 · 2018-07-10T09:10:58Z

source/dub/dependencyresolver.d

+{
+	auto idx = indexOf(p, ':');
+	if (idx < 0) return "";
+	return p[idx+1 .. $];


you could make this a one-liner using findSplit: return p.findSplit(":")[2];.

imo it's actually a lit bit more readable (and it would be even more readable if findSplit returned a tuple with start, match and end members) but it's not really required. Actually also think the performance might be a little bit worse.

it's actually a lit bit more readable (and it would be even more readable if findSplit returned a tuple with start, match and end members)

Yeah I tried this once

dlang/phobos#5968

tl;dr: it was reverted due to a regression in CTFE usage of tuples.

That would cause a range error for package names without a sub package part, wouldn't it?

no findSplit returns a tuple, it is always exactly 3 long (pre, match, post) no matter the input

Ah, I actually missed the "split" part.

WebFreak001

I'm not really a fan of the basePackage and subPackage names and using auto everywhere where they are used. Kind of makes me think it's some member function of a struct. I think if we wrote string as type and/or renamed them to basePackageName and subPackageName it would be a bit more clearer.

I didn't really look at the old code too much when reviewing so I don't know if any functionality is lost (I saw that some debug logs were removed though).

Do we have test cases that we can include that were previously breaking dependency resolution but don't anymore?

WebFreak001 · 2018-07-10T09:13:58Z

source/dub/dependencyresolver.d

-		bool[TreeNode] visited;
+		// Leave the possibility to opt-out from the loop limit
+		import std.process : environment;
+		bool no_loop_limit = environment.get("DUB_NO_RESOLVE_LIMIT") !is null;


this new flag should be documented somewhere

It's not new, but yeah, it should still be documented.

WebFreak001 · 2018-07-10T09:16:26Z

source/dub/dependencyresolver.d

+		import std.bitmanip : BitArray;

-				configs[pidx] = config;
+		bool[string] visited; // key = sub package or empty string


while this is being rewritten, maybe we could consider making this a normal array (or some container like RedBlackTree) and add helper functions with readable names such as isVisited(string name) and visit(string name) that check if it is in the container and add it respectively.

WebFreak001 · 2018-07-10T09:20:12Z

source/dub/dependencyresolver.d


-				configs[pidx] = config;
+		bool[string] visited; // key = sub package or empty string
+		CONFIG[] configs;


Every time I look at this code again I get confused what CONFIG is supposed to be, being in PackageConfigs. Can we add a little bit of documentation here and to PackageConfigs describing what they are used for?

WebFreak001 · 2018-07-10T09:21:46Z

source/dub/dependencyresolver.d

-					package_names[pidx] = basepack;
+		// build up the dependency graph, eliminating as many configurations/
+		// versions as possible
+		PackageConfigs[string] packs;


should we maybe call this cache or visited instead to make more clear what packs are stored in this variable?

WebFreak001 · 2018-07-10T09:22:58Z

source/dub/dependencyresolver.d

+
+		foreach (dep; dependencies) {
+			// lazily load all dependency configurations
+			auto depbase = basePackage(dep.pack);


why aren't we using UFCS here like in the other places?

WebFreak001 · 2018-07-10T09:33:13Z

source/dub/dependencyresolver.d

+		try constrainDependencies(n, dependencies, 0, packs, loop_counter);
+		catch (ResolveException e) {
+			// add any relevant information for this stack frame
+			e.chain(n, dependencies);


+1, I like this way of adding dependencies to the error

WebFreak001 · 2018-07-10T09:34:38Z

source/dub/dependencyresolver.d

+		if (first_err) throw first_err;
+
+		// should have thrown in constrainRec before reaching this
+		assert(false, "Got no configuration for dependency!?");


just in case this actually gets triggered, including which package triggered this would probably be a good idea.

WebFreak001 · 2018-07-10T09:45:24Z

source/dub/dependencyresolver.d


-				foreach (v; config.allConfigs)
-					findConfigsRec(TreeNode(ch.pack, v), parent_unique && config.allConfigs.length == 1);
+	private void constrainRec(TreeNode n, ref PackageConfigs[string] packs, ref long loop_counter)


Why is there constrainRec and constrainDependencies? I would have guessed constrainRec stands for constrainRecursive by the name but that doesn't really make sense with constrainDependencies being there

constrainDependencies calls back into constrainRec, but also calls itself recursively. Probably makes sense to just drop the Rec suffix.

WebFreak001 · 2018-07-10T09:48:03Z

source/dub/dependencyresolver.d

+
+		auto dep = &dependencies[depidx];
+		auto depbase = dep.pack.basePackage;
+		auto dp = &packs[depbase];


these variable names tho

WebFreak001 · 2018-07-10T09:51:23Z

source/dub/dependencyresolver.d

-					if (configs[i].allConfigs.length)
-						cs[i] = p~" "~configs[i].allConfigs[config_indices[i]].to!string~(i >= 0 && i >= conflict_index ? " (C)" : "");
-					else cs[i] = p ~ " [no config]";
+	private void constrainDependencies(TreeNode n, TreeNodes[] dependencies, size_t depidx, ref PackageConfigs[string] packs, ref long loop_counter)


is the n argument even used in this function?

Now it's used in the assertion message at the bottom of the function ;-)

WebFreak001 · 2018-07-10T09:55:57Z

travis is failing because of the absolute paths in the test file, maybe we should use relative paths relative to the package root directory instead?

andre2007 · 2018-07-10T10:09:30Z

By chance, does this pull request has any influence on optional dependencies? (#1148)

John-Colvin · 2018-07-10T10:56:08Z

source/dub/dependencyresolver.d

+	can be defined in terms of a version range.
+*/
 class DependencyResolver(CONFIGS, CONFIG) {
+	/** Encasulates a list of outgoing edges in the dependency graph.


s/Encasulates/Encapsulates/

s-ludwig · 2018-07-10T10:57:43Z

@WebFreak001

Are there also changes in performance by this?

It definitely improves performance for complex graphs (which is the whole point of this of course). For simple graphs it is probably a bit slower due to the heavy memory allocation and copying, but it doesn't matter because we are talking about milliseconds or less then. Most of the time is currently spent in superfluous queries to the registry anyway, since the cache mechanism stopped working at some point.

s-ludwig · 2018-07-10T11:10:39Z

@andre2007

By chance, does this pull request has any influence on optional dependencies? (#1148)

It shouldn't affect that issue. A proper solution for that requires on a higher level approach, one that comes after the configurations have been determined and the project is about to be built.

s-ludwig · 2018-07-10T11:12:31Z

travis is failing because of the absolute paths in the test file, maybe we should use relative paths relative to the package root directory instead?

I just fixed the test for now, as outputting relative paths would be more complicated (relative to the CWD would be easier than relative to the root directory).

s-ludwig · 2018-07-10T11:20:33Z

Do we have test cases that we can include that were previously breaking dependency resolution but don't anymore?

The problem is that the known test cases depend on a rather large number of public packages. Capturing them in a local test case would be quite involved and just sourcing them from the registry is error prone and slows down the CI tests unnecessarily.

WebFreak001 · 2018-07-10T11:30:21Z

but if we do have one public test case where it happens, can we at least try locally if the issue happened before and if it is fixed?

s-ludwig · 2018-07-10T11:57:10Z

but if we do have one public test case where it happens, can we at least try locally if the issue happened before and if it is fixed?

#1300 does get fixed by this and is the only public test case that I found that still fails on master.

s-ludwig · 2018-07-10T14:21:44Z

I just noticed that not all sources end up in the conflict error message, because the stack doesn't generally encompass the whole dependency graph, so an additional map will be required to fix that.

s-ludwig · 2018-07-11T12:24:28Z

Refactored the code and fixed the log issue.

WebFreak001

Looking good now. Maybe a unittest using conflicting subpackages would make sense too

s-ludwig · 2018-07-11T18:05:56Z

You are right, those were definitely easy enough to add. I'll rebase/clean up once the CI is green again.

s-ludwig · 2018-07-13T09:12:01Z

Not sure what those CI failures are about. Travis looks fine and I can't spot the failure for Jenkins.

wilzbach · 2018-07-13T09:35:14Z

Not sure what those CI failures are about. Travis looks fine and I can't spot the failure for Jenkins.

We're currently migrating off from Jenkins to Buildkite due to these spurious failures. You can ignore them safely though we should probably see what Buildkite is telling us before merging this.

andre2007 · 2018-07-26T06:13:29Z

I tried this PR. While it seems to solve an issue for me, I found another issue. Maybe this issue is completely unrelated to this pr or maybe it is somehow affected.

I have a small app with dependency to vibe-d. I have to execute "dub" 4 times until all dependencies are resolved right. The first 3 times there are following errors:

First try:
core.exception.AssertError@source\dub\project.d(156): Non-optional dependency libasync of vibe-d:core not found in dependency tree!?.

Second try:
core.exception.AssertError@source\dub\project.d(156): Non-optional dependency memutils of libasync not found in dependency tree!?.

Third try:
core.exception.AssertError@source\dub\project.d(156): Non-optional dependency taggedalgebraic of eventcore not found in dependency tree!?.

My dub packages folder is completely empty while executing dub the first time. My assumption was, if a dependency 1 (vibe.d) depends on 2 (vibe-d:core) which has a dependency to 3 (libasync) then this will fail if dependency 2 and 3 aren't downloaded yet.
But this assumption is wrong. I left in dub packages folder "libasync-0.8.3" and removed all other packages and still the error is thrown.
core.exception.AssertError@source\dub\project.d(156): Non-optional dependency libasync of vibe-d:core not found in dependency tree!?.

MartinNowak · 2018-07-27T11:06:43Z

We're currently migrating off from Jenkins to Buildkite due to these spurious failures.

No, we're migrating from Jenkins cause it's using too many server-side resources.
Most "spurious" failures are caused by our test runners/caches/cleanup and are not the fault of Jenkins.

MartinNowak · 2018-08-06T14:38:25Z

Will we get this merged until next week for 2.082 @s-ludwig @wilzbach?
It's still missing a changelog entry.

s-ludwig · 2018-08-06T20:04:00Z

Still need to look into the issue @andre2007 mentioned, but I'll try to do it this week.

andre2007 · 2018-08-10T06:43:41Z

@s-ludwig I found the root cause for my issue. It is not related to your pr.
While the Dub Registry provider returns the list of the dependencies of a package in method fetchPackageRecipe, the Maven provider does not return the list of dependencies. (Dub registry provider has this information directly). Maven provider doesn't have this information at hand.
Maven provider would need to download the package archive, extract dub.json /dub.sdl and return the dependencies.
I am currently think about what is the best solution.
Sorry for the noise.

gedaiu · 2018-08-10T10:28:14Z

It would be really nice to have this in the next release.

I tested this branch on some of my projects and it worked good :) If it's needed, I can test it with more projects.

s-ludwig · 2018-08-11T19:16:11Z

Okay, good to hear, was just going to look into it myself. I guess someone can pull the trigger then.

WebFreak001 · 2018-08-11T19:41:34Z

no changelog entry?

s-ludwig · 2018-08-11T20:02:55Z

Of course... forgot about that.

s-ludwig · 2018-08-11T21:56:57Z

Added now.

Although this is rather expensive in terms of memory allocation and stack usage, it does a much better job at reducing the number of combinations that have to be tested.

… skipped.

WebFreak001 · 2018-08-12T20:14:21Z

tests pass, reviews agree, feel like it's time to merge

s-ludwig requested review from MartinNowak and WebFreak001 July 10, 2018 08:32

WebFreak001 reviewed Jul 10, 2018

View reviewed changes

John-Colvin reviewed Jul 10, 2018

View reviewed changes

s-ludwig force-pushed the unify_dependency_resolution branch from eaed55e to 0af430c Compare July 10, 2018 11:39

WebFreak001 approved these changes Jul 11, 2018

View reviewed changes

s-ludwig force-pushed the unify_dependency_resolution branch 2 times, most recently from bcd8838 to 6307538 Compare July 11, 2018 19:57

wilzbach closed this Jul 13, 2018

wilzbach reopened this Jul 13, 2018

wilzbach force-pushed the unify_dependency_resolution branch from 98e733e to fbfd5dc Compare July 13, 2018 09:36

WebFreak001 mentioned this pull request Jul 17, 2018

DLangIDE fails to build #1515

Closed

andre2007 mentioned this pull request Jul 26, 2018

Endless loop while retrieving dependencies in multi registries scenario #1519

Closed

wilzbach mentioned this pull request Jul 27, 2018

Fix Issue 19107 - -de produces compilation error, -dw does not dlang/dmd#8519

Merged

AntonMeep approved these changes Aug 6, 2018

View reviewed changes

s-ludwig added 5 commits August 12, 2018 09:12

Implement a recursion based dependency resolution algorithm.

7116171

Although this is rather expensive in terms of memory allocation and stack usage, it does a much better job at reducing the number of combinations that have to be tested.

Adjust test for the changed error messages

d095be5

Add unit tests for dependency resolution error messages.

a4a764b

Fix unresolved optional dependencies causing other dependencies to be…

b006077

… skipped.

Add change log entry

98161aa

s-ludwig force-pushed the unify_dependency_resolution branch from 57b01f4 to 98161aa Compare August 12, 2018 07:13

WebFreak001 added the auto-merge label Aug 12, 2018

dlang-bot merged commit cbc32ff into master Aug 12, 2018

s-ludwig deleted the unify_dependency_resolution branch August 12, 2018 20:15

wilzbach mentioned this pull request Oct 28, 2018

Bump the host compiler to 2.082.1 dlang/dlang.org#2497

Merged

Uh oh!

Reimplement dependency resolution as a recursive process #1513

Reimplement dependency resolution as a recursive process #1513

Uh oh!

Conversation

s-ludwig commented Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Jul 10, 2018

Uh oh!

WebFreak001 commented Jul 10, 2018

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!

WebFreak001 left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WebFreak001 commented Jul 10, 2018

Uh oh!

andre2007 commented Jul 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-ludwig commented Jul 10, 2018

Uh oh!

s-ludwig commented Jul 10, 2018

Uh oh!

s-ludwig commented Jul 10, 2018

Uh oh!

s-ludwig commented Jul 10, 2018

Uh oh!

WebFreak001 commented Jul 10, 2018

Uh oh!

s-ludwig commented Jul 10, 2018

Uh oh!

s-ludwig commented Jul 10, 2018

Uh oh!

s-ludwig commented Jul 11, 2018

Uh oh!

WebFreak001 left a comment

Choose a reason for hiding this comment

Uh oh!

s-ludwig commented Jul 11, 2018

Uh oh!

s-ludwig commented Jul 10, 2018 •

edited

Loading

wilzbach commented Jul 13, 2018 •

edited

Loading