Parse PKGINFO once and reuse parsed structure everywhere

Instead of reparsing PKGINFO from the control filesystem over and over again, this does it once and as a tested helper on APKExpanded.

Behavior is the same as before with the difference of using `SplitN` over `Split` to not have a buggy example.

Author: markusthoemmes

pkg/apk/apk/implementation.go

@@ -1223,10 +1223,15 @@ func (a *APK) cachedPackage(ctx context.Context, pkg InstallablePackage, cacheDi
 		exp.SignatureHash = signatureHash[:]
 	}
 
-	datahash, err := a.datahash(exp.ControlFS)
+	pkgInfo, err := exp.PkgInfo()
 	if err != nil {
-		return nil, fmt.Errorf("datahash for %s: %w", pkg, err)
+		return nil, fmt.Errorf("reading pkginfo from %s: %w", pkg, err)
 	}
+	datahashVals := pkgInfo["datahash"]
+	if len(datahashVals) != 1 {
+		return nil, fmt.Errorf("saw %d datahash values", len(datahashVals))
+	}
+	datahash := datahashVals[0]
 
 	dat := filepath.Join(cacheDir, datahash+".dat.tar.gz")
 	df, err := os.Stat(dat)
@@ -1484,24 +1489,15 @@ func (a *APK) installPackage(ctx context.Context, pkg *Package, expanded *expand
 	}
 
 	// update the triggers
