Cleanups from PR review

internal/builder/builder.go

@@ -563,12 +563,12 @@ func (b *Builder) Save(logger logging.Logger, creatorMetadata CreatorMetadata) e
 
 func (b *Builder) addExplodedModules(kind string, logger logging.Logger, tmpDir string, image imgutil.Image, additionalModules []buildpack.BuildModule, layers dist.ModuleLayers) error {
 	collectionToAdd := map[string]moduleWithDiffID{}
-	splitAdditionalModules, errs := splitBuildModules(kind, tmpDir, additionalModules, logger)
+	toAdd, errs := explodeModules(kind, tmpDir, additionalModules, logger)
 	if len(errs) > 0 {
 		return e.Join(errs...)
 	}
 
-	for i, additionalModule := range splitAdditionalModules {
+	for i, additionalModule := range toAdd {
 		info, diffID, layerTar, module := additionalModule.module.Descriptor().Info(), additionalModule.diffID, additionalModule.tarPath, additionalModule.module
 
 		// check against builder layers
@@ -1143,15 +1143,19 @@ func sortKeys(collection map[string]moduleWithDiffID) []string {
 	return keys
 }
 
-// splitBuildModules takes the given Build Modules and concurrently read their internal tar files. In
-// case a flattened module is found, it will split each flattened buildpack into an individual module and skip all the empty ones
-// returns an explodedBuildModule array with the Build Module information but also the diffId and the tar file on disk
-func splitBuildModules(kind, tmpDir string, additionalModules []buildpack.BuildModule, logger logging.Logger) ([]moduleWithDiffID, []error) {
+// explodeModules takes a collection of build modules and concurrently reads their tar files.
+// It assumes the modules were extracted with `buildpack.extractBuildpacks`, which when provided a flattened buildpack package containing N buildpacks,
+// will return N modules: 1 module with a single tar containing ALL N buildpacks, and N-1 modules with empty tar files.
+// As we iterate through the modules, in case a flattened module (tar containing all N buildpacks) is found,
+// explodeModules will split the module into N modules, each with a single tar containing a single buildpack.
+// In case a module with an empty tar file is found, it is ignored.
+func explodeModules(kind, tmpDir string, additionalModules []buildpack.BuildModule, logger logging.Logger) ([]moduleWithDiffID, []error) {
 	modInfoChans := make([]chan modInfo, len(additionalModules))
 	for i := range modInfoChans {
 		modInfoChans[i] = make(chan modInfo, 1)
 	}
 
+	// Explode modules concurrently
 	for i, module := range additionalModules {
 		go func(i int, module buildpack.BuildModule) {
 			modTmpDir := filepath.Join(tmpDir, fmt.Sprintf("%s-%s", kind, strconv.Itoa(i)))
@@ -1166,44 +1170,59 @@ func splitBuildModules(kind, tmpDir string, additionalModules []buildpack.BuildM
 		}(i, module)
 	}
 
-	var result []moduleWithDiffID
-	var errs []error
-
-	// skip the flattened buildpacks that returned an empty tar file, their contents
-	// are included in a different Build Module that was split
+	// Iterate over modules sequentially, building up the result.
+	var (
+		result []moduleWithDiffID
+		errs   []error
+	)
 	for i, module := range additionalModules {
 		mi := <-modInfoChans[i]
 		if mi.err != nil {
 			errs = append(errs, mi.err)
 			continue
 		}
-		// moduleTar could be an individual Buildpack or flattened Buildpack that writes an empty tar on disk
-		for _, moduleTar := range mi.moduleTars {
-			explodedMod := moduleWithDiffID{tarPath: moduleTar.Path()}
+		if len(mi.moduleTars) == 1 {
+			// This entry is an individual buildpack or extension, or a module with empty tar
+			moduleTar := mi.moduleTars[0]
 			diffID, err := dist.LayerDiffID(moduleTar.Path())
 			if err != nil {
 				errs = append(errs, errors.Wrapf(err, "calculating layer diffID for path %s", moduleTar.Path()))
 				continue
 			}
 			if diffID.String() == emptyTarDiffID {
 				logger.Debugf("%s %s is a component of a flattened buildpack that will be added elsewhere, skipping...", istrings.Title(kind), style.Symbol(moduleTar.Info().FullName()))
-				continue // we don't need to keep empty tars
+				continue // we don't need to keep modules with empty tars
 			}
-			explodedMod.diffID = diffID.String()
-			if moduleTar.Info().FullName() == fmt.Sprintf("%s@%s", module.Descriptor().EscapedID(), module.Descriptor().Info().Version) ||
-				moduleTar.Info().FullName() == module.Descriptor().Info().FullName() {
-				explodedMod.module = module
-			} else {
-				// we need to match the exploded modules with its corresponding BuildModule.
-				// this is important when flattened modules where included
-				for _, additionalModule := range additionalModules {
-					if namesMatch(additionalModule, moduleTar) {
-						explodedMod.module = additionalModule
-						break
+			result = append(result, moduleWithDiffID{
+				tarPath: moduleTar.Path(),
+				diffID:  diffID.String(),
+				module:  module,
+			})
+		} else {
+			// This entry is a flattened buildpack that was exploded, we need to add each exploded buildpack to the result in order
+			for _, moduleTar := range mi.moduleTars {
+				diffID, err := dist.LayerDiffID(moduleTar.Path())
+				if err != nil {
+					errs = append(errs, errors.Wrapf(err, "calculating layer diffID for path %s", moduleTar.Path()))
+					continue
+				}
+				explodedMod := moduleWithDiffID{
+					tarPath: moduleTar.Path(),
+					diffID:  diffID.String(),
+				}
+				// find the module "info" for this buildpack - it could be the current module, or one of the modules with empty tars that was ignored
+				if namesMatch(module, moduleTar) {
+					explodedMod.module = module
+				} else {
+					for _, additionalModule := range additionalModules {
+						if namesMatch(additionalModule, moduleTar) {
+							explodedMod.module = additionalModule
+							break
+						}
 					}
 				}
+				result = append(result, explodedMod)
 			}
-			result = append(result, explodedMod)
 		}
 	}
 

internal/builder/builder_test.go

@@ -809,7 +809,7 @@ func testBuilder(t *testing.T, when spec.G, it spec.S) {
 						bpLayerString, err := json.Marshal(bpLayer)
 						h.AssertNil(t, err)
 
-						h.AssertNil(t, baseImage.SetLabel(
+						h.AssertNil(t, baseImage.SetLabel( // label builder as already having a buildpack with diffID `diffID`
 							dist.BuildpackLayersLabel,
 							string(bpLayerString),
 						))

pkg/buildpack/buildpack.go

@@ -7,7 +7,6 @@ import (
 	"os"
 	"path"
 	"path/filepath"
-	"runtime"
 	"strings"
 
 	"github.com/BurntSushi/toml"
@@ -31,7 +30,6 @@ type BuildModule interface {
 	// timestamp and root UID/GID).
 	Open() (io.ReadCloser, error)
 	Descriptor() Descriptor
-	ContainsFlattenedModules() bool
 }
 
 type Descriptor interface {
@@ -74,10 +72,6 @@ func (b *buildModule) Descriptor() Descriptor {
 	return b.descriptor
 }
 
-func (b *buildModule) ContainsFlattenedModules() bool {
-	return b.flattened
-}
-
 // FromBlob constructs a buildpack or extension from a blob. It is assumed that the buildpack
 // contents are structured as per the distribution spec (currently '/cnb/buildpacks/{ID}/{version}/*' or
 // '/cnb/extensions/{ID}/{version}/*').
@@ -356,10 +350,6 @@ func ToLayerTar(dest string, module BuildModule) (string, error) {
 }
 
 func ToNLayerTar(dest string, module BuildModule) ([]ModuleTar, error) {
-	if !module.ContainsFlattenedModules() {
-		return handleSingleOrEmptyModule(dest, module)
-	}
-
 	modReader, err := module.Open()
 	if err != nil {
 		return nil, errors.Wrap(err, "opening blob")
@@ -369,22 +359,40 @@ func ToNLayerTar(dest string, module BuildModule) ([]ModuleTar, error) {
 	tarCollection := newModuleTarCollection(dest)
 	tr := tar.NewReader(modReader)
 
-	// read the first header
-	header, err := tr.Next()
-	if err != nil {
-		if err == io.EOF {
-			return handleSingleOrEmptyModule(dest, module)
+	var (
+		header     *tar.Header
+		forWindows bool
+	)
+
+	for {
+		header, err = tr.Next()
+		if err != nil {
+			if err == io.EOF {
+				return handleSingleOrEmptyModule(dest, module)
+			}
+			return nil, err
+		}
+		if header.Name == "Files" {
+			forWindows = true
+		}
+		if strings.Contains(header.Name, `/cnb/buildpacks/`) || strings.Contains(header.Name, `\cnb\buildpacks\`) {
+			// Only for Windows, the first four headers are:
+			// - Files
+			// - Hives
+			// - Files/cnb
+			// - Files/cnb/buildpacks
+			// Skip over these until we find "Files/cnb/buildpacks/<buildpack-id>":
+			break
 		}
-		return nil, fmt.Errorf("failed to read first header '%s': %w", header.Name, err)
 	}
-
-	// the original version should be blank because the first header should look like "/cnb/buildpacks/<buildpack-id>"
+	// The header should look like "/cnb/buildpacks/<buildpack-id>"
+	// The version should be blank because the first header is missing <buildpack-version>.
 	origID, origVersion := parseBpIDAndVersion(header)
 	if origVersion != "" {
 		return nil, fmt.Errorf("first header '%s' contained unexpected version", header.Name)
 	}
 
-	if err := toNLayerTar(origID, origVersion, header, tr, tarCollection); err != nil {
+	if err := toNLayerTar(origID, origVersion, header, tr, tarCollection, forWindows); err != nil {
 		return nil, err
 	}
 
@@ -396,36 +404,14 @@ func ToNLayerTar(dest string, module BuildModule) ([]ModuleTar, error) {
 	return tarCollection.moduleTars(), nil
 }
 
-func toNLayerTar(origID, origVersion string, firstHeader *tar.Header, tr *tar.Reader, tc *moduleTarCollection) error {
+func toNLayerTar(origID, origVersion string, firstHeader *tar.Header, tr *tar.Reader, tc *moduleTarCollection, forWindows bool) error {
 	toWrite := []*tar.Header{firstHeader}
-	if runtime.GOOS == "windows" && !strings.Contains(firstHeader.Name, "/cnb/buildpacks/") {
-		// On windows we can see the following headers before finding */cnb/buildpacks/<buildpack-id>/*
-		// Files
-		// Hives
-		// Files/cnb
-		// Files/cnb/buildpacks
-		for {
-			next, err := tr.Next()
-			if err != nil {
-				return err
-			}
-			if !strings.Contains(next.Name, "/cnb/buildpacks/") {
-				toWrite = append(toWrite, next)
-			} else {
-				origID, origVersion = parseBpIDAndVersion(next)
-				tc.buffer = toWrite
-				toWrite = []*tar.Header{next}
-				break
-			}
-		}
-	}
-
 	if origVersion == "" {
 		// the first header only contains the id - e.g., /cnb/buildpacks/<buildpack-id>,
 		// read the next header to get the version
 		secondHeader, err := tr.Next()
 		if err != nil {
-			return err
+			return fmt.Errorf("getting second header: %w; first header was %s", err, firstHeader.Name)
 		}
 		nextID, nextVersion := parseBpIDAndVersion(secondHeader)
 		if nextID != origID || nextVersion == "" {
@@ -440,16 +426,13 @@ func toNLayerTar(origID, origVersion string, firstHeader *tar.Header, tr *tar.Re
 		realFirstHeader.Name = filepath.ToSlash(filepath.Dir(firstHeader.Name))
 		toWrite = append([]*tar.Header{&realFirstHeader}, toWrite...)
 	}
+	if forWindows {
+		toWrite = append(windowsPreamble(), toWrite...)
+	}
 	mt, err := tc.get(origID, origVersion)
 	if err != nil {
-		return err
+		return fmt.Errorf("getting module from collection: %w", err)
 	}
-	for _, h := range tc.buffer {
-		if err := mt.writer.WriteHeader(h); err != nil {
-			return fmt.Errorf("failed to write header '%s': %w", h.Name, err)
-		}
-	}
-
 	for _, h := range toWrite {
 		if err := mt.writer.WriteHeader(h); err != nil {
 			return fmt.Errorf("failed to write header '%s': %w", h.Name, err)
@@ -463,31 +446,52 @@ func toNLayerTar(origID, origVersion string, firstHeader *tar.Header, tr *tar.Re
 			if err == io.EOF {
 				return nil
 			}
-			return err
+			return fmt.Errorf("getting next header: %w", err)
 		}
 		nextID, nextVersion := parseBpIDAndVersion(header)
 		if nextID != origID || nextVersion != origVersion {
 			// we found a new module, recurse
-			return toNLayerTar(nextID, nextVersion, header, tr, tc)
+			return toNLayerTar(nextID, nextVersion, header, tr, tc, forWindows)
 		}
 
 		err = mt.writer.WriteHeader(header)
 		if err != nil {
-			return errors.Wrapf(err, "failed to write header for '%s'", header.Name)
+			return fmt.Errorf("failed to write header for '%s': %w", header.Name, err)
 		}
 
 		buf, err := io.ReadAll(tr)
 		if err != nil {
-			return errors.Wrapf(err, "failed to read contents of '%s'", header.Name)
+			return fmt.Errorf("failed to read contents of '%s': %w", header.Name, err)
 		}
 
 		_, err = mt.writer.Write(buf)
 		if err != nil {
-			return errors.Wrapf(err, "failed to write contents to '%s'", header.Name)
+			return fmt.Errorf("failed to write contents to '%s': %w", header.Name, err)
 		}
 	}
 }
 
+func windowsPreamble() []*tar.Header {
+	return []*tar.Header{
+		{
+			Name:     "Files",
+			Typeflag: tar.TypeDir,
+		},
+		{
+			Name:     "Hives",
+			Typeflag: tar.TypeDir,
+		},
+		{
+			Name:     "Files/cnb",
+			Typeflag: tar.TypeDir,
+		},
+		{
+			Name:     "Files/cnb/buildpacks",
+			Typeflag: tar.TypeDir,
+		},
+	}
+}
+
 func parseBpIDAndVersion(hdr *tar.Header) (id, version string) {
 	// splitting "/cnb/buildpacks/{ID}/{version}/*" returns
 	// [0] = "" -> first element is empty or "Files" in windows
@@ -496,7 +500,7 @@ func parseBpIDAndVersion(hdr *tar.Header) (id, version string) {
 	// [3] = "{ID}"
 	// [4] = "{version}"
 	// ...
-	parts := strings.Split(hdr.Name, "/")
+	parts := strings.Split(strings.ReplaceAll(filepath.Clean(hdr.Name), `\`, `/`), `/`)
 	size := len(parts)
 	switch {
 	case size < 4:
@@ -571,14 +575,12 @@ func newModuleTar(dest, id, version string) (moduleTar, error) {
 type moduleTarCollection struct {
 	rootPath string
 	modules  map[string]moduleTar
-	buffer   []*tar.Header
 }
 
 func newModuleTarCollection(rootPath string) *moduleTarCollection {
 	return &moduleTarCollection{
 		rootPath: rootPath,
 		modules:  map[string]moduleTar{},
-		buffer:   []*tar.Header{},
 	}
 }
 

pkg/buildpack/buildpack_test.go

@@ -874,10 +874,6 @@ func (eb *errorBuildModule) Descriptor() buildpack.Descriptor {
 	return nil
 }
 
-func (eb *errorBuildModule) ContainsFlattenedModules() bool {
-	return eb.flattened
-}
-
 type expectedBuildpack struct {
 	id      string
 	version string