diff --git a/cmd/image-builder/build.go b/cmd/image-builder/build.go index f644fb8..6d2dbd3 100644 --- a/cmd/image-builder/build.go +++ b/cmd/image-builder/build.go @@ -4,31 +4,34 @@ import ( "fmt" "os" "path/filepath" + "strings" "github.com/osbuild/bootc-image-builder/bib/pkg/progress" "github.com/osbuild/images/pkg/imagefilter" ) type buildOptions struct { - OutputDir string - StoreDir string + OutputDir string + StoreDir string + OutputBasename string WriteManifest bool WriteBuildlog bool } -func buildImage(pbar progress.ProgressBar, res *imagefilter.Result, osbuildManifest []byte, opts *buildOptions) error { +func buildImage(pbar progress.ProgressBar, res *imagefilter.Result, osbuildManifest []byte, opts *buildOptions) (string, error) { if opts == nil { opts = &buildOptions{} } + basename := basenameFor(res, opts.OutputBasename) if opts.WriteManifest { - p := filepath.Join(opts.OutputDir, fmt.Sprintf("%s.osbuild-manifest.json", outputNameFor(res))) + p := filepath.Join(opts.OutputDir, fmt.Sprintf("%s.osbuild-manifest.json", basename)) if err := os.MkdirAll(filepath.Dir(p), 0755); err != nil { - return err + return "", err } if err := os.WriteFile(p, osbuildManifest, 0644); err != nil { - return err + return "", err } } @@ -38,16 +41,32 @@ func buildImage(pbar progress.ProgressBar, res *imagefilter.Result, osbuildManif } if opts.WriteBuildlog { if err := os.MkdirAll(opts.OutputDir, 0755); err != nil { - return fmt.Errorf("cannot create buildlog base directory: %w", err) + return "", fmt.Errorf("cannot create buildlog base directory: %w", err) } - p := filepath.Join(opts.OutputDir, fmt.Sprintf("%s.buildlog", outputNameFor(res))) + p := filepath.Join(opts.OutputDir, fmt.Sprintf("%s.buildlog", basename)) f, err := os.Create(p) if err != nil { - return fmt.Errorf("cannot create buildlog: %w", err) + return "", fmt.Errorf("cannot create buildlog: %w", err) } defer f.Close() osbuildOpts.BuildLog = f } - return progress.RunOSBuild(pbar, osbuildManifest, res.ImgType.Exports(), osbuildOpts) + if err := progress.RunOSBuild(pbar, osbuildManifest, res.ImgType.Exports(), osbuildOpts); err != nil { + return "", err + } + // Rename *sigh*, see https://github.com/osbuild/images/pull/1039 + // for my preferred way. Every frontend to images has to duplicate + // similar code like this. + pipelineDir := filepath.Join(opts.OutputDir, res.ImgType.Exports()[0]) + srcName := filepath.Join(pipelineDir, res.ImgType.Filename()) + imgExt := strings.SplitN(res.ImgType.Filename(), ".", 2)[1] + dstName := filepath.Join(opts.OutputDir, fmt.Sprintf("%s.%v", basename, imgExt)) + if err := os.Rename(srcName, dstName); err != nil { + return "", fmt.Errorf("cannot rename artifact to final name: %w", err) + } + // best effort, remove the now empty pipeline export dir from osbuild + _ = os.Remove(pipelineDir) + + return dstName, nil } diff --git a/cmd/image-builder/main.go b/cmd/image-builder/main.go index e48ca1a..ae98d34 100644 --- a/cmd/image-builder/main.go +++ b/cmd/image-builder/main.go @@ -7,7 +7,6 @@ import ( "io" "os" "os/signal" - "path/filepath" "syscall" "github.com/sirupsen/logrus" @@ -28,8 +27,12 @@ var ( osStderr io.Writer = os.Stderr ) -// generate the default output directory name for the given image -func outputNameFor(img *imagefilter.Result) string { +// basenameFor returns the basename for directory and filenames +// for the given imageType. This can be user overriden via userBasename. +func basenameFor(img *imagefilter.Result, userBasename string) string { + if userBasename != "" { + return userBasename + } return fmt.Sprintf("%s-%s-%s", img.Distro.Name(), img.ImgType.Name(), img.Arch.Name()) } @@ -123,11 +126,14 @@ func cmdManifestWrapper(pbar progress.ProgressBar, cmd *cobra.Command, args []st if useLibrepo { rpmDownloader = osbuild.RpmDownloaderLibrepo } - blueprintPath, err := cmd.Flags().GetString("blueprint") if err != nil { return nil, err } + // no error check here as this is (deliberately) not defined on + // "manifest" (if "images" learn to set the output filename in + // manifests we would change this + outputFilename, _ := cmd.Flags().GetString("output-name") bp, err := blueprintload.Load(blueprintPath) if err != nil { @@ -158,13 +164,17 @@ func cmdManifestWrapper(pbar progress.ProgressBar, cmd *cobra.Command, args []st return nil, err } } + if len(img.ImgType.Exports()) > 1 { + return nil, fmt.Errorf("image %q has multiple exports: this is current unsupport: please report this as a bug", basenameFor(img, "")) + } opts := &manifestOptions{ - OutputDir: outputDir, - BlueprintPath: blueprintPath, - Ostree: ostreeImgOpts, - RpmDownloader: rpmDownloader, - WithSBOM: withSBOM, + OutputDir: outputDir, + OutputFilename: outputFilename, + BlueprintPath: blueprintPath, + Ostree: ostreeImgOpts, + RpmDownloader: rpmDownloader, + WithSBOM: withSBOM, ForceRepos: forceRepos, } @@ -206,6 +216,10 @@ func cmdBuild(cmd *cobra.Command, args []string) error { if err != nil { return err } + outputBasename, err := cmd.Flags().GetString("output-name") + if err != nil { + return err + } withManifest, err := cmd.Flags().GetBool("with-manifest") if err != nil { return err @@ -255,20 +269,18 @@ func cmdBuild(cmd *cobra.Command, args []string) error { return err } } + outputDir = basenameFor(res, outputDir) - // XXX: support output filename via commandline (c.f. - // https://github.com/osbuild/images/pull/1039) - if outputDir == "" { - outputDir = outputNameFor(res) - } buildOpts := &buildOptions{ - OutputDir: outputDir, - StoreDir: cacheDir, - WriteManifest: withManifest, - WriteBuildlog: withBuildlog, + OutputDir: outputDir, + OutputBasename: outputBasename, + StoreDir: cacheDir, + WriteManifest: withManifest, + WriteBuildlog: withBuildlog, } pbar.SetPulseMsgf("Image building step") - if err := buildImage(pbar, res, mf.Bytes(), buildOpts); err != nil { + imagePath, err := buildImage(pbar, res, mf.Bytes(), buildOpts) + if err != nil { return err } pbar.Stop() @@ -276,8 +288,6 @@ func cmdBuild(cmd *cobra.Command, args []string) error { if uploader != nil { // XXX: integrate better into the progress, see bib - imagePath := filepath.Join(outputDir, res.ImgType.Name(), res.ImgType.Filename()) - if err := uploadImageWithProgress(uploader, imagePath); err != nil { return err } @@ -395,6 +405,7 @@ operating systems like Fedora, CentOS and RHEL with easy customizations support. // XXX: add "--verbose" here, similar to how bib is doing this // (see https://github.com/osbuild/bootc-image-builder/pull/790/commits/5cec7ffd8a526e2ca1e8ada0ea18f927695dfe43) buildCmd.Flags().String("progress", "auto", "type of progress bar to use (e.g. verbose,term)") + buildCmd.Flags().String("output-name", "", "set specific output basename") rootCmd.AddCommand(buildCmd) buildCmd.Flags().AddFlagSet(uploadCmd.Flags()) // add after the rest of the uploadCmd flag set is added to avoid diff --git a/cmd/image-builder/main_test.go b/cmd/image-builder/main_test.go index c629c16..365776a 100644 --- a/cmd/image-builder/main_test.go +++ b/cmd/image-builder/main_test.go @@ -314,6 +314,46 @@ func TestManifestIntegrationOstreeSmokeErrors(t *testing.T) { } } +// this is needed because images currently hardcodes the artifact filenames +// so we need to faithfully reproduce this in our tests. see images PR#1039 +// for an alternative way that would make this unneeded. +func makeFakeOsbuildScript() string { + return ` +cat - > "$0".stdin + +output_dir="" +export="" +while [[ $# -gt 0 ]]; do + key="$1" + case $key in + --output-directory) + output_dir="$2" + shift 2 + ;; + --export) + export="$2" + shift 2 + ;; + *) + shift 1 + esac +done +mkdir -p "$output_dir/$export" +case $export in + qcow2) + echo "fake-img-qcow2" > "$output_dir/$export/disk.qcow2" + ;; + image) + echo "fake-img-raw" > "$output_dir/$export/image.raw" + ;; + *) + echo "Unknown export: $1 - add to testscript" + exit 1 + ;; +esac +` +} + func TestBuildIntegrationHappy(t *testing.T) { if testing.Short() { t.Skip("manifest generation takes a while") @@ -330,6 +370,14 @@ func TestBuildIntegrationHappy(t *testing.T) { defer restore() tmpdir := t.TempDir() + curdir, err := os.Getwd() + require.NoError(t, err) + err = os.Chdir(tmpdir) + require.NoError(t, err) + defer func() { + os.Chdir(curdir) + }() + restore = main.MockOsArgs([]string{ "build", "qcow2", @@ -339,11 +387,11 @@ func TestBuildIntegrationHappy(t *testing.T) { }) defer restore() - script := `cat - > "$0".stdin` + script := makeFakeOsbuildScript() fakeOsbuildCmd := testutil.MockCommand(t, "osbuild", script) defer fakeOsbuildCmd.Restore() - err := main.Run() + err = main.Run() assert.NoError(t, err) assert.Contains(t, fakeStdout.String(), `Image build successful, result in "centos-9-qcow2-x86_64"`+"\n") @@ -420,7 +468,7 @@ func TestBuildIntegrationArgs(t *testing.T) { restore = main.MockOsArgs(cmd) defer restore() - script := `cat - > "$0".stdin` + script := makeFakeOsbuildScript() fakeOsbuildCmd := testutil.MockCommand(t, "osbuild", script) defer fakeOsbuildCmd.Restore() @@ -436,7 +484,9 @@ func TestBuildIntegrationArgs(t *testing.T) { // ensure we get exactly the expected files files, err := filepath.Glob(outputDir + "/*") assert.NoError(t, err) - assert.Equal(t, len(tc.expectedFiles), len(files), files) + // we always have the qcow2 dir + expectedFiles := append(tc.expectedFiles, "qcow") + assert.Equal(t, len(expectedFiles), len(files), files) for _, expected := range tc.expectedFiles { _, err = os.Stat(filepath.Join(outputDir, expected)) assert.NoError(t, err, fmt.Sprintf("file %q missing", expected)) @@ -466,11 +516,13 @@ func TestBuildIntegrationErrorsProgressVerbose(t *testing.T) { restore := main.MockNewRepoRegistry(testrepos.New) defer restore() + outputDir := t.TempDir() restore = main.MockOsArgs([]string{ "build", "qcow2", "--distro", "centos-9", "--progress=verbose", + "--output-dir", outputDir, }) defer restore() @@ -787,10 +839,11 @@ func TestBuildCrossArchCheckSkippedOnExperimentalBuildroot(t *testing.T) { "--distro", "centos-9", "--cache", tmpdir, "--arch=s390x", + "--output-dir", tmpdir, }) defer restore() - script := `cat - > "$0".stdin` + script := makeFakeOsbuildScript() fakeOsbuildCmd := testutil.MockCommand(t, "osbuild", script) defer fakeOsbuildCmd.Restore() @@ -804,3 +857,57 @@ func TestBuildCrossArchCheckSkippedOnExperimentalBuildroot(t *testing.T) { } } } + +func TestBuildIntegrationOutputFilename(t *testing.T) { + if testing.Short() { + t.Skip("manifest generation takes a while") + } + if !hasDepsolveDnf() { + t.Skip("no osbuild-depsolve-dnf binary found") + } + + restore := main.MockNewRepoRegistry(testrepos.New) + defer restore() + + var fakeStdout bytes.Buffer + restore = main.MockOsStdout(&fakeStdout) + defer restore() + + tmpdir := t.TempDir() + outputDir := filepath.Join(tmpdir, "output") + restore = main.MockOsArgs([]string{ + "build", + "qcow2", + fmt.Sprintf("--blueprint=%s", makeTestBlueprint(t, testBlueprint)), + "--distro", "centos-9", + "--cache", tmpdir, + "--output-dir", outputDir, + "--output-name=foo", + "--with-manifest", + "--with-sbom", + "--with-buildlog", + }) + defer restore() + + script := makeFakeOsbuildScript() + fakeOsbuildCmd := testutil.MockCommand(t, "osbuild", script) + defer fakeOsbuildCmd.Restore() + + err := main.Run() + assert.NoError(t, err) + + expectedFiles := []string{ + "foo.buildroot-build.spdx.json", + "foo.image-os.spdx.json", + "foo.osbuild-manifest.json", + "foo.buildlog", + "foo.qcow2", + } + files, err := filepath.Glob(outputDir + "/*") + assert.NoError(t, err) + assert.Equal(t, len(expectedFiles), len(files), files) + for _, expected := range expectedFiles { + _, err = os.Stat(filepath.Join(outputDir, expected)) + assert.NoError(t, err, fmt.Sprintf("file %q missing from %v", expected, files)) + } +} diff --git a/cmd/image-builder/manifest.go b/cmd/image-builder/manifest.go index c254491..3fc8078 100644 --- a/cmd/image-builder/manifest.go +++ b/cmd/image-builder/manifest.go @@ -1,9 +1,11 @@ package main import ( + "fmt" "io" "os" "path/filepath" + "strings" "github.com/osbuild/images/pkg/distro" "github.com/osbuild/images/pkg/imagefilter" @@ -16,11 +18,12 @@ import ( ) type manifestOptions struct { - OutputDir string - BlueprintPath string - Ostree *ostree.ImageOptions - RpmDownloader osbuild.RpmDownloader - WithSBOM bool + OutputDir string + OutputFilename string + BlueprintPath string + Ostree *ostree.ImageOptions + RpmDownloader osbuild.RpmDownloader + WithSBOM bool ForceRepos []string } @@ -52,11 +55,12 @@ func generateManifest(dataDir string, extraRepos []string, img *imagefilter.Resu RpmDownloader: opts.RpmDownloader, } if opts.WithSBOM { - outputDir := opts.OutputDir - if outputDir == "" { - outputDir = outputNameFor(img) - } + outputDir := basenameFor(img, opts.OutputDir) + overrideSBOMBase := strings.SplitN(opts.OutputFilename, ".", 2)[0] manifestGenOpts.SBOMWriter = func(filename string, content io.Reader, docType sbom.StandardType) error { + if overrideSBOMBase != "" { + filename = fmt.Sprintf("%s.%s", overrideSBOMBase, strings.SplitN(filename, ".", 2)[1]) + } return sbomWriter(outputDir, filename, content) } } diff --git a/cmd/image-builder/upload_test.go b/cmd/image-builder/upload_test.go index bbda00f..5631e82 100644 --- a/cmd/image-builder/upload_test.go +++ b/cmd/image-builder/upload_test.go @@ -121,12 +121,6 @@ func TestUploadCmdlineErrors(t *testing.T) { } } -var fakeOsbuildScriptAmiFmt = `#!/bin/sh -e -cat - > "$0".stdin -mkdir -p %[1]s/ami -echo -n %[2]s > %[1]s/ami/image.raw -` - func TestBuildAndUploadWithAWSMock(t *testing.T) { if testing.Short() { t.Skip("manifest generation takes a while") @@ -145,9 +139,8 @@ func TestBuildAndUploadWithAWSMock(t *testing.T) { }) defer restore() - fakeDiskContent := "fake-raw-img" outputDir := t.TempDir() - fakeOsbuildScript := fmt.Sprintf(fakeOsbuildScriptAmiFmt, outputDir, fakeDiskContent) + fakeOsbuildScript := makeFakeOsbuildScript() fakeOsbuildCmd := testutil.MockCommand(t, "osbuild", fakeOsbuildScript) defer fakeOsbuildCmd.Restore() @@ -174,7 +167,7 @@ func TestBuildAndUploadWithAWSMock(t *testing.T) { assert.Equal(t, amiName, "aws-ami-3") assert.Equal(t, 1, fa.checkCalls) assert.Equal(t, 1, fa.uploadAndRegisterCalls) - assert.Equal(t, fakeDiskContent, fa.uploadAndRegisterRead.String()) + assert.Equal(t, "fake-img-raw\n", fa.uploadAndRegisterRead.String()) } func TestBuildAmiButNotUpload(t *testing.T) { @@ -193,9 +186,8 @@ func TestBuildAmiButNotUpload(t *testing.T) { }) defer restore() - fakeDiskContent := "fake-raw-img" outputDir := t.TempDir() - fakeOsbuildScript := fmt.Sprintf(fakeOsbuildScriptAmiFmt, outputDir, fakeDiskContent) + fakeOsbuildScript := makeFakeOsbuildScript() fakeOsbuildCmd := testutil.MockCommand(t, "osbuild", fakeOsbuildScript) defer fakeOsbuildCmd.Restore() @@ -225,9 +217,8 @@ func TestBuildAndUploadWithAWSPartialCmdlineErrors(t *testing.T) { t.Skip("no osbuild-depsolve-dnf binary found") } - fakeDiskContent := "fake-raw-img" outputDir := t.TempDir() - fakeOsbuildScript := fmt.Sprintf(fakeOsbuildScriptAmiFmt, outputDir, fakeDiskContent) + fakeOsbuildScript := makeFakeOsbuildScript() fakeOsbuildCmd := testutil.MockCommand(t, "osbuild", fakeOsbuildScript) defer fakeOsbuildCmd.Restore() diff --git a/test/test_container.py b/test/test_container.py index dc7c7d0..2c09c80 100644 --- a/test/test_container.py +++ b/test/test_container.py @@ -22,7 +22,8 @@ def test_container_builds_image(tmp_path, build_container, use_librepo): f"--use-librepo={use_librepo}", ]) arch = "x86_64" - assert (output_dir / f"centos-9-minimal-raw-{arch}/xz/disk.raw.xz").exists() + basename = f"centos-9-minimal-raw-{arch}" + assert (output_dir / basename / f"{basename}.raw.xz").exists() # XXX: ensure no other leftover dirs dents = os.listdir(output_dir) assert len(dents) == 1, f"too many dentries in output dir: {dents}" @@ -88,8 +89,9 @@ def test_container_with_progress(tmp_path, build_fake_container, progress, needl "-v", f"{output_dir}:/output", build_fake_container, "build", - "minimal-raw", + "qcow2", "--distro", "centos-9", + "--output-dir=.", f"--progress={progress}", ], text=True) assert needle in output