-	if err := a.updateTriggers(pkg, expanded.ControlFS); err != nil {
-		return nil, fmt.Errorf("unable to update triggers for pkg %s: %w", pkg.Name, err)
-	}
-
-	return installedFiles, nil
-}
-
-func (a *APK) datahash(controlFS fs.FS) (string, error) {
-	values, err := a.controlValue(controlFS, "datahash")
+	pkgInfo, err := expanded.PkgInfo()
 	if err != nil {
-		return "", fmt.Errorf("reading datahash from control: %w", err)
+		return nil, fmt.Errorf("reading pkginfo from %s: %w", pkg.Name, err)
 	}
-
-	if len(values) != 1 {
-		return "", fmt.Errorf("saw %d datahash values", len(values))
+	if err := a.updateTriggers(pkg, pkgInfo["triggers"]); err != nil {
+		return nil, fmt.Errorf("unable to update triggers for pkg %s: %w", pkg.Name, err)
 	}
 
-	return values[0], nil
+	return installedFiles, nil
 }
 
 func packageRefs(pkgs []*RepositoryPackage) []string {

pkg/apk/apk/installed.go

@@ -22,7 +22,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"io/fs"
 	"os"
 	"path/filepath"
 	"sort"
@@ -190,33 +189,14 @@ func (a *APK) readScriptsTar() (io.ReadCloser, error) {
 	return a.fs.Open(scriptsFilePath)
 }
 
-// TODO: We should probably parse control section on the first pass and reuse it.
-func (a *APK) controlValue(controlFs fs.FS, want string) ([]string, error) {
-	mapping, err := controlValue(controlFs, want)
-	if err != nil {
-		return nil, err
-	}
-
-	values, ok := mapping[want]
-	if !ok {
-		return []string{}, nil
-	}
-	return values, nil
-}
-
 // updateTriggers insert the triggers into the triggers file
-func (a *APK) updateTriggers(pkg *Package, controlFs fs.FS) error {
+func (a *APK) updateTriggers(pkg *Package, values []string) error {
 	triggers, err := a.fs.OpenFile(triggersFilePath, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0)
 	if err != nil {
 		return fmt.Errorf("unable to open triggers file %s: %w", triggersFilePath, err)
 	}
 	defer triggers.Close()
 
-	values, err := a.controlValue(controlFs, "triggers")
-	if err != nil {
-		return fmt.Errorf("updating triggers for %s: %w", pkg.Name, err)
-	}
-
 	for _, value := range values {
 		if _, err := fmt.Fprintf(triggers, "Q1%s %s\n", base64.StdEncoding.EncodeToString(pkg.Checksum), value); err != nil {
 			return fmt.Errorf("unable to write triggers file %s: %w", triggersFilePath, err)

pkg/apk/apk/installed_test.go

@@ -35,7 +35,6 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
-	"chainguard.dev/apko/internal/tarfs"
 	"chainguard.dev/apko/pkg/apk/expandapk"
 )
 
@@ -433,38 +432,10 @@ func TestUpdateTriggers(t *testing.T) {
 		Version:  "1.0.0",
 		Checksum: randBytes,
 	}
-	// this is not a fully valid PKGINFO file by any stretch, but for now it is sufficient
 	triggers := "/bin /usr/bin /foo /bar/*"
-	pkginfo := strings.Join([]string{
-		fmt.Sprintf("pkgname = %s", pkg.Name),
-		fmt.Sprintf("pkgver = %s", pkg.Version),
-		fmt.Sprintf("triggers = %s", triggers),
-	}, "\n")
-	// construct the controlTarGz
-	scripts := map[string][]byte{
-		".pre-install":  []byte("echo 'pre install'"),
-		".post-install": []byte("echo 'post install'"),
-		".pre-upgrade":  []byte("echo 'pre upgrade'"),
-		".post-upgrade": []byte("echo 'post upgrade'"),
-		".PKGINFO":      []byte(pkginfo),
-	}
-	var buf bytes.Buffer
-	tw := tar.NewWriter(&buf)
-	for name, content := range scripts {
-		_ = tw.WriteHeader(&tar.Header{
-			Name: name,
-			Mode: 0o644,
-			Size: int64(len(content)),
-		})
-		_, _ = tw.Write(content)
-	}
-	tw.Close()
 
-	// pass the controltargz to updateScriptsTar
-	r := bytes.NewReader(buf.Bytes())
-	fs, err := tarfs.New(r, int64(buf.Len()))
 	require.NoError(t, err, "unable to create tarfs: %v", err)
-	err = a.updateTriggers(pkg, fs)
+	err = a.updateTriggers(pkg, []string{triggers})
 	require.NoError(t, err, "unable to update triggers: %v", err)
 
 	// successfully wrote it; not check that it was written correctly

pkg/apk/apk/util.go

@@ -14,15 +14,6 @@
 
 package apk
 
-import (
-	"errors"
-	"fmt"
-	"io"
-	"io/fs"
-	"slices"
-	"strings"
-)
-
 func uniqify[T comparable](s []T) []T {
 	seen := make(map[T]struct{}, len(s))
 	uniq := make([]T, 0, len(s))
@@ -37,42 +28,3 @@ func uniqify[T comparable](s []T) []T {
 
 	return uniq
 }
-
-func controlValue(controlFs fs.FS, want ...string) (map[string][]string, error) {
-	f, err := controlFs.Open(".PKGINFO")
-	if err != nil {
-		if errors.Is(err, fs.ErrNotExist) {
-			return nil, fmt.Errorf("control file not found")
-		}
-		return nil, fmt.Errorf("opening .PKGINFO: %w", err)
-	}
-	defer f.Close()
-
-	b, err := io.ReadAll(f)
-	if err != nil {
-		return nil, fmt.Errorf("unable to read .PKGINFO from control tar.gz file: %w", err)
-	}
-	mapping := map[string][]string{}
-	lines := strings.SplitSeq(string(b), "\n")
-	for line := range lines {
-		parts := strings.Split(line, "=")
-		if len(parts) != 2 {
-			continue
-		}
-		key := strings.TrimSpace(parts[0])
-		if !slices.Contains(want, key) {
-			continue
-		}
-
-		values, ok := mapping[key]
-		if !ok {
-			values = []string{}
-		}
-
-		value := strings.TrimSpace(parts[1])
-		values = append(values, value)
-
-		mapping[key] = values
-	}
-	return mapping, nil
-}

pkg/apk/expandapk/expandapk.go

@@ -102,7 +102,45 @@ type APKExpanded struct {
 	SignatureSize int64
 
 	sync.Mutex
-	controlData []byte
+	parsedPkgInfo map[string][]string
+	controlData   []byte
+}
+
+// PkgInfo parses and returns the .PKGINFO file as a map of keys to values.
+func (a *APKExpanded) PkgInfo() (map[string][]string, error) {
+	a.Lock()
+	defer a.Unlock()
+	if a.parsedPkgInfo != nil {
+		return a.parsedPkgInfo, nil
+	}
+
+	f, err := a.ControlFS.Open(".PKGINFO")
+	if err != nil {
+		return nil, fmt.Errorf("opening .PKGINFO: %w", err)
+	}
+	defer f.Close()
+
+	scanner := bufio.NewScanner(f)
+	mapping := make(map[string][]string)
+	for scanner.Scan() {
+		line := scanner.Text()
+		parts := strings.SplitN(line, "=", 2)
+		if len(parts) != 2 {
+			continue
+		}
+		key := strings.TrimSpace(parts[0])
+		value := strings.TrimSpace(parts[1])
+
+		values := mapping[key]
+		values = append(values, value)
+		mapping[key] = values
+	}
+	if err := scanner.Err(); err != nil {
+		return nil, fmt.Errorf("reading .PKGINFO: %w", err)
+	}
+
+	a.parsedPkgInfo = mapping
+	return a.parsedPkgInfo, nil
 }
 
 func (a *APKExpanded) ControlData() ([]byte, error) {

pkg/apk/expandapk/expandapk_test.go

@@ -0,0 +1,119 @@
+package expandapk
+
+import (
+	"archive/tar"
+	"bytes"
+	"testing"
+
+	"chainguard.dev/apko/internal/tarfs"
+	"github.com/stretchr/testify/require"
+)
+
+func TestPkgInfo(t *testing.T) {
+	tests := []struct {
+		name    string
+		content string
+		want    map[string][]string
+	}{{
+		// See example at https://wiki.alpinelinux.org/wiki/Apk_spec
+		name: "example",
+		content: `
+# Generated by abuild 3.9.0-r2
+# using fakeroot version 1.25.3
+# Wed Jul  6 19:09:49 UTC 2022
+pkgname = busybox
+pkgver = 1.35.0-r18
+pkgdesc = Size optimized toolbox of many common UNIX utilities
+url = https://busybox.net/
+builddate = 1657134589
+packager = Buildozer <alpine-devel@lists.alpinelinux.org>
+size = 958464
+arch = x86_64
+origin = busybox
+commit = 332d2fff53cd4537d415e15e55e8ceb6fe6eaedb
+maintainer = Example <example+alpine@example.com>
+provider_priority = 100
+license = GPL-2.0-only
+replaces = busybox-initscripts
+provides = /bin/sh
+triggers = /bin /usr/bin /sbin /usr/sbin /lib/modules/*
+# automatically detected:
+provides = cmd:busybox=1.35.0-r18
+provides = cmd:sh=1.35.0-r18
+depend = so:libc.musl-x86_64.so.1
+datahash = 7d3351ac6c3ebaf18182efb5390061f50d077ce5ade60a15909d91278f70ada7
+`,
+		want: map[string][]string{
+			"pkgname":           {"busybox"},
+			"pkgver":            {"1.35.0-r18"},
+			"pkgdesc":           {"Size optimized toolbox of many common UNIX utilities"},
+			"url":               {"https://busybox.net/"},
+			"builddate":         {"1657134589"},
+			"packager":          {"Buildozer <alpine-devel@lists.alpinelinux.org>"},
+			"size":              {"958464"},
+			"arch":              {"x86_64"},
+			"origin":            {"busybox"},
+			"commit":            {"332d2fff53cd4537d415e15e55e8ceb6fe6eaedb"},
+			"maintainer":        {"Example <example+alpine@example.com>"},
+			"provider_priority": {"100"},
+			"license":           {"GPL-2.0-only"},
+			"replaces":          {"busybox-initscripts"},
+			"provides": {
+				"/bin/sh",
+				"cmd:busybox=1.35.0-r18",
+				"cmd:sh=1.35.0-r18",
+			},
+			"triggers": {"/bin /usr/bin /sbin /usr/sbin /lib/modules/*"},
+			"depend":   {"so:libc.musl-x86_64.so.1"},
+			"datahash": {"7d3351ac6c3ebaf18182efb5390061f50d077ce5ade60a15909d91278f70ada7"},
+		},
+	}, {
+		name: "empty",
+		content: `
+# Empty file
+`,
+		want: map[string][]string{},
+	}, {
+		name: "weird keys and values",
+		content: `
+foobar
+=nokey
+novalue=
+invalid=pair=with=equals
+validkey=validvalue
+`,
+		want: map[string][]string{
+			"":         {"nokey"},
+			"novalue":  {""},
+			"invalid":  {"pair=with=equals"},
+			"validkey": {"validvalue"},
+		},
+	}}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var buf bytes.Buffer
+			tw := tar.NewWriter(&buf)
+			err := tw.WriteHeader(&tar.Header{
+				Name: ".PKGINFO",
+				Mode: 0o644,
+				Size: int64(len([]byte(tt.content))),
+			})
+			require.NoError(t, err)
+			_, err = tw.Write([]byte(tt.content))
+			require.NoError(t, err)
+			require.NoError(t, tw.Close())
+
+			controlFs, err := tarfs.New(bytes.NewReader(buf.Bytes()), int64(buf.Len()))
+			require.NoError(t, err)
+
+			exp := &APKExpanded{ControlFS: controlFs}
+
+			got, err := exp.PkgInfo()
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			require.Equal(t, tt.want, got)
+		})
+	}
+}