Merge pull request #2170 from infosiftr/fix-bashbrew-build-sorting

Move "build order" sorting functions into a common file and refactor them to use topsort properly and via a single unified function (single node per logical "thing" we're sorting, resolve tag names to canonical node names appropriately)

go/src/bashbrew/main.go

@@ -4,11 +4,8 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
-	"sort"
-	"strings"
 
 	"github.com/codegangsta/cli"
-	"pault.ag/go/topsort"
 )
 
 // TODO somewhere, ensure that the Docker engine we're talking to is API version 1.22+ (Docker 1.10+)
@@ -52,91 +49,6 @@ func initDefaultCachePath() string {
 	return filepath.Join(xdgCache, "bashbrew")
 }
 
-func repos(all bool, args ...string) ([]string, error) {
-	ret := []string{}
-
-	if all {
-		dir, err := os.Open(defaultLibrary)
-		if err != nil {
-			return nil, err
-		}
-		names, err := dir.Readdirnames(-1)
-		dir.Close()
-		if err != nil {
-			return nil, err
-		}
-		sort.Strings(names)
-		for _, name := range names {
-			ret = append(ret, filepath.Join(defaultLibrary, name))
-		}
-	}
-
-	ret = append(ret, args...)
-
-	if len(ret) < 1 {
-		return nil, fmt.Errorf(`need at least one repo (either explicitly or via "--all")`)
-	}
-
-	return ret, nil
-}
-
-func sortRepos(repos []string) ([]string, error) {
-	if noSortFlag || len(repos) <= 1 {
-		return repos, nil
-	}
-
-	network := topsort.NewNetwork()
-
-	rs := []*Repo{}
-	for _, repo := range repos {
-		r, err := fetch(repo)
-		if err != nil {
-			return nil, err
-		}
-		rs = append(rs, r)
-		network.AddNode(r.Identifier(), repo)
-		network.AddNode(r.RepoName, repo)
-	}
-
-	for _, r := range rs {
-		for _, entry := range r.Entries() {
-			from, err := r.DockerFrom(&entry)
-			if err != nil {
-				return nil, err
-			}
-			if i := strings.IndexRune(from, ':'); i >= 0 {
-				// we want "repo -> repo" relations, no tags
-				from = from[:i]
-			}
-			if from == r.RepoName {
-				// "a:a -> a:b" is OK (ignore that here -- see Repo.SortedEntries for that)
-				continue
-			}
-			// TODO somehow reconcile/avoid "a:a -> b:b, b:b -> a:c" (which will exhibit here as cyclic)
-			network.AddEdgeIfExists(from, r.Identifier())
-			network.AddEdgeIfExists(from, r.RepoName)
-		}
-	}
-
-	nodes, err := network.Sort()
-	if err != nil {
-		return nil, err
-	}
-
-	ret := []string{}
-	seen := map[string]bool{}
-	for _, node := range nodes {
-		repo := node.Value.(string)
-		if seen[repo] {
-			continue
-		}
-		seen[repo] = true
-		ret = append(ret, repo)
-	}
-
-	return ret, nil
-}
-
 func main() {
 	app := cli.NewApp()
 	app.Name = "bashbrew"

go/src/bashbrew/repo.go

@@ -4,12 +4,41 @@ import (
 	"fmt"
 	"os"
 	"path"
+	"path/filepath"
+	"sort"
 	"strings"
 
 	"github.com/docker-library/go-dockerlibrary/manifest"
-	"pault.ag/go/topsort"
 )
 
+func repos(all bool, args ...string) ([]string, error) {
+	ret := []string{}
+
+	if all {
+		dir, err := os.Open(defaultLibrary)
+		if err != nil {
+			return nil, err
+		}
+		names, err := dir.Readdirnames(-1)
+		dir.Close()
+		if err != nil {
+			return nil, err
+		}
+		sort.Strings(names)
+		for _, name := range names {
+			ret = append(ret, filepath.Join(defaultLibrary, name))
+		}
+	}
+
+	ret = append(ret, args...)
+
+	if len(ret) < 1 {
+		return nil, fmt.Errorf(`need at least one repo (either explicitly or via "--all")`)
+	}
+
+	return ret, nil
+}
+
 func latestizeRepoTag(repoTag string) string {
 	if repoTag != "scratch" && strings.IndexRune(repoTag, ':') < 0 {
 		return repoTag + ":latest"
@@ -25,8 +54,8 @@ type Repo struct {
 }
 
 func (r Repo) Identifier() string {
-	if r.TagName != "" {
-		return r.RepoName + ":" + r.TagName
+	if r.TagEntry != nil {
+		return r.EntryIdentifier(*r.TagEntry)
 	}
 	return r.RepoName
 }
@@ -35,6 +64,16 @@ func (r Repo) EntryIdentifier(entry manifest.Manifest2822Entry) string {
 	return r.RepoName + ":" + entry.Tags[0]
 }
 
+// create a new "Repo" object representing a single "Manifest2822Entry" object
+func (r Repo) EntryRepo(entry *manifest.Manifest2822Entry) *Repo {
+	return &Repo{
+		RepoName: r.RepoName,
+		TagName:  entry.Tags[0],
+		Manifest: r.Manifest,
+		TagEntry: entry,
+	}
+}
+
 func (r Repo) SkipConstraints(entry manifest.Manifest2822Entry) bool {
 	repoTag := r.RepoName + ":" + entry.Tags[0]
 
@@ -87,51 +126,6 @@ func (r Repo) Entries() []manifest.Manifest2822Entry {
 	}
 }
 
-func (r Repo) SortedEntries() ([]manifest.Manifest2822Entry, error) {
-	entries := r.Entries()
-
-	if noSortFlag || len(entries) <= 1 {
-		return entries, nil
-	}
-
-	network := topsort.NewNetwork()
-
-	for i, entry := range entries {
-		for _, tag := range r.Tags("", false, entry) {
-			network.AddNode(tag, &entries[i])
-		}
-	}
-
-	for _, entry := range entries {
-		from, err := r.DockerFrom(&entry)
-		if err != nil {
-			return nil, err
-		}
-		for _, tag := range r.Tags("", false, entry) {
-			network.AddEdgeIfExists(from, tag)
-		}
-	}
-
-	nodes, err := network.Sort()
-	if err != nil {
-		return nil, err
-	}
-
-	seen := map[*manifest.Manifest2822Entry]bool{}
-	ret := []manifest.Manifest2822Entry{}
-	for _, node := range nodes {
-		entry := node.Value.(*manifest.Manifest2822Entry)
-		if seen[entry] {
-			// TODO somehow reconcile "a:a -> b:b, b:b -> a:c"
-			continue
-		}
-		ret = append(ret, *entry)
-		seen[entry] = true
-	}
-
-	return ret, nil
-}
-
 func (r Repo) Tags(namespace string, uniq bool, entry manifest.Manifest2822Entry) []string {
 	tagRepo := path.Join(namespace, r.RepoName)
 	ret := []string{}

go/src/bashbrew/sort.go

@@ -0,0 +1,135 @@
+package main
+
+import (
+	"github.com/docker-library/go-dockerlibrary/manifest"
+	"pault.ag/go/topsort"
+)
+
+func sortRepos(repos []string) ([]string, error) {
+	rs := []*Repo{}
+	rsMap := map[*Repo]string{}
+	for _, repo := range repos {
+		r, err := fetch(repo)
+		if err != nil {
+			return nil, err
+		}
+		if _, ok := rsMap[r]; ok {
+			// if we have a duplicate, let's prefer the first
+			continue
+		}
+		rs = append(rs, r)
+		rsMap[r] = repo
+	}
+
+	// short circuit if we don't have to go further
+	if noSortFlag || len(repos) <= 1 {
+		return repos, nil
+	}
+
+	rs, err := sortRepoObjects(rs)
+	if err != nil {
+		return nil, err
+	}
+
+	ret := []string{}
+	for _, r := range rs {
+		ret = append(ret, rsMap[r])
+	}
+	return ret, nil
+}
+
+func (r Repo) SortedEntries() ([]manifest.Manifest2822Entry, error) {
+	entries := r.Entries()
+
+	// short circuit if we don't have to go further
+	if noSortFlag || len(entries) <= 1 {
+		return entries, nil
+	}
+
+	// create individual "Repo" objects for each entry in "r" so they can be sorted by the same "sortRepoObjects" function
+	rs := []*Repo{}
+	for i := range entries {
+		rs = append(rs, r.EntryRepo(&entries[i]))
+	}
+
+	rs, err := sortRepoObjects(rs)
+	if err != nil {
+		return nil, err
+	}
+
+	ret := []manifest.Manifest2822Entry{}
+	for _, entryR := range rs {
+		ret = append(ret, *entryR.TagEntry)
+	}
+	return ret, nil
+}
+
+func sortRepoObjects(rs []*Repo) ([]*Repo, error) {
+	// short circuit if we don't have to go further
+	if noSortFlag || len(rs) <= 1 {
+		return rs, nil
+	}
+
+	network := topsort.NewNetwork()
+
+	// a map of alternate tag names to the canonical "node name" for topsort purposes
+	canonicalNodes := map[string]string{}
+	canonicalRepos := map[string]*Repo{}
+
+	for _, r := range rs {
+		node := r.Identifier()
+		for _, entry := range r.Entries() {
+			for _, tag := range r.Tags("", false, entry) {
+				if canonicalRepo, ok := canonicalRepos[tag]; ok && canonicalRepo.TagName != "" {
+					// if we run into a duplicate, we want to prefer a specific tag over a full repo
+					continue
+				}
+
+				canonicalNodes[tag] = node
+				canonicalRepos[tag] = r
+			}
+		}
+		network.AddNode(node, r)
+	}
+
+	for _, r := range rs {
+		for _, entry := range r.Entries() {
+			from, err := r.DockerFrom(&entry)
+			if err != nil {
+				return nil, err
+			}
+			from = latestizeRepoTag(from)
+
+			fromNode, ok := canonicalNodes[from]
+			if !ok {
+				// if our FROM isn't in the list of things we're sorting, it isn't relevant in this context
+				continue
+			}
+
+			// TODO somehow reconcile/avoid "a:a -> b:b, b:b -> a:c" (which will exhibit here as cyclic)
+			for _, tag := range r.Tags("", false, entry) {
+				if tagNode, ok := canonicalNodes[tag]; ok {
+					if tagNode == fromNode {
+						// don't be cyclic
+						continue
+					}
+					if err := network.AddEdge(fromNode, tagNode); err != nil {
+						return nil, err
+					}
+				}
+			}
+		}
+	}
+
+	nodes, err := network.Sort()
+	if err != nil {
+		return nil, err
+	}
+
+	ret := []*Repo{}
+	for _, node := range nodes {
+		ret = append(ret, node.Value.(*Repo))
+	}
+
+	return ret, nil
+}