From 69f564aa5fe3dcc35b45cda698d991f618889fa1 Mon Sep 17 00:00:00 2001
From: sam boyer <tech@samboyer.org>
Date: Tue, 24 Jan 2017 08:21:08 -0500
Subject: [PATCH] Allow project-level cycles involving root project
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Fixes #151.

♬ : Anna Kendrick / Cups (Pitch Perfect’s “When I’m Gone”)
---
 bridge.go             |  2 +-
 rootdata.go           | 11 +++++++++--
 selection.go          | 14 ++------------
 solve_bimodal_test.go |  7 +++----
 solver.go             |  7 ++++++-
 5 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/bridge.go b/bridge.go
index 34945dc..222b372 100644
--- a/bridge.go
+++ b/bridge.go
@@ -284,7 +284,7 @@ func (b *bridge) vtu(id ProjectIdentifier, v Version) versionTypeUnion {
 // responsible for that code.
 func (b *bridge) ListPackages(id ProjectIdentifier, v Version) (PackageTree, error) {
 	if b.s.rd.isRoot(id.ProjectRoot) {
-		panic("should never call ListPackages on root project")
+		return b.s.rd.rpt, nil
 	}
 
 	b.s.mtr.push("b-list-pkgs")
diff --git a/rootdata.go b/rootdata.go
index c8ae5ac..af075b2 100644
--- a/rootdata.go
+++ b/rootdata.go
@@ -141,12 +141,17 @@ func (rd rootdata) combineConstraints() []workingConstraint {
 
 // needVersionListFor indicates whether we need a version list for a given
 // project root, based solely on general solver inputs (no constraint checking
-// required). This will be true if any of the following conditions hold:
+// required). Assuming the argument is not the root project itself, this will be
+// true if any of the following conditions hold:
 //
 //  - ChangeAll is on
 //  - The project is not in the lock
 //  - The project is in the lock, but is also in the list of projects to change
 func (rd rootdata) needVersionsFor(pr ProjectRoot) bool {
+	if rd.isRoot(pr) {
+		return false
+	}
+
 	if rd.chngall {
 		return true
 	}
@@ -154,7 +159,9 @@ func (rd rootdata) needVersionsFor(pr ProjectRoot) bool {
 	if _, has := rd.rlm[pr]; !has {
 		// not in the lock
 		return true
-	} else if _, has := rd.chng[pr]; has {
+	}
+
+	if _, has := rd.chng[pr]; has {
 		// in the lock, but marked for change
 		return true
 	}
diff --git a/selection.go b/selection.go
index 7f03c51..cab3e77 100644
--- a/selection.go
+++ b/selection.go
@@ -82,12 +82,7 @@ func (s *selection) getRequiredPackagesIn(id ProjectIdentifier) map[string]int {
 	uniq := make(map[string]int)
 	for _, dep := range s.deps[id.ProjectRoot] {
 		for _, pkg := range dep.dep.pl {
-			if count, has := uniq[pkg]; has {
-				count++
-				uniq[pkg] = count
-			} else {
-				uniq[pkg] = 1
-			}
+			uniq[pkg] = uniq[pkg] + 1
 		}
 	}
 
@@ -105,12 +100,7 @@ func (s *selection) getSelectedPackagesIn(id ProjectIdentifier) map[string]int {
 	for _, p := range s.projects {
 		if p.a.a.id.eq(id) {
 			for _, pkg := range p.a.pl {
-				if count, has := uniq[pkg]; has {
-					count++
-					uniq[pkg] = count
-				} else {
-					uniq[pkg] = 1
-				}
+				uniq[pkg] = uniq[pkg] + 1
 			}
 		}
 	}
diff --git a/solve_bimodal_test.go b/solve_bimodal_test.go
index be53c3f..cf66740 100644
--- a/solve_bimodal_test.go
+++ b/solve_bimodal_test.go
@@ -291,16 +291,15 @@ var bimodalFixtures = map[string]bimodalFixture{
 				pkg("root", "a"),
 			),
 			dsp(mkDepspec("a 1.0.0"),
-				pkg("a"),
+				pkg("a", "b"),
 				pkg("a/foo"),
 			),
 			dsp(mkDepspec("b 1.0.0"),
-				pkg("b", "b/baz"),
-				pkg("b/baz", "a/foo"),
+				pkg("b", "a/foo"),
 			),
 		},
 		r: mksolution(
-			"a 1.0.0",
+			mklp("a 1.0.0", ".", "foo"),
 			"b 1.0.0",
 		),
 	},
diff --git a/solver.go b/solver.go
index d75166e..a039e4c 100644
--- a/solver.go
+++ b/solver.go
@@ -455,7 +455,6 @@ func (s *solver) solve() (map[atom]map[string]struct{}, error) {
 func (s *solver) selectRoot() error {
 	s.mtr.push("select-root")
 	// Push the root project onto the queue.
-	// TODO(sdboyer) maybe it'd just be better to skip this?
 	awp := s.rd.rootAtom()
 	s.sel.pushSelection(awp, true)
 
@@ -1063,6 +1062,12 @@ func (s *solver) selectAtom(a atomWithPackages, pkgonly bool) {
 	}
 
 	for _, dep := range deps {
+		// Root can come back up here if there's a project-level cycle.
+		// Satisfiability checks have already ensured invariants are maintained,
+		// so we know we can just skip it here.
+		if s.rd.isRoot(dep.Ident.ProjectRoot) {
+			continue
+		}
 		// If this is dep isn't in the lock, do some prefetching. (If it is, we
 		// might be able to get away with zero network activity for it, so don't
 		// prefetch). This provides an opportunity for some parallelism wins, on