Skip to content

Merge SAT vars for each package #153

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

Draft
aszady wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Merge SAT vars for each package #153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
91cfd90
Add `Resource` abstraction to the SAT loader
aszady Jan 23, 2026
3990d92
Wrap Var into ResourceVar
aszady Jan 23, 2026
3f468a3
Remove fields from Var
aszady Jan 23, 2026
288e949
Merge SAT vars for each package
aszady Sep 26, 2025
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
121 changes: 47 additions & 74 deletions pkg/sat/loader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"cmp"
"fmt"
"regexp"
"sort"
"strconv"
"strings"

Expand All @@ -19,7 +18,7 @@ import (

type Loader struct {
m *Model
provides map[string][]*Var
provides map[string][]*ResourceVar
varsCount int
}

Expand Down Expand Up @@ -51,11 +50,25 @@ func NewLoader() *Loader {
bestPackages: map[BestKey]*api.Package{},
forceIgnoreWithDependencies: map[api.PackageKey]*api.Package{},
},
provides: map[string][]*Var{},
provides: map[string][]*ResourceVar{},
varsCount: 0,
}
}

// Resource is a convenience abstraction over
// `api.Entry` and `api.ProvidedFile`
// that captures only the necessary information we need
type Resource struct {
Name string // capability name, or file name
Version api.Version // empty for files
}

// ResourceVar encapsulates a Resource for which we created a Var in our model.
type ResourceVar struct {
Var *Var
Resource Resource
}

// Load takes a list of all involved packages to install, a list of regular
// expressions which denote packages which should be taken into account for
// solving the problem, but they should then be ignored together with their
Expand Down Expand Up @@ -135,40 +148,29 @@ func (loader *Loader) Load(packages []*api.Package, matched, ignoreRegex, allowR
}
}

pkgProvides := [][]*Var{}
pkgVars := []*Var{}

// Generate variables
for _, pkg := range packages {
pkgVar, resourceVars := loader.explodePackageToVars(pkg)
// Single variable for SAT formula (whether to pick that package or not):
pkgVar := &Var{satVarName: loader.ticket(), Package: pkg}
loader.m.packages[pkg.Name] = append(loader.m.packages[pkg.Name], pkgVar)
pkgProvides = append(pkgProvides, resourceVars)
for _, v := range resourceVars {
loader.provides[v.Context.Provides] = append(loader.provides[v.Context.Provides], v)
loader.m.vars[v.satVarName] = v
}
}

packagesKeys := maps.Keys(loader.m.packages)
slices.Sort(packagesKeys)
loader.m.vars[pkgVar.satVarName] = pkgVar
pkgVars = append(pkgVars, pkgVar)

for _, x := range packagesKeys {
sort.SliceStable(loader.m.packages[x], func(i, j int) bool {
return rpm.ComparePackage(loader.m.packages[x][i].Package, loader.m.packages[x][j].Package, archOrder) < 0
})
// Links between packages (individual resources), only for the loader:
for _, resource := range loader.explodeProvidedResources(pkgVar.Package) {
prVar := &ResourceVar{Var: pkgVar, Resource: *resource}
loader.provides[resource.Name] = append(loader.provides[resource.Name], prVar)
}
}

logrus.Infof("Loaded %v packages.", len(pkgProvides))
logrus.Infof("Loaded %v packages.", len(pkgVars))

