ibcli: add new --output-name flag and predictable default names

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
 }

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,29 +269,25 @@ 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()
 	fmt.Fprintf(osStdout, "Image build successful, result in %q\n", outputDir)
 
 	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

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")
@@ -339,11 +379,16 @@ 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()
+	var err error
+	// run inside the tmpdir to validate that the default output dir
+	// creation works
+	testutil.Chdir(t, tmpdir, func() {
+		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 +465,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 +481,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 +513,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 +836,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 +854,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.n.0",
+		"--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.n.0.buildroot-build.spdx.json",
+		"foo.n.0.image-os.spdx.json",
+		"foo.n.0.osbuild-manifest.json",
+		"foo.n.0.buildlog",
+		"foo.n.0.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))
+	}
+}