Skip to content
This repository was archived by the owner on Aug 7, 2025. It is now read-only.

Improve dir subtraction #804

Merged
bryteise merged 2 commits into from
May 1, 2025
Merged

Improve dir subtraction #804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0327999
Avoid defining chrootDir in a loop
Apr 30, 2025
0f60955
Improve manifest include subtraction
Apr 30, 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
55 changes: 47 additions & 8 deletions swupd/manifest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"os"
"path/filepath"
"runtime"
"slices"
"sort"
"strconv"
"strings"
Expand Down Expand Up @@ -755,22 +756,60 @@ func (m *Manifest) addManifestFiles(ui UpdateInfo, c config) error {
}
} else {
// Add files to manifest that do not exist in included bundles.
chrootDir := filepath.Join(c.imageBase, fmt.Sprint(ui.version), "full")
includes := m.GetRecursiveIncludes()
usedDirs := make(map[string]struct{})
bundleDirs := []string{}
bundleFiles := []string{}
for f := range m.BundleInfo.Files {
fullPath := filepath.Join(chrootDir, f)
if fi, err := os.Lstat(fullPath); err == nil {
if !fi.IsDir() {
usedDirs[filepath.Dir(f)] = struct{}{}
Copy link

@bwarden bwarden May 1, 2025

Choose a reason for hiding this comment

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

Do we need any special handling for a file f in the root dir, e.g. /foo? I assume Dir would return ./ in that case, but the only non-dirs in root should be the symlinks lib, lib64, bin, and sbin, right?

Copy link
Contributor Author

@bryteise bryteise May 1, 2025

Choose a reason for hiding this comment

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

filepath.Dir("/file") will return "/" which is correct in my mind though won't come up as mixer doesn't put / in the manifests so it wouldn't be checked for removal.

bundleFiles = append(bundleFiles, f)
} else {
bundleDirs = append(bundleDirs, f)
}
}
}
for _, f := range bundleFiles {
isIncluded := false
for _, inc := range includes {
// Handle cycles
if inc.Name == m.Name {
continue
}
chrootDir := filepath.Join(c.imageBase, fmt.Sprint(ui.version), "full")
fullPath := filepath.Join(chrootDir, f)
if fi, err := os.Lstat(fullPath); err == nil {
if !fi.IsDir() {
if _, ok := inc.BundleInfo.Files[f]; ok {
isIncluded = true
break
}
if _, ok := inc.BundleInfo.Files[f]; ok {
isIncluded = true
break
}
}
if !isIncluded {
if err := m.addFile(f, c, ui.version); err != nil {
return err
}
}
}
// Directories are sorted into reverse order to allow usedDirs
// to populate correctly (/a/b/c will be seen before /a/b which
// will allow /a to be set when /a/b is seen)
slices.Sort(bundleDirs)
slices.Reverse(bundleDirs)
for _, f := range bundleDirs {
if _, ok := usedDirs[f]; ok {
usedDirs[filepath.Dir(f)] = struct{}{}
}
}
for _, f := range bundleDirs {
isIncluded := false
for _, inc := range includes {
Copy link

@bwarden bwarden May 1, 2025

Choose a reason for hiding this comment

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

Do we need to fudge includes so os-core is always the first element?

Copy link
Contributor Author

@bryteise bryteise May 1, 2025

Choose a reason for hiding this comment

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

In Manifest files I believe os-core is already the first element but mixer shouldn't be impacted by include order. The subtraction should happen regardless if something is in os-core no matter if os-core is looked at first or last from that logic.

if _, ok := inc.BundleInfo.Files[f]; ok {
if inc.Name == "os-core" {
isIncluded = true
break
} else if _, ok := usedDirs[f]; !ok {
isIncluded = true
break
}
}
}
Expand Down
84 changes: 80 additions & 4 deletions swupd/swupd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ func TestFullRun(t *testing.T) {
mustValidateZeroPack(t, ts.path("www/10/Manifest.test-bundle"), ts.path("www/10/pack-test-bundle-from-0.tar"))
mustHaveDeltaCount(t, infoTestBundle, 0)
// Empty file (bundle file), "foo".
mustHaveFullfileCount(t, infoTestBundle, 3)
mustHaveFullfileCount(t, infoTestBundle, 2)

testBundle := ts.parseManifest(10, "test-bundle")
checkIncludes(t, testBundle, "os-core")
Expand All @@ -112,7 +112,6 @@ func TestFullRun(t *testing.T) {
checkFileInManifest(t, osCore, 10, "/usr/share")
checkFileInManifest(t, osCore, 10, "/usr/share/clear")
checkFileInManifest(t, osCore, 10, "/usr/share/clear/bundles")
checkFileInManifest(t, osCore, 10, "/usr")
}

// Imported from swupd-server/test/functional/full-run-delta.
Expand All @@ -137,7 +136,7 @@ func TestFullRunDelta(t *testing.T) {

ts.createPack("os-core", 0, 10, ts.path("image"))
info := ts.createPack("test-bundle", 0, 10, ts.path("image"))
mustHaveFullfileCount(t, info, 5) // largefile, foo and foobarbaz and the test-bundle file.
mustHaveFullfileCount(t, info, 4) // largefile, foo and foobarbaz and the test-bundle file.

mustValidateZeroPack(t, ts.path("www/10/Manifest.os-core"), ts.path("www/10/pack-os-core-from-0.tar"))
mustValidateZeroPack(t, ts.path("www/10/Manifest.test-bundle"), ts.path("www/10/pack-test-bundle-from-0.tar"))
Expand All @@ -164,7 +163,7 @@ func TestFullRunDelta(t *testing.T) {

ts.createPack("os-core", 0, 20, ts.path("image"))
info = ts.createPack("test-bundle", 0, 20, ts.path("image"))
mustHaveFullfileCount(t, info, 3) // largefile and the test-bundle file.
mustHaveFullfileCount(t, info, 2) // largefile and the test-bundle file.

mustValidateZeroPack(t, ts.path("www/20/Manifest.os-core"), ts.path("www/20/pack-os-core-from-0.tar"))
mustValidateZeroPack(t, ts.path("www/20/Manifest.test-bundle"), ts.path("www/20/pack-test-bundle-from-0.tar"))
Expand All @@ -191,6 +190,83 @@ func TestFullRunDelta(t *testing.T) {
// not done by new swupd since it seems the client doesn't take advantage of them.
}

// Test an import cycle for includes subtraction
func TestRunBundleCycle(t *testing.T) {
ts := newTestSwupd(t, "bundle-cycle-")
defer ts.cleanup()

// Version 10.
ts.Bundles = []string{"os-core", "test-bundle1", "test-bundle2"}

ts.mkdir("image/10/os-core/a")
ts.write("image/10/test-bundle1/a/b/t1", "t1")
ts.write("image/10/test-bundle2/a/b/t2", "t2")
ts.write("image/10/noship/test-bundle1-includes", "test-bundle2")
ts.write("image/10/noship/test-bundle2-includes", "test-bundle1")

ts.createManifestsFromChroots(10)
ts.createFullfiles(10)

ts.createPack("os-core", 0, 10, ts.path("image"))
info1 := ts.createPack("test-bundle1", 0, 10, ts.path("image"))
info2 := ts.createPack("test-bundle2", 0, 10, ts.path("image"))
mustHaveFullfileCount(t, info1, 3) // b dir, t1 file and the test-bundle1 file.
mustHaveFullfileCount(t, info2, 3) // b dir, t2 file and the test-bundle2 file.

mustValidateZeroPack(t, ts.path("www/10/Manifest.os-core"), ts.path("www/10/pack-os-core-from-0.tar"))
mustValidateZeroPack(t, ts.path("www/10/Manifest.test-bundle1"), ts.path("www/10/pack-test-bundle1-from-0.tar"))
mustValidateZeroPack(t, ts.path("www/10/Manifest.test-bundle2"), ts.path("www/10/pack-test-bundle2-from-0.tar"))

osCore := ts.parseManifest(10, "os-core")
testBundle1 := ts.parseManifest(10, "test-bundle1")
testBundle2 := ts.parseManifest(10, "test-bundle2")
checkIncludes(t, testBundle1, "os-core", "test-bundle2")
checkIncludes(t, testBundle2, "os-core", "test-bundle1")
checkFileInManifest(t, osCore, 10, "/a")
checkFileInManifest(t, testBundle1, 10, "/a/b")
checkFileInManifest(t, testBundle1, 10, "/a/b/t1")
checkFileInManifest(t, testBundle2, 10, "/a/b")
checkFileInManifest(t, testBundle2, 10, "/a/b/t2")
}

// Test an import without cycle to see directory subtraction outside os-core
func TestRunBundleIncludes(t *testing.T) {
ts := newTestSwupd(t, "bundle-includes-")
defer ts.cleanup()

// Version 10.
ts.Bundles = []string{"os-core", "test-bundle1", "test-bundle2"}

ts.write("image/10/test-bundle1/a/t1", "t1")
ts.write("image/10/test-bundle2/t2", "t2")
// this would have subtracted with a cycle too but
// keeping this test separate in case directory subtraction
// improves in the case of cycles.
ts.mkdir("image/10/test-bundle2/a")
ts.write("image/10/noship/test-bundle2-includes", "test-bundle1")

ts.createManifestsFromChroots(10)
ts.createFullfiles(10)

ts.createPack("os-core", 0, 10, ts.path("image"))
info1 := ts.createPack("test-bundle1", 0, 10, ts.path("image"))
info2 := ts.createPack("test-bundle2", 0, 10, ts.path("image"))
mustHaveFullfileCount(t, info1, 3) // a dir, t1 file and the test-bundle1 file.
mustHaveFullfileCount(t, info2, 2) // t2 file and the test-bundle2 file.

mustValidateZeroPack(t, ts.path("www/10/Manifest.os-core"), ts.path("www/10/pack-os-core-from-0.tar"))
mustValidateZeroPack(t, ts.path("www/10/Manifest.test-bundle1"), ts.path("www/10/pack-test-bundle1-from-0.tar"))
mustValidateZeroPack(t, ts.path("www/10/Manifest.test-bundle2"), ts.path("www/10/pack-test-bundle2-from-0.tar"))

testBundle1 := ts.parseManifest(10, "test-bundle1")
testBundle2 := ts.parseManifest(10, "test-bundle2")
checkIncludes(t, testBundle2, "os-core", "test-bundle1")
checkFileInManifest(t, testBundle1, 10, "/a")
fileNotInManifest(t, testBundle2, "/a")
checkFileInManifest(t, testBundle1, 10, "/a/t1")
checkFileInManifest(t, testBundle2, 10, "/t2")
}

func TestAddFilesToBundleInfo(t *testing.T) {
ts := newTestSwupd(t, "extra-files")
defer ts.cleanup()
Expand Down
Loading