// Generate imply rules
for _, resourceVars := range pkgProvides {
for _, pkgVar := range pkgVars {
var ands []bf.Formula

// Synchronize all the variables for a given package to the same value.
pkgVar := resourceVars[len(resourceVars)-1]
for _, res := range resourceVars {
ands = append(ands, bf.Eq(bf.Var(pkgVar.satVarName), bf.Var(res.satVarName)))
}

ands = append(ands, bf.Implies(bf.Var(pkgVar.satVarName), loader.explodePackageRequires(pkgVar)))
if conflicts := loader.explodePackageConflicts(pkgVar); conflicts != nil {
ands = append(ands, bf.Implies(bf.Var(pkgVar.satVarName), bf.Not(conflicts)))
Expand All @@ -184,53 +186,24 @@ func (loader *Loader) Load(packages []*api.Package, matched, ignoreRegex, allowR
return loader.constructRequirements(matched, archOrder)
}

func (loader *Loader) explodePackageToVars(pkg *api.Package) (pkgVar *Var, resourceVars []*Var) {
// explodeProvidedResources collects all resources a `pkg` can provide (package, capabilities, files)
// and returns them in unified form of Resource.
func (loader *Loader) explodeProvidedResources(pkg *api.Package) (provided []*Resource) {

for _, p := range pkg.Format.Provides.Entries {
if p.Name == pkg.Name {
pkgVar = &Var{
satVarName: loader.ticket(),
varType: VarTypePackage,
Context: VarContext{
PackageKey: pkg.Key(),
Provides: pkg.Name,
},
Package: pkg,
ResourceVersion: &pkg.Version,
}
resourceVars = append(resourceVars, pkgVar)
} else {
resVar := &Var{
satVarName: loader.ticket(),
varType: VarTypeResource,
Context: VarContext{
PackageKey: pkg.Key(),
Provides: p.Name,
},
ResourceVersion: &api.Version{
Rel: p.Rel,
Ver: p.Ver,
Epoch: p.Epoch,
},
Package: pkg,
}
resourceVars = append(resourceVars, resVar)
}
provided = append(provided, &Resource{
Name: p.Name,
Version: api.Version{p.Text, p.Epoch, p.Ver, p.Rel},
})
}

for _, f := range pkg.Format.Files {
resVar := &Var{
satVarName: loader.ticket(),
varType: VarTypeFile,
Context: VarContext{
PackageKey: pkg.Key(),
Provides: f.Text,
},
Package: pkg,
ResourceVersion: &api.Version{},
}
resourceVars = append(resourceVars, resVar)
provided = append(provided, &Resource{
Name: f.Text,
})
}
return pkgVar, resourceVars

return
}

// explodePackageRequires builds a formula that could be a right hand side operand to implication.
Expand Down Expand Up @@ -353,14 +326,14 @@ func (loader *Loader) resolveNewest(pkgName string, archOrder []string) (*Var, e
}
newest := pkgs[0]
for _, p := range pkgs {
if rpm.ComparePackage(p.Package, newest.Package, archOrder) > 0 {
if rpm.ComparePackage(p.Var.Package, newest.Var.Package, archOrder) > 0 {
newest = p
}
}
return newest, nil
return newest.Var, nil
}

func compareRequires(entry api.Entry, provides []*Var) (accepts []*Var, err error) {
func compareRequires(entry api.Entry, provides []*ResourceVar) (accepts []*Var, err error) {
for _, dep := range provides {
entryVer := api.Version{
Text: entry.Text,
Expand All @@ -370,7 +343,7 @@ func compareRequires(entry api.Entry, provides []*Var) (accepts []*Var, err erro
}

// Requirement "EQ 2.14" matches 2.14-5.fc33
depVer := *dep.ResourceVersion
depVer := dep.Resource.Version
if entryVer.Rel == "" {
depVer.Rel = ""
}
Expand Down Expand Up @@ -408,13 +381,13 @@ func compareRequires(entry api.Entry, provides []*Var) (accepts []*Var, err erro
works = true
}
case "":
return provides, nil
works = true
default:
return nil, fmt.Errorf("can't interprate flags value %s", entry.Flags)
}
}
if works {
accepts = append(accepts, dep)
accepts = append(accepts, dep.Var)
}
}
return accepts, nil
Expand Down
66 changes: 26 additions & 40 deletions pkg/sat/loader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,6 @@ func TestLoader_Load(t *testing.T) {
x3 := bf.Var("x3")
x4 := bf.Var("x4")
x5 := bf.Var("x5")
x6 := bf.Var("x6")

t.Run("Trivial Loading", func(t *testing.T) {
model, _ := doSimpleLoad([]*api.Package{}, false)
Expand All @@ -190,7 +189,7 @@ func TestLoader_Load(t *testing.T) {
expectedPackages(g, model, map[string][]string{
"A": []string{"0:1.0-1"},
})
expectedVars(g, model, "A-0:1.0-1(A)")
expectedVars(g, model, "A-0:1.0-1")
Copy link
Copy Markdown
Collaborator

@manuelnaranjo manuelnaranjo Oct 25, 2025

Choose a reason for hiding this comment

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

This parenthesis requirement duplicates appears on real RPM requires, I feel dropping this from tests may actually break real resolution
Why are you dropping them?

Copy link
Copy Markdown
Contributor Author

@aszady aszady Oct 27, 2025

Choose a reason for hiding this comment

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

Not sure what do you mean :(

This follows directly from the change in func (v Var) String() string, as Var no longer has info about specific resources the package provides.
Is there some case not covered by tests?

Copy link
Copy Markdown
Collaborator

@manuelnaranjo manuelnaranjo Oct 28, 2025

Choose a reason for hiding this comment

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

This is what I mean https://g.co/gemini/share/f453cd2e46af, as you can see an RPM may say it depends on feature(modifier) I think we're dropping the modifier part with this change set, we will only notice the breaking change at runtime not while rendering the required RPMs

Copy link
Copy Markdown
Contributor Author

@aszady aszady Oct 28, 2025

Choose a reason for hiding this comment

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

I think that at no point bazeldnf parses the content of provides/requires/conflicts name, so there's no place to drop that modifier.

What this change does for e.g. glibc-related SAT variables, would be this transformation of a debug string for Var struct. For example:
SAT variables:

  • glibc(libc.so.6(GLIBC_2.16))
  • glibc(libc.so.6(GLIBC_2.17))
  • …
    will be replaced by single one:
  • glibc.

The info about particular resource provided (e.g. libc.so.6(GLIBC_2.16)) is now processed only at the previous stage of SAT construction – in the ProvidedResource struct.

The intention of this change is to keep behaviour the same, while simplifying the internals.

Copy link
Copy Markdown
Collaborator

@kellyma2 kellyma2 Jan 23, 2026

Choose a reason for hiding this comment

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

I have to echo @manuelnaranjo 's comment - it looks like in some of the later tests we're previously expecting multiple distinct qualifiers for the same package? The belief is that theyre not being used?

expectedBest(g, model, map[string]string{"A": "0:1.0-1"})
expectedIgnores(g, model)
expectedAnds(g, model,
Expand All @@ -206,7 +205,7 @@ func TestLoader_Load(t *testing.T) {
expectedPackages(g, model, map[string][]string{
"A": []string{"0:2.0-1"},
})
expectedVars(g, model, "A-0:2.0-1(A)")
expectedVars(g, model, "A-0:2.0-1")
expectedBest(g, model, map[string]string{"A": "0:2.0-1"})
expectedIgnores(g, model)
expectedAnds(g, model,
Expand All @@ -222,7 +221,7 @@ func TestLoader_Load(t *testing.T) {
expectedPackages(g, model, map[string][]string{
"A": []string{"0:1.0-1", "0:2.0-1"},
})
expectedVars(g, model, "A-0:1.0-1(A)", "A-0:2.0-1(A)")
expectedVars(g, model, "A-0:1.0-1", "A-0:2.0-1")
expectedBest(g, model, map[string]string{"A": "0:2.0-1"})
expectedIgnores(g, model)
expectedAnds(g, model,
Expand All @@ -245,9 +244,9 @@ func TestLoader_Load(t *testing.T) {
expectedVars(
g,
m,
"pkg-a-0:1.0(pkg-a)",
"pkg-b-0:1.0(pkg-b)",
"pkg-c-0:1.0(pkg-c)",
"pkg-a-0:1.0",
"pkg-b-0:1.0",
"pkg-c-0:1.0",
)
expectedBest(g, m, map[string]string{
"pkg-a": "0:1.0",
Expand Down Expand Up @@ -346,9 +345,8 @@ func TestLoader_Load(t *testing.T) {
expectedVars(
g,
model,
"app-0:1.0(app)", // x1
"toolkit-0:2.0(toolkit)", // x2
"toolkit-0:2.0(/usr/bin/tool)", // x3
"app-0:1.0", // x1
"toolkit-0:2.0", // x2
)
expectedBest(g, model, map[string]string{
"app": "0:1.0",
Expand All @@ -358,7 +356,6 @@ func TestLoader_Load(t *testing.T) {
expectedAnds(g, model,
x1, // Install: app
bf.Implies(x1, x2), // Requirement: app => toolkit
bf.Eq(x2, x3), // Equivalence (toolkit)
)
})

Expand All @@ -377,11 +374,9 @@ func TestLoader_Load(t *testing.T) {
expectedVars(
g,
model,
"apache-0:2.4(apache)",
"apache-0:2.4(webserver)",
"app-0:1.0(app)",
"nginx-0:1.2(nginx)",
"nginx-0:1.2(webserver)",
"apache-0:2.4", // x1
"app-0:1.0", // x2
"nginx-0:1.2", // x3
)
expectedBest(g, model, map[string]string{
"app": "0:1.0",
Expand All @@ -390,10 +385,8 @@ func TestLoader_Load(t *testing.T) {
})
expectedIgnores(g, model)
expectedAnds(g, model,
x3, // Install: app
bf.Implies(x3, bf.Or(x1, x4)), // Requirement: app => apache or nginx
bf.Eq(x1, x2), // Equivalence (apache)
bf.Eq(x4, x5), // Equivalence (nginx)
x2, // Install: app
bf.Implies(x2, bf.Or(x1, x3)), // Requirement: app => apache or nginx
)
})

Expand All @@ -409,7 +402,7 @@ func TestLoader_Load(t *testing.T) {
"B": []string{"0:1.0"},
"C": []string{"0:1.0"},
})
expectedVars(g, model, "A-0:1.0(A)", "B-0:1.0(B)", "C-0:1.0(C)")
expectedVars(g, model, "A-0:1.0", "B-0:1.0", "C-0:1.0")
expectedBest(g, model, map[string]string{
"A": "0:1.0",
"B": "0:1.0",
Expand All @@ -433,8 +426,7 @@ func TestLoader_Load(t *testing.T) {
expectedVars(
g,
model,
"platform-python-0:3.6(platform-python)",
"platform-python-0:3.6(/usr/libexec/platform-python)",
"platform-python-0:3.6",
)
expectedBest(
g,
Expand All @@ -443,7 +435,7 @@ func TestLoader_Load(t *testing.T) {
)
expectedIgnores(g, model)
expectedAnds(g, model,
bf.Eq(x1, x2), // Equivalence (platform-python)
bf.True, // Nothing to install
Copy link
Copy Markdown
Collaborator

@kellyma2 kellyma2 Jan 23, 2026

Choose a reason for hiding this comment

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

what is causing this change and why is it desirable?

)

// verify side effect
Expand All @@ -461,7 +453,7 @@ func TestLoader_Load(t *testing.T) {
expectedPackages(g, model, map[string][]string{
"A": []string{"5:1.0-2"},
})
expectedVars(g, model, "A-5:1.0-2(A)")
expectedVars(g, model, "A-5:1.0-2")
expectedBest(g, model, map[string]string{"A": "5:1.0-2"})
expectedAnds(g, model,
bf.True, // Nothing to install
Expand All @@ -483,12 +475,9 @@ func TestLoader_Load(t *testing.T) {
expectedVars(
g,
model,
"gcc-0:11.0(gcc)", // x1
"gcc-0:11.0(gcc)", // x2
"gcc11-0:11.0(gcc11)", // x3
"gcc11-0:11.0(gcc)", // x4
"gcc11-0:11.0(gcc11)", // x5
"pkgX-0:1.0(pkgX)", // x6
"gcc-0:11.0", // x1
"gcc11-0:11.0", // x2
"pkgX-0:1.0", // x3
)
expectedBest(g, model, map[string]string{
"gcc": "0:11.0",
Expand All @@ -497,12 +486,9 @@ func TestLoader_Load(t *testing.T) {
})

expectedAnds(g, model,
x6, // Install: pkgX
bf.Implies(x6, bf.Or(x1, x3)), // Requirement: pkgX => gcc or gcc11
bf.Implies(x1, x3), // Requirement: gcc => gcc11
bf.Eq(x1, x2), // Equivalence (gcc)
bf.Eq(x3, x4), // Equivalence (gcc11)
bf.Eq(x3, x5), // Equivalence (gcc11)
x3, // Install: pkgX
bf.Implies(x3, bf.Or(x1, x2)), // Requirement: pkgX => gcc or gcc11
bf.Implies(x1, x2), // Requirement: gcc => gcc11
)

})
Expand All @@ -511,7 +497,7 @@ func TestLoader_Load(t *testing.T) {
pkgA := newPackage("A", "1.0", nil, nil, []string{"A"}, nil)
model, _ := doLoad([]*api.Package{pkgA}, nil, nil, nil, false)

expectedVars(g, model, "A-0:1.0(A)")
expectedVars(g, model, "A-0:1.0")
expectedAnds(g, model,
bf.True, // Nothing to install
)
Expand All @@ -526,7 +512,7 @@ func TestLoader_Load(t *testing.T) {
expectedPackages(g, model, map[string][]string{
"A": []string{"0:1.0-1"},
})
expectedVars(g, model, "A-0:1.0-1(A)")
expectedVars(g, model, "A-0:1.0-1")
expectedAnds(g, model,
bf.Not(x1), // Can't install package `A` (missing dependency `B`).
)
Expand Down Expand Up @@ -582,7 +568,7 @@ func TestLoader_Load(t *testing.T) {
expectedPackages(g, model, map[string][]string{
"X": []string{selectedVersion},
})
expectedVars(g, model, "X-"+selectedVersion+"(X)")
expectedVars(g, model, "X-"+selectedVersion)
expectedBest(g, model, map[string]string{
"X": selectedVersion,
})
Expand Down
Loading
Loading