Cleanup tar-building code

Signed-off-by: Micah Young <ymicah@vmware.com> Signed-off-by: Andrew Meyer <meyeran@vmware.com>
mdelillo · May 5, 2020 · aeb6e51 · aeb6e51
1 parent d04e7ff
commit aeb6e51
Show file tree

Hide file tree

Showing 14 changed files with 66 additions and 127 deletions.
diff --git a/internal/archive/archive.go b/internal/archive/archive.go
@@ -37,15 +37,15 @@ func GenerateTar(genFn func(TarWriter) error) io.ReadCloser {
 	return GenerateTarWithWriter(genFn, DefaultTarWriterFactory)
 }
 
-// GenerateTar returns a reader to a tar from a generator function using a factory writer. Note that the
-// generator will not fully execute until the reader is fully read from. Any errors
-// returned by the generator will be returned when reading the reader.
-func GenerateTarWithWriter(genFn func(TarWriter) error, tarWriterFactory TarWriterFactory) io.ReadCloser {
+// GenerateTarWithTar returns a reader to a tar from a generator function using a writer from the provided factory.
+// Note that the generator will not fully execute until the reader is fully read from. Any errors returned by the
+// generator will be returned when reading the reader.
+func GenerateTarWithWriter(genFn func(TarWriter) error, twf TarWriterFactory) io.ReadCloser {
 	errChan := make(chan error)
 	pr, pw := io.Pipe()
 
 	go func() {
-		tw := tarWriterFactory.NewTarWriter(pw)
+		tw := twf.NewTarWriter(pw)
 		defer func() {
 			if r := recover(); r != nil {
 				tw.Close()
@@ -91,23 +91,16 @@ func aggregateError(base, addition error) error {
 	return errors.Wrap(addition, base.Error())
 }
 
-func CreateSingleFileTarReader(path, txt string) (io.Reader, error) {
-	var buf bytes.Buffer
+func CreateSingleFileTarReader(path, txt string) io.ReadCloser {
 	tarBuilder := TarBuilder{}
 	tarBuilder.AddFile(path, 0644, NormalizedDateTime, []byte(txt))
-	_, err := tarBuilder.WriteTo(&buf)
-	if err != nil {
-		return nil, err
-	}
-
-	return bytes.NewReader(buf.Bytes()), nil
+	return tarBuilder.Reader(DefaultTarWriterFactory)
 }
 
-func CreateSingleFileTar(tw TarWriter, path, txt string) error {
+func CreateSingleFileTar(tarFile, path, txt string) error {
 	tarBuilder := TarBuilder{}
 	tarBuilder.AddFile(path, 0644, NormalizedDateTime, []byte(txt))
-	_, err := tarBuilder.WriteToTarWriter(tw)
-	return err
+	return tarBuilder.WriteToPath(tarFile, DefaultTarWriterFactory)
 }
 
 // ErrEntryNotExist is an error returned if an entry path doesn't exist

diff --git a/internal/archive/archive_test.go b/internal/archive/archive_test.go
@@ -62,14 +62,11 @@ func testArchive(t *testing.T, when spec.G, it spec.S) {
 
 		when("tgz has the path", func() {
 			it.Before(func() {
-				tw := tar.NewWriter(tarFile)
-				defer tw.Close()
-				err = archive.CreateSingleFileTar(tw, "file1", "file-1 content")
+				err = archive.CreateSingleFileTar(tarFile.Name(), "file1", "file-1 content")
 				h.AssertNil(t, err)
 			})
 
 			it("returns the file contents", func() {
-				_, err = tarFile.Seek(0, 0)
 				h.AssertNil(t, err)
 				_, contents, err := archive.ReadTarEntry(tarFile, "file1")
 				h.AssertNil(t, err)
@@ -79,15 +76,11 @@ func testArchive(t *testing.T, when spec.G, it spec.S) {
 
 		when("tgz has ./path", func() {
 			it.Before(func() {
-				tw := tar.NewWriter(tarFile)
-				defer tw.Close()
-				err = archive.CreateSingleFileTar(tw, "./file1", "file-1 content")
+				err = archive.CreateSingleFileTar(tarFile.Name(), "./file1", "file-1 content")
 				h.AssertNil(t, err)
 			})
 
 			it("returns the file contents", func() {
-				_, err = tarFile.Seek(0, 0)
-				h.AssertNil(t, err)
 				_, contents, err := archive.ReadTarEntry(tarFile, "file1")
 				h.AssertNil(t, err)
 				h.AssertEq(t, string(contents), "file-1 content")

diff --git a/internal/archive/tar_builder.go b/internal/archive/tar_builder.go
@@ -3,7 +3,12 @@ package archive
 import (
 	"archive/tar"
 	"io"
+	"os"
 	"time"
+
+	"github.com/pkg/errors"
+
+	"github.com/buildpacks/pack/internal/style"
 )
 
 type TarBuilder struct {
@@ -37,38 +42,35 @@ func (t *TarBuilder) AddDir(path string, mode int64, modTime time.Time) {
 	})
 }
 
-func (t *TarBuilder) Reader() io.ReadCloser {
+func (t *TarBuilder) Reader(twf TarWriterFactory) io.ReadCloser {
 	pr, pw := io.Pipe()
 	go func() {
 		var err error
 		defer func() {
 			pw.CloseWithError(err)
 		}()
-		_, err = t.WriteTo(pw)
+		_, err = t.WriteTo(pw, twf)
 	}()
 
 	return pr
 }
 
-// func (t *TarBuilder) WriteToPath(path string) error {
-// 	fh, err := os.Create(path)
-// 	if err != nil {
-// 		return errors.Wrapf(err, "create file for tar: %s", style.Symbol(path))
-// 	}
-// 	defer fh.Close()
-//
-// 	_, err = t.WriteTo(fh)
-// 	return err
-// }
-
-func (t *TarBuilder) WriteTo(w io.Writer) (int64, error) {
-	tw := tar.NewWriter(w)
-	defer tw.Close()
-	return t.WriteToTarWriter(tw)
+func (t *TarBuilder) WriteToPath(path string, twf TarWriterFactory) error {
+	fh, err := os.Create(path)
+	if err != nil {
+		return errors.Wrapf(err, "create file for tar: %s", style.Symbol(path))
+	}
+	defer fh.Close()
+
+	_, err = t.WriteTo(fh, twf)
+	return err
 }
 
-func (t *TarBuilder) WriteToTarWriter(tw TarWriter) (int64, error) {
+func (t *TarBuilder) WriteTo(w io.Writer, twf TarWriterFactory) (int64, error) {
 	var written int64
+	tw := twf.NewTarWriter(w)
+	defer tw.Close()
+
 	for _, f := range t.files {
 		if err := tw.WriteHeader(&tar.Header{
 			Typeflag: f.typeFlag,

diff --git a/internal/builder/builder.go b/internal/builder/builder.go
@@ -552,17 +552,7 @@ func (b *Builder) orderLayer(order dist.Order, dest string) (string, error) {
 	}
 
 	layerTar := filepath.Join(dest, "order.tar")
-
-	fh, err := os.Create(layerTar)
-	if err != nil {
-		return "", err
-	}
-	defer fh.Close()
-
-	tw := b.layerWriterFactory.NewTarWriter(fh)
-	defer tw.Close()
-
-	err = archive.CreateSingleFileTar(tw, orderPath, contents)
+	err = layer.CreateSingleFileTar(layerTar, orderPath, contents, b.layerWriterFactory)
 	if err != nil {
 		return "", errors.Wrapf(err, "failed to create order.toml layer tar")
 	}
@@ -588,17 +578,7 @@ func (b *Builder) stackLayer(dest string) (string, error) {
 	}
 
 	layerTar := filepath.Join(dest, "stack.tar")
-
-	fh, err := os.Create(layerTar)
-	if err != nil {
-		return "", err
-	}
-	defer fh.Close()
-
-	tw := b.layerWriterFactory.NewTarWriter(fh)
-	defer tw.Close()
-
-	err = archive.CreateSingleFileTar(tw, stackPath, buf.String())
+	err = layer.CreateSingleFileTar(layerTar, stackPath, buf.String(), b.layerWriterFactory)
 	if err != nil {
 		return "", errors.Wrapf(err, "failed to create stack.toml layer tar")
 	}

diff --git a/internal/builder/builder_test.go b/internal/builder/builder_test.go
@@ -1,7 +1,6 @@
 package builder_test
 
 import (
-	"archive/tar"
 	"bytes"
 	"encoding/json"
 	"fmt"
@@ -319,13 +318,7 @@ func testBuilder(t *testing.T, when spec.G, it spec.S) {
 				defer os.RemoveAll(tmpDir)
 
 				layerFile := filepath.Join(tmpDir, "order.tar")
-				f, err := os.Create(layerFile)
-				h.AssertNil(t, err)
-				defer f.Close()
-
-				tw := tar.NewWriter(f)
-				defer tw.Close()
-				err = archive.CreateSingleFileTar(tw, "/cnb/order.toml", "some content")
+				err = archive.CreateSingleFileTar(layerFile, "/cnb/order.toml", "some content")
 				h.AssertNil(t, err)
 
 				h.AssertNil(t, baseImage.AddLayer(layerFile))

diff --git a/internal/dist/buildpack.go b/internal/dist/buildpack.go
@@ -124,23 +124,6 @@ func (b *distBlob) Open() (io.ReadCloser, error) {
 	return b.openFn(), nil
 }
 
-/*
- * Two formats for BP blob:
-   - Root format:
-	/
-	/buildpack.toml
-	/bin/
-	/bin/build
-	/bin/detect
-
-   - Dist format:
-	Hives/
-	Files/cnb/buildpacks/<bp ID>/<bp ver>
-	Files/cnb/buildpacks/<bp ID>/<bp ver>/bin/
-	...
-	File/cnb/buildpacks/<bp ID>/<bp ver>/buildpack.toml
-*/
-
 func toDistTar(tw archive.TarWriter, bpd BuildpackDescriptor, blob Blob) error {
 	ts := archive.NormalizedDateTime
 

diff --git a/internal/dist/buildpack_test.go b/internal/dist/buildpack_test.go
@@ -64,7 +64,7 @@ homepage = "http://geocities.com/cool-bp"
 [[stacks]]
 id = "some.stack.id"
 `))
-						return tarBuilder.Reader()
+						return tarBuilder.Reader(archive.DefaultTarWriterFactory)
 					},
 				},
 				archive.DefaultTarWriterFactory,
@@ -97,7 +97,7 @@ id = "some.stack.id"
 						tarBuilder.AddDir("bin", 0700, time.Now())
 						tarBuilder.AddFile("bin/detect", 0700, time.Now(), []byte("detect-contents"))
 						tarBuilder.AddFile("bin/build", 0700, time.Now(), []byte("build-contents"))
-						return tarBuilder.Reader()
+						return tarBuilder.Reader(archive.DefaultTarWriterFactory)
 					},
 				},
 				archive.DefaultTarWriterFactory,
@@ -157,7 +157,7 @@ version = "1.2.3"
 [[stacks]]
 id = "some.stack.id"
 `))
-					return tarBuilder.Reader()
+					return tarBuilder.Reader(archive.DefaultTarWriterFactory)
 				},
 			}
 
@@ -196,7 +196,7 @@ id = "some.stack.id"
 								tarBuilder := archive.TarBuilder{}
 								tarBuilder.AddFile("buildpack.toml", 0700, time.Now(), []byte(bpTOMLData))
 								tarBuilder.AddDir("some-dir", 0600, time.Now())
-								return tarBuilder.Reader()
+								return tarBuilder.Reader(archive.DefaultTarWriterFactory)
 							},
 						},
 						archive.DefaultTarWriterFactory,
@@ -222,7 +222,7 @@ id = "some.stack.id"
 								tarBuilder.AddFile("buildpack.toml", 0700, time.Now(), []byte(bpTOMLData))
 								tarBuilder.AddFile("bin/detect", 0600, time.Now(), []byte("detect-contents"))
 								tarBuilder.AddFile("bin/build", 0600, time.Now(), []byte("build-contents"))
-								return tarBuilder.Reader()
+								return tarBuilder.Reader(archive.DefaultTarWriterFactory)
 							},
 						},
 						archive.DefaultTarWriterFactory,
@@ -252,7 +252,7 @@ id = "some.stack.id"
 								tarBuilder := archive.TarBuilder{}
 								tarBuilder.AddFile("buildpack.toml", 0700, time.Now(), []byte(bpTOMLData))
 								tarBuilder.AddFile("some-file", 0700, time.Now(), []byte("some-data"))
-								return tarBuilder.Reader()
+								return tarBuilder.Reader(archive.DefaultTarWriterFactory)
 							},
 						},
 						archive.DefaultTarWriterFactory,
@@ -277,7 +277,7 @@ id = "some.stack.id"
 								tarBuilder := archive.TarBuilder{}
 								tarBuilder.AddFile("buildpack.toml", 0700, time.Now(), []byte(bpTOMLData))
 								tarBuilder.AddFile("some-file", 0600, time.Now(), []byte("some-data"))
-								return tarBuilder.Reader()
+								return tarBuilder.Reader(archive.DefaultTarWriterFactory)
 							},
 						},
 						archive.DefaultTarWriterFactory,
@@ -301,7 +301,7 @@ id = "some.stack.id"
 					&readerBlob{
 						openFn: func() io.ReadCloser {
 							tarBuilder := archive.TarBuilder{}
-							return tarBuilder.Reader()
+							return tarBuilder.Reader(archive.DefaultTarWriterFactory)
 						},
 					},
 					archive.DefaultTarWriterFactory,
@@ -323,7 +323,7 @@ version = "1.2.3"
 
 [[stacks]]
 id = "some.stack.id"`))
-							return tarBuilder.Reader()
+							return tarBuilder.Reader(archive.DefaultTarWriterFactory)
 						},
 					},
 					archive.DefaultTarWriterFactory,
@@ -346,7 +346,7 @@ version = "1.2.3"
 
 [[stacks]]
 id = "some.stack.id"`))
-							return tarBuilder.Reader()
+							return tarBuilder.Reader(archive.DefaultTarWriterFactory)
 						},
 					},
 					archive.DefaultTarWriterFactory,
@@ -368,7 +368,7 @@ version = ""
 
 [[stacks]]
 id = "some.stack.id"`))
-							return tarBuilder.Reader()
+							return tarBuilder.Reader(archive.DefaultTarWriterFactory)
 						},
 					},
 					archive.DefaultTarWriterFactory,
@@ -396,7 +396,7 @@ id = "some.stack.id"
   id = "bp.nested"
   version = "bp.nested.version"
 `))
-							return tarBuilder.Reader()
+							return tarBuilder.Reader(archive.DefaultTarWriterFactory)
 						},
 					},
 					archive.DefaultTarWriterFactory,
@@ -416,7 +416,7 @@ id = "some.stack.id"
 id = "bp.one"
 version = "1.2.3"
 `))
-							return tarBuilder.Reader()
+							return tarBuilder.Reader(archive.DefaultTarWriterFactory)
 						},
 					},
 					archive.DefaultTarWriterFactory,

diff --git a/internal/fakes/fake_buildpack.go b/internal/fakes/fake_buildpack.go
@@ -56,5 +56,5 @@ func (b *fakeBuildpack) Open() (io.ReadCloser, error) {
 		tarBuilder.AddFile(bpDir+"/bin/detect", b.chmod, ts, []byte("detect-contents"))
 	}
 
-	return tarBuilder.Reader(), nil
+	return tarBuilder.Reader(archive.DefaultTarWriterFactory), nil
 }
diff --git a/internal/fakes/fake_buildpack_blob.go b/internal/fakes/fake_buildpack_blob.go
@@ -44,5 +44,5 @@ func (b *fakeBuildpackBlob) Open() (reader io.ReadCloser, err error) {
 	tarBuilder.AddFile("bin/build", b.chmod, time.Now(), []byte("build-contents"))
 	tarBuilder.AddFile("bin/detect", b.chmod, time.Now(), []byte("detect-contents"))
 
-	return tarBuilder.Reader(), err
+	return tarBuilder.Reader(archive.DefaultTarWriterFactory), err
 }
diff --git a/internal/fakes/fake_images.go b/internal/fakes/fake_images.go
@@ -56,11 +56,7 @@ func NewFakeBuilderImage(t *testing.T, tmpDir, name string, stackID, uid, gid st
 	tarBuilder.AddFile("/cnb/order.toml", 0777, archive.NormalizedDateTime, orderTomlBytes.Bytes())
 
 	orderTar := filepath.Join(tmpDir, fmt.Sprintf("order.%s.toml", h.RandString(8)))
-	fh, err := os.Create(orderTar)
-	h.AssertNil(t, err)
-	defer fh.Close()
-	_, err = tarBuilder.WriteTo(fh)
-	h.AssertNil(t, err)
+	h.AssertNil(t, tarBuilder.WriteToPath(orderTar, archive.DefaultTarWriterFactory))
 	h.AssertNil(t, fakeBuilderImage.AddLayer(orderTar))
 
 	return fakeBuilderImage

diff --git a/internal/layer/factory.go b/internal/layer/factory.go
@@ -13,7 +13,6 @@ type tarWriterFactory struct {
 	os string
 }
 
-// TODO: Move to method on `imgutil.Image`
 func NewTarWriterFactory(image imgutil.Image) (archive.TarWriterFactory, error) {
 	os, err := image.OS()
 	if err != nil {
@@ -31,12 +30,3 @@ func (f tarWriterFactory) NewTarWriter(fileWriter io.Writer) archive.TarWriter {
 	// Linux images use tar.Writer
 	return tar.NewWriter(fileWriter)
 }
-
-/*
-imgutil      lifecycle
-    ^          ^
-     \        /
-        pack
-
-
-*/