Merge pull request #174 from aszady/bestkey/arch

Consider the architecture when choosing the best package variant

Author: kellyma2

pkg/sat/loader.go

@@ -1,6 +1,7 @@
 package sat
 
 import (
+	"cmp"
 	"fmt"
 	"regexp"
 	"sort"
@@ -22,12 +23,32 @@ type Loader struct {
 	varsCount int
 }
 
+// BestKey groups packages for the purpose of `--nobest` option disabled,
+// so that for each set of packages sharing this key, only the "best" package will be preserved
+// (best in terms of repo priority, version criteria).
+type BestKey struct {
+	name string
+	arch string
+}
+
+// CompareBestKey provides an arbitrary, deterministic, total order on BestKey
+func CompareBestKey(k1 BestKey, k2 BestKey) int {
+	return cmp.Or(
+		cmp.Compare(k1.name, k2.name),
+		cmp.Compare(k1.arch, k2.arch),
+	)
+}
+
+func MakeBestKey(pkg *api.Package) BestKey {
+	return BestKey{name: pkg.Name, arch: pkg.Arch}
+}
+
 func NewLoader() *Loader {
 	return &Loader{
 		m: &Model{
 			packages:                    map[string][]*Var{},
 			vars:                        map[string]*Var{},
-			bestPackages:                map[string]*api.Package{},
+			bestPackages:                map[BestKey]*api.Package{},
 			forceIgnoreWithDependencies: map[api.PackageKey]*api.Package{},
 		},
 		provides:  map[string][]*Var{},
@@ -97,17 +118,18 @@ func (loader *Loader) Load(packages []*api.Package, matched, ignoreRegex, allowR
 
 	// Create an index to pick the best candidates
 	for _, pkg := range packages {
-		if loader.m.bestPackages[pkg.Name] == nil {
-			loader.m.bestPackages[pkg.Name] = pkg
-		} else if rpm.ComparePackage(pkg, loader.m.bestPackages[pkg.Name], archOrder) > 0 {
-			loader.m.bestPackages[pkg.Name] = pkg
+		key := MakeBestKey(pkg)
+		if loader.m.bestPackages[key] == nil {
+			loader.m.bestPackages[key] = pkg
+		} else if rpm.ComparePackage(pkg, loader.m.bestPackages[key], archOrder) > 0 {
+			loader.m.bestPackages[key] = pkg
 		}
 	}
 
 	if !nobest {
 		packages = nil
 		bestPackagesKeys := maps.Keys(loader.m.bestPackages)
-		slices.Sort(bestPackagesKeys)
+		slices.SortFunc(bestPackagesKeys, CompareBestKey)
 		for _, v := range bestPackagesKeys {
 			packages = append(packages, loader.m.bestPackages[v])
 		}

pkg/sat/loader_test.go

@@ -38,7 +38,7 @@ func expectedBest(g *WithT, m *Model, pkgVersions map[string]string) {
 	g.Expect(m.bestPackages).To(HaveLen(len(pkgVersions)))
 
 	for k, version := range pkgVersions {
-		g.Expect(m.bestPackages[k].Version.String()).To(Equal(version))
+		g.Expect(m.bestPackages[BestKey{name: k}].Version.String()).To(Equal(version))
 	}
 }
 

pkg/sat/sat.go

@@ -54,7 +54,7 @@ type Model struct {
 	// vars contain as key an exact identifier for a provided resource and the actual SAT variable as value
 	vars map[string]*Var
 
-	bestPackages map[string]*api.Package
+	bestPackages map[BestKey]*api.Package
 
 	ands                        []bf.Formula
 	forceIgnoreWithDependencies map[api.PackageKey]*api.Package
@@ -68,8 +68,8 @@ func (m *Model) Var(v string) *Var {
 	return m.vars[v]
 }
 
-func (m *Model) BestPackage(p string) *api.Package {
-	return m.bestPackages[p]
+func (m *Model) BestPackage(k BestKey) *api.Package {
+	return m.bestPackages[k]
 }
 
 func (m *Model) Ands() bf.Formula {
@@ -186,8 +186,10 @@ func Resolve(model *Model) (install []*api.Package, excluded []*api.Package, for
 			}
 		}
 		for _, v := range installMap {
-			if rpm.Compare(model.BestPackage(v.Name).Version, v.Version) != 0 {
-				logrus.Infof("Picking %v instead of best candiate %v", v, model.BestPackage(v.Name))
+			key := MakeBestKey(v)
+			bestPackage := model.BestPackage(key)
+			if bestPackage != v {
+				logrus.Infof("Picking %v instead of best candiate %v", v, bestPackage)
 			}
 			install = append(install, v)
 		}

pkg/sat/sat_test.go

@@ -1429,7 +1429,7 @@ func TestNewResolver(t *testing.T) {
 			"testa",
 		},
 			install:  []string{"testa-0:2.x86_64"},
-			exclude:  []string{},
+			exclude:  []string{"testa-0:3.noarch"},
 			solvable: true,
 		},
 		{name: "prioritize dependency: best arch & version", packages: []*api.Package{
@@ -1443,7 +1443,7 @@ func TestNewResolver(t *testing.T) {
 			"testa",
 		},
 			install:  []string{"testa-0:1.noarch", "testb-0:2.x86_64"},
-			exclude:  []string{},
+			exclude:  []string{"testb-0:3.noarch"},
 			solvable: true,
 		},
 		{name: "cross-arch dependency (by name and by resource)", packages: []*api.Package{
@@ -1478,9 +1478,21 @@ func TestNewResolver(t *testing.T) {
 			"testa",
 		},
 			install:  []string{"testa-0:1.x86_64", "testb-0:1.x86_64"},
-			exclude:  []string{},
+			exclude:  []string{"testa-0:1.noarch", "testb-0:1.noarch"},
 			solvable: true,
 		},
+		{name: "dependency on non-primary architecture", packages: []*api.Package{
+			newPkgAP("testa", "1", "noarch", 1, []string{}, []string{"/usr/lib/libb.so"}, []string{}),
+			newPkgAP("testb", "1", "x86_64", 1, []string{"/usr/lib64/libb.so"}, []string{}, []string{}),
+			newPkgAP("testb", "1", "i686", 1, []string{"/usr/lib/libb.so"}, []string{}, []string{}),
+		}, requires: []string{
+			"testa",
+		},
+			architectures: []string{"x86_64", "i686"},
+			install:       []string{"testa-0:1.noarch", "testb-0:1.i686"},
+			exclude:       []string{"testb-0:1.x86_64"},
+			solvable:      true,
+		},
 
 		// TODO: Add test cases.
 	}