Repository: osbuild/osbuild-composer
Pull request #3090 (2022-10-20 21:55:20)

Fix loading cross distro compose results
Fixes a problem with the store load not being able to load composes for other distros when the image type isn't supported on the host distro.

This pull request includes:

- [X] adequate testing for the new functionality or fixed issue
- [X] adequate documentation informing people about the change such as
  - [ ] submit a PR for the [guides](https://github.com/osbuild/guides) repository if this PR changed any behavior described there: https://www.osbuild.org/guides/


achilleas-k: Did an auto-rebase on main.
bcl: ppc64le and s390x on Fedora don't have the same image types so the update test was failing. Forcing the host arch to x86_64 like it did previously should fix that for now, but that test could use some work at some point in the future.

Patch:
diff --git a/cmd/osbuild-store-dump/main.go b/cmd/osbuild-store-dump/main.go
index 239058598..3019b3701 100644
--- a/cmd/osbuild-store-dump/main.go
+++ b/cmd/osbuild-store-dump/main.go
@@ -12,6 +12,7 @@ import (
 	"github.com/osbuild/osbuild-composer/internal/blueprint"
 	"github.com/osbuild/osbuild-composer/internal/distro"
 	"github.com/osbuild/osbuild-composer/internal/distro/fedora"
+	"github.com/osbuild/osbuild-composer/internal/distroregistry"
 	"github.com/osbuild/osbuild-composer/internal/dnfjson"
 	"github.com/osbuild/osbuild-composer/internal/rpmmd"
 	"github.com/osbuild/osbuild-composer/internal/store"
@@ -142,7 +143,8 @@ func main() {
 	}
 	rpmmdCache := path.Join(homeDir, ".cache/osbuild-composer/rpmmd")
 
-	s := store.New(&cwd, a, nil)
+	dr, _ := distroregistry.New(d, nil)
+	s := store.New(&cwd, dr, nil)
 	if s == nil {
 		panic("could not create store")
 	}
diff --git a/internal/distroregistry/distroregistry.go b/internal/distroregistry/distroregistry.go
index 7a168a3f9..a298e905a 100644
--- a/internal/distroregistry/distroregistry.go
+++ b/internal/distroregistry/distroregistry.go
@@ -40,14 +40,16 @@ type supportedDistro struct {
 }
 
 type Registry struct {
-	distros    map[string]distro.Distro
-	hostDistro distro.Distro
+	distros      map[string]distro.Distro
+	hostDistro   distro.Distro
+	hostArchName string
 }
 
 func New(hostDistro distro.Distro, distros ...distro.Distro) (*Registry, error) {
 	reg := &Registry{
-		distros:    make(map[string]distro.Distro),
-		hostDistro: hostDistro,
+		distros:      make(map[string]distro.Distro),
+		hostDistro:   hostDistro,
+		hostArchName: common.CurrentArch(),
 	}
 	for _, d := range distros {
 		name := d.Name()
@@ -136,3 +138,13 @@ func mangleHostDistroName(name string, isBeta, isStream bool) string {
 func (r *Registry) FromHost() distro.Distro {
 	return r.hostDistro
 }
+
+// HostArchName returns the host's arch name
+func (r *Registry) HostArchName() string {
+	return r.hostArchName
+}
+
+// SetHostArchName can be used to override the host's arch name for testing
+func (r *Registry) SetHostArchName(name string) {
+	r.hostArchName = name
+}
diff --git a/internal/store/fixtures.go b/internal/store/fixtures.go
index dff422659..93d936a66 100644
--- a/internal/store/fixtures.go
+++ b/internal/store/fixtures.go
@@ -41,7 +41,8 @@ func FixtureBase() *Store {
 		},
 	}
 
-	d := test_distro.New()
+	dr := test_distro.NewRegistry()
+	d := dr.FromHost()
 	arch, err := d.GetArch(test_distro.TestArchName)
 	if err != nil {
 		panic(fmt.Sprintf("failed to get architecture %s for a test distro: %v", test_distro.TestArchName, err))
@@ -54,7 +55,8 @@ func FixtureBase() *Store {
 	if err != nil {
 		panic(fmt.Sprintf("failed to create a manifest: %v", err))
 	}
-	s := New(nil, arch, nil)
+
+	s := New(nil, dr, nil)
 
 	pkgs := []rpmmd.PackageSpec{
 		{
@@ -176,7 +178,8 @@ func FixtureFinished() *Store {
 		},
 	}
 
-	d := test_distro.New()
+	dr := test_distro.NewRegistry()
+	d := dr.FromHost()
 	arch, err := d.GetArch(test_distro.TestArchName)
 	if err != nil {
 		panic(fmt.Sprintf("failed to get architecture %s for a test distro: %v", test_distro.TestArchName, err))
@@ -189,7 +192,8 @@ func FixtureFinished() *Store {
 	if err != nil {
 		panic(fmt.Sprintf("failed to create a manifest: %v", err))
 	}
-	s := New(nil, arch, nil)
+
+	s := New(nil, dr, nil)
 
 	pkgs := []rpmmd.PackageSpec{
 		{
@@ -272,13 +276,14 @@ func FixtureEmpty() *Store {
 		Customizations: nil,
 	}
 
-	d := test_distro.New()
-	arch, err := d.GetArch(test_distro.TestArchName)
+	dr := test_distro.NewRegistry()
+	d := dr.FromHost()
+	_, err := d.GetArch(test_distro.TestArchName)
 	if err != nil {
 		panic(fmt.Sprintf("failed to get architecture %s for a test distro: %v", test_distro.TestArchName, err))
 	}
 
-	s := New(nil, arch, nil)
+	s := New(nil, dr, nil)
 
 	s.blueprints[bName] = b
 
@@ -310,13 +315,14 @@ func FixtureOldChanges() *Store {
 		Customizations: nil,
 	}
 
-	d := test_distro.New()
-	arch, err := d.GetArch(test_distro.TestArchName)
+	dr := test_distro.NewRegistry()
+	d := dr.FromHost()
+	_, err := d.GetArch(test_distro.TestArchName)
 	if err != nil {
 		panic(fmt.Sprintf("failed to get architecture %s for a test distro: %v", test_distro.TestArchName, err))
 	}
 
-	s := New(nil, arch, nil)
+	s := New(nil, dr, nil)
 
 	s.PushBlueprint(b, "Initial commit")
 	b.Version = "0.0.1"
diff --git a/internal/store/json.go b/internal/store/json.go
index 424a2ac57..096d73334 100644
--- a/internal/store/json.go
+++ b/internal/store/json.go
@@ -2,6 +2,7 @@ package store
 
 import (
 	"errors"
+	"fmt"
 	"log"
 	"sort"
 	"time"
@@ -10,6 +11,7 @@ import (
 	"github.com/osbuild/osbuild-composer/internal/blueprint"
 	"github.com/osbuild/osbuild-composer/internal/common"
 	"github.com/osbuild/osbuild-composer/internal/distro"
+	"github.com/osbuild/osbuild-composer/internal/distroregistry"
 	"github.com/osbuild/osbuild-composer/internal/rpmmd"
 	"github.com/osbuild/osbuild-composer/internal/target"
 )
@@ -96,11 +98,11 @@ func newWorkspaceFromV0(workspaceStruct workspaceV0) map[string]blueprint.Bluepr
 	return workspace
 }
 
-func newComposesFromV0(composesStruct composesV0, arch distro.Arch, log *log.Logger) map[uuid.UUID]Compose {
+func newComposesFromV0(composesStruct composesV0, dr *distroregistry.Registry, log *log.Logger) map[uuid.UUID]Compose {
 	composes := make(map[uuid.UUID]Compose)
 
 	for composeID, composeStruct := range composesStruct {
-		c, err := newComposeFromV0(composeStruct, arch)
+		c, err := newComposeFromV0(composeStruct, dr)
 		if err != nil {
 			if log != nil {
 				log.Printf("ignoring compose: %v", err)
@@ -143,15 +145,32 @@ func newImageBuildFromV0(imageBuildStruct imageBuildV0, arch distro.Arch) (Image
 	}, nil
 }
 
-func newComposeFromV0(composeStruct composeV0, arch distro.Arch) (Compose, error) {
+func newComposeFromV0(composeStruct composeV0, dr *distroregistry.Registry) (Compose, error) {
 	if len(composeStruct.ImageBuilds) != 1 {
 		return Compose{}, errors.New("compose with unsupported number of image builds")
 	}
+
+	// Get the distro from the blueprint (empty means use host distro)
+	bp := composeStruct.Blueprint.DeepCopy()
+	distroName := bp.Distro
+	if len(distroName) == 0 {
+		distroName = dr.FromHost().Name()
+	}
+	distro := dr.GetDistro(distroName)
+	if distro == nil {
+		return Compose{}, fmt.Errorf("Unknown distro - %s", distroName)
+	}
+
+	// Get the host distro's architecture. This contains the distro+arch specific image types
+	arch, err := distro.GetArch(dr.HostArchName())
+	if err != nil {
+		return Compose{}, err
+	}
+
 	ib, err := newImageBuildFromV0(composeStruct.ImageBuilds[0], arch)
 	if err != nil {
 		return Compose{}, err
 	}
-	bp := composeStruct.Blueprint.DeepCopy()
 
 	pkgs := make([]rpmmd.PackageSpec, len(composeStruct.Packages))
 	copy(pkgs, composeStruct.Packages)
@@ -234,11 +253,11 @@ func newCommitsFromV0(commitsMapStruct commitsV0, changesMapStruct changesV0) ma
 	return commitsMap
 }
 
-func newStoreFromV0(storeStruct storeV0, arch distro.Arch, log *log.Logger) *Store {
+func newStoreFromV0(storeStruct storeV0, dr *distroregistry.Registry, log *log.Logger) *Store {
 	return &Store{
 		blueprints:        newBlueprintsFromV0(storeStruct.Blueprints),
 		workspace:         newWorkspaceFromV0(storeStruct.Workspace),
-		composes:          newComposesFromV0(storeStruct.Composes, arch, log),
+		composes:          newComposesFromV0(storeStruct.Composes, dr, log),
 		sources:           newSourceConfigsFromV0(storeStruct.Sources),
 		blueprintsChanges: newChangesFromV0(storeStruct.Changes),
 		blueprintsCommits: newCommitsFromV0(storeStruct.Commits, storeStruct.Changes),
diff --git a/internal/store/store.go b/internal/store/store.go
index bcc37dfee..71158e5b8 100644
--- a/internal/store/store.go
+++ b/internal/store/store.go
@@ -20,6 +20,7 @@ import (
 
 	"github.com/osbuild/osbuild-composer/internal/blueprint"
 	"github.com/osbuild/osbuild-composer/internal/common"
+	"github.com/osbuild/osbuild-composer/internal/distroregistry"
 	"github.com/osbuild/osbuild-composer/internal/rpmmd"
 	"github.com/osbuild/osbuild-composer/internal/target"
 
@@ -79,7 +80,7 @@ func (e *NoLocalTargetError) Error() string {
 	return e.message
 }
 
-func New(stateDir *string, arch distro.Arch, log *log.Logger) *Store {
+func New(stateDir *string, dr *distroregistry.Registry, log *log.Logger) *Store {
 	var storeStruct storeV0
 	var db *jsondb.JSONDatabase
 
@@ -91,7 +92,7 @@ func New(stateDir *string, arch distro.Arch, log *log.Logger) *Store {
 		}
 	}
 
-	store := newStoreFromV0(storeStruct, arch, log)
+	store := newStoreFromV0(storeStruct, dr, log)
 
 	store.stateDir = stateDir
 	store.db = db
diff --git a/internal/weldr/api.go b/internal/weldr/api.go
index 64e604d2d..032f786c2 100644
--- a/internal/weldr/api.go
+++ b/internal/weldr/api.go
@@ -164,13 +164,12 @@ func New(repoPaths []string, stateDir string, solver *dnfjson.BaseSolver, dr *di
 		return nil, fmt.Errorf("error loading repository definitions: %v", err)
 	}
 
-	var hostArch distro.Arch
 	hostDistro := dr.GetDistro(hostDistroName)
 	if hostDistro != nil {
 		// get canonical distro name if the host distro is supported
 		hostDistroName = hostDistro.Name()
 
-		hostArch, err = hostDistro.GetArch(archName)
+		_, err = hostDistro.GetArch(archName)
 		if err != nil {
 			return nil, fmt.Errorf("Host distro does not support host architecture: %v", err)
 		}
@@ -185,7 +184,7 @@ func New(repoPaths []string, stateDir string, solver *dnfjson.BaseSolver, dr *di
 		log.Printf("host distro %q is not supported: only cross-distro builds are available", hostDistroName)
 	}
 
-	store := store.New(&stateDir, hostArch, logger)
+	store := store.New(&stateDir, dr, logger)
 	compatOutputDir := path.Join(stateDir, "outputs")
 
 	api := &API{


Test patch:
diff --git a/internal/distro/test_distro/distro.go b/internal/distro/test_distro/distro.go
index 3fbc35db0..029d4f13b 100644
--- a/internal/distro/test_distro/distro.go
+++ b/internal/distro/test_distro/distro.go
@@ -9,6 +9,7 @@ import (
 	"github.com/osbuild/osbuild-composer/internal/blueprint"
 	"github.com/osbuild/osbuild-composer/internal/container"
 	"github.com/osbuild/osbuild-composer/internal/distro"
+	"github.com/osbuild/osbuild-composer/internal/distroregistry"
 	"github.com/osbuild/osbuild-composer/internal/osbuild"
 	"github.com/osbuild/osbuild-composer/internal/rpmmd"
 )
@@ -344,6 +345,18 @@ func New() *TestDistro {
 	return newTestDistro(TestDistroName, TestDistroModulePlatformID, TestDistroReleasever)
 }
 
+func NewRegistry() *distroregistry.Registry {
+	td := New()
+	registry, err := distroregistry.New(td, td)
+	if err != nil {
+		panic(err)
+	}
+
+	// Override the host's architecture name with the test's name
+	registry.SetHostArchName(TestArchName)
+	return registry
+}
+
 // New2 returns new instance of TestDistro named "test-distro-2".
 func New2() *TestDistro {
 	return newTestDistro(TestDistro2Name, TestDistro2ModulePlatformID, TestDistro2Releasever)
diff --git a/internal/store/json_test.go b/internal/store/json_test.go
index bec573da9..89846a101 100644
--- a/internal/store/json_test.go
+++ b/internal/store/json_test.go
@@ -15,6 +15,7 @@ import (
 	"github.com/osbuild/osbuild-composer/internal/distro"
 	"github.com/osbuild/osbuild-composer/internal/distro/fedora"
 	"github.com/osbuild/osbuild-composer/internal/distro/test_distro"
+	"github.com/osbuild/osbuild-composer/internal/distroregistry"
 	"github.com/osbuild/osbuild-composer/internal/rpmmd"
 	"github.com/osbuild/osbuild-composer/internal/target"
 	"github.com/stretchr/testify/assert"
@@ -114,13 +115,14 @@ func Test_imageTypeFromCompatString(t *testing.T) {
 
 func TestMarshalEmpty(t *testing.T) {
 	d := test_distro.New()
-	arch, err := d.GetArch(test_distro.TestArchName)
+	_, err := d.GetArch(test_distro.TestArchName)
 	if err != nil {
 		panic(fmt.Sprintf("failed to get architecture %s for a test distro: %v", test_distro.TestArchName, err))
 	}
 	store1 := FixtureEmpty()
 	storeV0 := store1.toStoreV0()
-	store2 := newStoreFromV0(*storeV0, arch, nil)
+	dr := test_distro.NewRegistry()
+	store2 := newStoreFromV0(*storeV0, dr, nil)
 	if !reflect.DeepEqual(store1, store2) {
 		t.Errorf("marshal/unmarshal roundtrip not a noop for empty store:\n got: %#v\n want: %#v", store1, store2)
 	}
@@ -128,13 +130,14 @@ func TestMarshalEmpty(t *testing.T) {
 
 func TestMarshalFinished(t *testing.T) {
 	d := test_distro.New()
-	arch, err := d.GetArch(test_distro.TestArchName)
+	_, err := d.GetArch(test_distro.TestArchName)
 	if err != nil {
 		panic(fmt.Sprintf("failed to get architecture %s for a test distro: %v", test_distro.TestArchName, err))
 	}
 	store1 := FixtureFinished()
 	storeV0 := store1.toStoreV0()
-	store2 := newStoreFromV0(*storeV0, arch, nil)
+	dr := test_distro.NewRegistry()
+	store2 := newStoreFromV0(*storeV0, dr, nil)
 	if !reflect.DeepEqual(store1, store2) {
 		t.Errorf("marshal/unmarshal roundtrip not a noop for base store:\n got: %#v\n want: %#v", store2, store1)
 	}
@@ -187,11 +190,12 @@ func TestStore_toStoreV0(t *testing.T) {
 func Test_newStoreFromV0(t *testing.T) {
 	type args struct {
 		storeStruct storeV0
-		arch        distro.Arch
+		registry    *distroregistry.Registry
 	}
 
-	testDistro := test_distro.New()
-	testArch, _ := testDistro.GetArch(test_distro.TestArchName)
+	//testDistro := test_distro.New()
+	//	testArch, _ := testDistro.GetArch(test_distro.TestArchName)
+	dr := test_distro.NewRegistry()
 
 	tests := []struct {
 		name string
@@ -202,14 +206,14 @@ func Test_newStoreFromV0(t *testing.T) {
 			name: "empty",
 			args: args{
 				storeStruct: storeV0{},
-				arch:        testArch,
+				registry:    dr,
 			},
-			want: New(nil, testArch, nil),
+			want: New(nil, dr, nil),
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := newStoreFromV0(tt.args.storeStruct, tt.args.arch, nil); !reflect.DeepEqual(got, tt.want) {
+			if got := newStoreFromV0(tt.args.storeStruct, tt.args.registry, nil); !reflect.DeepEqual(got, tt.want) {
 				t.Errorf("newStoreFromV0() =\n got: %#v\n want: %#v", got, tt.want)
 			}
 		})
@@ -303,9 +307,13 @@ func Test_upgrade(t *testing.T) {
 		assert.NoErrorf(err, "Could not read test-store '%s': %v", fileName, err)
 		err = json.Unmarshal([]byte(file), &storeStruct)
 		assert.NoErrorf(err, "Could not parse test-store '%s': %v", fileName, err)
-		arch, err := fedora.NewF35().GetArch(distro.X86_64ArchName)
+		f35 := fedora.NewF35()
+		registry, err := distroregistry.New(f35, f35)
 		assert.NoError(err)
-		store := newStoreFromV0(storeStruct, arch, nil)
+
+		// The test data has image types only supported on Fedora X86_64
+		registry.SetHostArchName(distro.X86_64ArchName)
+		store := newStoreFromV0(storeStruct, registry, nil)
 		assert.Equal(1, len(store.blueprints))
 		assert.Equal(1, len(store.blueprintsChanges))
 		assert.Equal(1, len(store.blueprintsCommits))
@@ -1094,25 +1102,26 @@ func Test_newComposeFromV0(t *testing.T) {
 	testDistro := test_distro.New()
 	testArch, _ := testDistro.GetArch(test_distro.TestArchName)
 	testImageType, _ := testArch.GetImageType(test_distro.TestImageTypeName)
+	dr := test_distro.NewRegistry()
 
 	tests := []struct {
-		name    string
-		compose composeV0
-		arch    distro.Arch
-		want    Compose
-		errOk   bool
+		name     string
+		compose  composeV0
+		registry *distroregistry.Registry
+		want     Compose
+		errOk    bool
 	}{
 		{
-			name:    "empty",
-			compose: composeV0{},
-			arch:    testArch,
-			want:    Compose{},
-			errOk:   true,
+			name:     "empty",
+			compose:  composeV0{},
+			registry: dr,
+			want:     Compose{},
+			errOk:    true,
 		},
 		{
-			name:  "qcow2 compose",
-			arch:  testArch,
-			errOk: false,
+			name:     "qcow2 compose",
+			registry: dr,
+			errOk:    false,
 			compose: composeV0{
 				Blueprint: &bp,
 				ImageBuilds: []imageBuildV0{
@@ -1182,7 +1191,7 @@ func Test_newComposeFromV0(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := newComposeFromV0(tt.compose, tt.arch)
+			got, err := newComposeFromV0(tt.compose, tt.registry)
 			if err != nil {
 				if !tt.errOk {
 					t.Errorf("newComposeFromV0() error = %v", err)
@@ -1371,22 +1380,23 @@ func Test_newComposesFromV0(t *testing.T) {
 	testDistro := test_distro.New()
 	testArch, _ := testDistro.GetArch(test_distro.TestArchName)
 	testImageType, _ := testArch.GetImageType(test_distro.TestImageTypeName)
+	dr := test_distro.NewRegistry()
 
 	tests := []struct {
 		name     string
-		arch     distro.Arch
+		registry *distroregistry.Registry
 		composes composesV0
 		want     map[uuid.UUID]Compose
 	}{
 		{
 			name:     "empty",
-			arch:     testArch,
+			registry: dr,
 			composes: composesV0{},
 			want:     make(map[uuid.UUID]Compose),
 		},
 		{
-			name: "two composes",
-			arch: testArch,
+			name:     "two composes",
+			registry: dr,
 			composes: composesV0{
 				uuid.MustParse("f53b49c0-d321-447e-8ab8-6e827891e3f0"): {
 					Blueprint: &bp,
@@ -1525,7 +1535,7 @@ func Test_newComposesFromV0(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := newComposesFromV0(tt.composes, tt.arch, nil); !reflect.DeepEqual(got, tt.want) {
+			if got := newComposesFromV0(tt.composes, tt.registry, nil); !reflect.DeepEqual(got, tt.want) {
 				t.Errorf("newComposesFromV0() =\n got: %#v\n want: %#v", got, tt.want)
 			}
 		})
diff --git a/internal/store/store_test.go b/internal/store/store_test.go
index 96cd2fceb..6cc489e80 100644
--- a/internal/store/store_test.go
+++ b/internal/store/store_test.go
@@ -133,10 +133,11 @@ func (suite *storeTest) SetupSuite() {
 //setup before each test
 func (suite *storeTest) SetupTest() {
 	distro := test_distro.New()
-	arch, err := distro.GetArch(test_distro.TestArchName)
+	_, err := distro.GetArch(test_distro.TestArchName)
 	suite.NoError(err)
 	suite.dir = suite.T().TempDir()
-	suite.myStore = New(&suite.dir, arch, nil)
+	dr := test_distro.NewRegistry()
+	suite.myStore = New(&suite.dir, dr, nil)
 }
 
 func (suite *storeTest) TestRandomSHA1String() {