From 867774f61cc86e96b73da8afebbd873a85057c76 Mon Sep 17 00:00:00 2001 From: William Douglas Date: Thu, 24 Apr 2025 14:54:37 -0700 Subject: [PATCH 1/3] Relax bundle name requirements Both '.' and '+' are valid names for bundles for swupd-client. In order to use package names, mixer-tools also needs to allow these characters. Signed-off-by: William Douglas --- bat/tests/bundle-commands/run.bats | 35 +++++++++++++++--------------- builder/bundleset.go | 2 +- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/bat/tests/bundle-commands/run.bats b/bat/tests/bundle-commands/run.bats index f99fa3990..265ed772c 100755 --- a/bat/tests/bundle-commands/run.bats +++ b/bat/tests/bundle-commands/run.bats @@ -17,7 +17,8 @@ setup() { } @test "List the bundles in the mix" { - mixer bundle create foo.bar # 'bundle list' should work even if an invalid bundle is created + mixer bundle create foo^bar # 'bundle list' should work even if an invalid bundle is created + mixer bundle add foo\^bar # 'bundle add' will not add an invalid name but won't fail bundles=$(mixer $MIXARGS bundle list | grep -v "(included)") [[ $(echo "$bundles" | wc -l) -eq 4 ]] # Exactly 4 bundles in the mix @@ -26,7 +27,7 @@ setup() { [[ "$bundles" =~ kernel-native ]] [[ "$bundles" =~ bootloader ]] - rm -f local-bundles/foo.bar # Delete invalid bundle from local-bundles (test case clean up) + rm -f local-bundles/foo\^bar # Delete invalid bundle from local-bundles (test case clean up) } @test "Add an upstream bundle to the mix" { @@ -97,47 +98,47 @@ setup() { } @test "Skip invalid bundles from mix" { - mixer $MIXARGS bundle create foocar foo.car foojar --add # Create and add valid as well as invalid bundles + mixer $MIXARGS bundle create foo.bar foo\^car foojar --add # Create and add valid as well as invalid bundles - mixer $MIXARGS bundle list | grep -q foocar # "foocar" bundle is in the mix + mixer $MIXARGS bundle list | grep -q foo\.bar # "foo.car" bundle is in the mix mixer $MIXARGS bundle list | grep -q foojar # "foojar" bundle is in the mix - run $(mixer $MIXARGS bundle list | grep foo.car) # "foo.car" is an invalid bundle and is not in the mix + run $(mixer $MIXARGS bundle list | grep foo\^car) # "foo^car" is an invalid bundle and is not in the mix [[ ${#lines[@]} -eq 0 ]] # Delete bundle from local-bundles and mix (test case clean up) - mixer $MIXARGS bundle remove foocar foojar - rm -f local-bundles/foocar local-bundles/foojar local-bundles/foo.car + mixer $MIXARGS bundle remove foo.bar foojar + rm -f local-bundles/foo.bar local-bundles/foojar local-bundles/foo\^car } @test "Validate a bundle" { - echo "package" >> $BATS_TEST_DIRNAME/local-bundles/foobar - mixer $MIXARGS bundle create foo.bar + echo "package" >> $BATS_TEST_DIRNAME/local-bundles/foo+bar + mixer $MIXARGS bundle create foo\^bar - run mixer $MIXARGS bundle validate foobar + run mixer $MIXARGS bundle validate foo+bar [[ "$status" -eq 0 ]] # basic validation should pass - run mixer $MIXARGS bundle validate foobar --strict + run mixer $MIXARGS bundle validate foo+bar --strict [[ "$status" -eq 1 ]] # strict validation should fail [[ "$output" =~ "Empty Description in bundle header" ]] [[ "$output" =~ "Empty Maintainer in bundle header" ]] [[ "$output" =~ "Empty Status in bundle header" ]] [[ "$output" =~ "Empty Capabilities in bundle header" ]] - run mixer $MIXARGS bundle validate foo.bar + run mixer $MIXARGS bundle validate foo\^bar [[ "$status" -eq 1 ]] # basic validation should fail - [[ "$output" =~ "Invalid bundle name \"foo.bar\" derived from file" ]] + [[ "$output" =~ "Invalid bundle name \"foo^bar\" derived from file" ]] - run mixer $MIXARGS bundle validate foo.bar --strict + run mixer $MIXARGS bundle validate foo\^bar --strict [[ "$status" -eq 1 ]] # strict validation should fail - [[ "$output" =~ "Invalid bundle name \"foo.bar\" derived from file" ]] - [[ "$output" =~ "Invalid bundle name \"foo.bar\" in bundle header Title" ]] + [[ "$output" =~ "Invalid bundle name \"foo^bar\" derived from file" ]] + [[ "$output" =~ "Invalid bundle name \"foo^bar\" in bundle header Title" ]] [[ "$output" =~ "Empty Description in bundle header" ]] [[ "$output" =~ "Empty Maintainer in bundle header" ]] [[ "$output" =~ "Empty Status in bundle header" ]] [[ "$output" =~ "Empty Capabilities in bundle header" ]] - rm -f local-bundles/foo.bar # Delete invalid bundle from local-bundles (test case clean up) + rm -f local-bundles/foo\^bar # Delete invalid bundle from local-bundles (test case clean up) } # vi: ft=sh ts=8 sw=2 sts=2 et tw=80 diff --git a/builder/bundleset.go b/builder/bundleset.go index 23aeeb1e8..32d10a276 100644 --- a/builder/bundleset.go +++ b/builder/bundleset.go @@ -15,7 +15,7 @@ import ( ) var ( - validBundleNameRegex = regexp.MustCompile(`^[A-Za-z0-9-_]+$`) + validBundleNameRegex = regexp.MustCompile(`^[A-Za-z0-9-_+.]+$`) validPackageNameRegex = regexp.MustCompile(`^[A-Za-z0-9-_+.]+$`) bundleHeaderFieldRegex = regexp.MustCompile(`^# \[([A-Z]+)\]:\s*(.*)$`) ) From 87591e2f54ed13abd07f5bac6352cc97f6a8c9ff Mon Sep 17 00:00:00 2001 From: William Douglas Date: Thu, 24 Apr 2025 15:01:45 -0700 Subject: [PATCH 2/3] Identify manifest in logging Improve the usability of log messages by indicating which manifest is invalid when printing errors. Signed-off-by: William Douglas --- swupd/manifest.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/swupd/manifest.go b/swupd/manifest.go index c97434a7d..f53fd5c90 100644 --- a/swupd/manifest.go +++ b/swupd/manifest.go @@ -187,18 +187,18 @@ func readManifestFileEntry(fields []string, m *Manifest) error { // CheckHeaderIsValid verifies that all header fields in the manifest are valid. func (m *Manifest) CheckHeaderIsValid() error { if m.Header.Format == 0 { - return errors.New("manifest format not set") + return errors.Errorf("manifest format not set (%s)", m.Name) } if m.Header.Version == 0 { - return errors.New("manifest has version zero, version must be positive") + return errors.Errorf("manifest has version zero, version must be positive (%s)", m.Name) } if m.Header.FileCount == 0 { - return errors.New("manifest has a zero file count") + return fmt.Errorf("manifest has a zero file count (%s)", m.Name) } if m.Header.TimeStamp.IsZero() { - return errors.New("manifest timestamp not set") + return errors.Errorf("manifest timestamp not set (%s)", m.Name) } // Includes are not required. From 40387d58d88b1a36f4c2712cb5d741d6ea383a15 Mon Sep 17 00:00:00 2001 From: William Douglas Date: Thu, 24 Apr 2025 15:03:22 -0700 Subject: [PATCH 3/3] Add support for package bundles Add package level bundles "pundles" first class support in mixer. This involves tagging pundles with "pundle" in the bundle definition file's "MAINTAINER" field. This support requires changes to bundle constraints, primarily that cycles are now allowed in bundle includes. As part of this, directory subtraction will no longer be done for includes as the true directory owner isn't something that can be decided on in a cycle. Validation also needed a slight adjustment for pundles to reflect their minimal bundle definition files. Signed-off-by: William Douglas --- builder/bundles.go | 18 ++++++++++++ builder/bundleset.go | 62 +++++++++++++++++++++++++++++++-------- builder/bundleset_test.go | 54 +++++++++++++++++++++++++++++++--- swupd/manifest.go | 16 ++++++++-- swupd/swupd_test.go | 6 ++-- 5 files changed, 133 insertions(+), 23 deletions(-) diff --git a/builder/bundles.go b/builder/bundles.go index 17890bd0c..5e15e26fc 100644 --- a/builder/bundles.go +++ b/builder/bundles.go @@ -404,6 +404,24 @@ func resolvePackagesWithOptions(numWorkers int, set bundleSet, packagerCmd []str return } + if bundle.Header.Maintainer == "pundle" { + singlePkgMap := make(repoPkgMap) + for _, pkgs := range rpm { + for _, pkg := range pkgs { + if pkg.name == bundle.Name { + singlePkgMap[pkg.repo] = append(singlePkgMap[pkg.repo], pkg) + break + } + } + if len(singlePkgMap) > 0 { + break + } + } + if len(singlePkgMap) > 0 { + rpm = singlePkgMap + } + } + for _, pkgs := range rpm { // Add packages to bundle's AllPackages for _, pkg := range pkgs { diff --git a/builder/bundleset.go b/builder/bundleset.go index 32d10a276..dd76f5934 100644 --- a/builder/bundleset.go +++ b/builder/bundleset.go @@ -44,11 +44,10 @@ type bundleSet map[string]*bundle // the following constraints: // 1. Completeness. For each bundle in the set, every bundle included by that // bundle is also in the set. -// 2. Cycle-Free. The set contains no bundle include cycles. func validateAndFillBundleSet(bundles bundleSet) error { // Sort the bundles so that all includes and optional (also-add) includes appear // before a bundle, then calculate AllPackages for each bundle. - // Cycles and missing bundles are identified as part of sorting the bundles. + // Missing bundles are identified as part of sorting the bundles. sortedBundles, err := sortBundles(bundles) if err != nil { return err @@ -58,9 +57,13 @@ func validateAndFillBundleSet(bundles bundleSet) error { for k, v := range b.DirectPackages { b.AllPackages[k] = v } - for _, include := range b.DirectIncludes { - for k, v := range bundles[include].AllPackages { - b.AllPackages[k] = v + } + for _, b := range sortedBundles { + if b.Header.Maintainer != "pundle" { + for _, include := range b.DirectIncludes { + for k, v := range bundles[include].AllPackages { + b.AllPackages[k] = v + } } } } @@ -90,7 +93,8 @@ func sortBundles(bundles bundleSet) ([]*bundle, error) { visit = func(b *bundle) error { switch mark[b] { case Visiting: - return fmt.Errorf("cycle found in bundles: %s -> %s", strings.Join(visiting, " -> "), b.Name) + // In a cycle, short circuit + return nil case NotVisited: mark[b] = Visiting visiting = append(visiting, b.Name) @@ -161,14 +165,21 @@ func validateBundleFile(filename string, lvl ValidationLevel) error { } // Strict Validation - err = validateBundle(b) - if err != nil { - errText += err.Error() + "\n" - } + if b.Header.Maintainer == "pundle" { + err = validatePundle(b) + if err != nil { + errText += err.Error() + "\n" + } + } else { + err = validateBundle(b) + if err != nil { + errText += err.Error() + "\n" + } - name := filepath.Base(filename) - if name != b.Header.Title { - errText += fmt.Sprintf("Bundle name %q and bundle header Title %q do not match\n", name, b.Header.Title) + name := filepath.Base(filename) + if name != b.Header.Title { + errText += fmt.Sprintf("Bundle name %q and bundle header Title %q do not match\n", name, b.Header.Title) + } } if errText != "" { @@ -195,6 +206,31 @@ func validateBundleFilename(filename string) error { return nil } +func validatePundle(b *bundle) error { + var errText string + + if b.Header.Title != "" { + errText = fmt.Sprintf("pundle name %q detected in pundle header Title (should be empty)\n", b.Header.Title) + } + if b.Header.Description != "" { + errText += "Non-empty Description in pundle header\n" + } + if b.Header.Maintainer != "pundle" { + errText += "Non-pundle Maintainer in bundle header\n" + } + if b.Header.Status == "" { + errText += "Non-empty Status in bundle header\n" + } + if b.Header.Capabilities == "" { + errText += "Non-empty Capabilities in bundle header\n" + } + if errText != "" { + return errors.New(strings.TrimSuffix(errText, "\n")) + } + + return nil +} + func validateBundle(b *bundle) error { var errText string diff --git a/builder/bundleset_test.go b/builder/bundleset_test.go index ac7dccf8d..f260d873b 100644 --- a/builder/bundleset_test.go +++ b/builder/bundleset_test.go @@ -534,11 +534,57 @@ func TestParseBundleSet(t *testing.T) { }, }, - {"cyclic error two bundles", - FilesMap{"a": "include(b)", "b": "include(a)"}, Error}, + { + "cycles are fine, two bundles", + FilesMap{ + "a": Lines("include(b) A"), + "b": Lines("include(a) B"), + }, + CountsMap{ + "a": 2, + "b": 2, + }, + }, + + { + "cycles are fine, two pundles", + FilesMap{ + "a": "# [MAINTAINER]: pundle\ninclude(b)\nA", + "b": "# [MAINTAINER]: pundle\ninclude(a)\nB", + }, + CountsMap{ + "a": 1, + "b": 1, + }, + }, - {"cyclic error three bundles", - FilesMap{"a": "include(b)", "b": "include(c)", "c": "include(a)"}, Error}, + { + "cycles are fine, three bundles", + FilesMap{ + "a": Lines("include(c) include(b) A"), + "b": Lines("include(c) include(a) B"), + "c": Lines("include(b) include(a) C"), + }, + CountsMap{ + "a": 3, + "b": 3, + "c": 3, + }, + }, + + { + "cycles are fine, three pundles", + FilesMap{ + "a": "# [MAINTAINER]: pundle\ninclude(c)\ninclude(b)\nA", + "b": "# [MAINTAINER]: pundle\ninclude(c)\ninclude(a)\nB", + "c": "# [MAINTAINER]: pundle\ninclude(b)\ninclude(a)\nC", + }, + CountsMap{ + "a": 1, + "b": 1, + "c": 1, + }, + }, {"bundle not available", FilesMap{"a": "include(c)"}, Error}, diff --git a/swupd/manifest.go b/swupd/manifest.go index f53fd5c90..62398bb7f 100644 --- a/swupd/manifest.go +++ b/swupd/manifest.go @@ -759,9 +759,19 @@ func (m *Manifest) addManifestFiles(ui UpdateInfo, c config) error { for f := range m.BundleInfo.Files { isIncluded := false for _, inc := range includes { - if _, ok := inc.BundleInfo.Files[f]; ok { - isIncluded = true - break + // 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 !isIncluded { diff --git a/swupd/swupd_test.go b/swupd/swupd_test.go index 596e42969..5212c9526 100644 --- a/swupd/swupd_test.go +++ b/swupd/swupd_test.go @@ -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, 2) + mustHaveFullfileCount(t, infoTestBundle, 3) testBundle := ts.parseManifest(10, "test-bundle") checkIncludes(t, testBundle, "os-core") @@ -137,7 +137,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, 4) // largefile, foo and foobarbaz and the test-bundle file. + mustHaveFullfileCount(t, info, 5) // 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")) @@ -164,7 +164,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, 2) // largefile and the test-bundle file. + mustHaveFullfileCount(t, info, 3) // 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"))