Repository: goreleaser/nfpm
Pull request #431 (2021-12-28 14:03:31)

fix: create tree of dir content types
refs https://github.com/goreleaser/goreleaser/issues/2792

erikgeiser: This solution assumes that the contents are sorted because by creating the parent tree of `dir` you rely on the fact that none of the following `dir` entries (that possibly have different permissions, etc.) are already including in the parent tree of the earlier entry.

Now we do sort the contents, but in a completely different package. Keep in mind that just assuming order in the packagers might be a huge foot gun for those that use `nfpm` as a library.
codecov[bot]: # [Codecov](https://codecov.io/gh/goreleaser/nfpm/pull/431?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=goreleaser) Report
> Merging [#431](https://codecov.io/gh/goreleaser/nfpm/pull/431?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=goreleaser) (303c70b) into [main](https://codecov.io/gh/goreleaser/nfpm/commit/ebe9ddfb6f5a348f5d6edfef8f5ca0a839d4bbd9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=goreleaser) (ebe9ddf) will **decrease** coverage by `0.21%`.
> The diff coverage is `0.00%`.

[![Impacted file tree graph](https://codecov.io/gh/goreleaser/nfpm/pull/431/graphs/tree.svg?width=650&height=150&src=pr&token=9QKGIxn7wg&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=goreleaser)](https://codecov.io/gh/goreleaser/nfpm/pull/431?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=goreleaser)

```diff
@@            Coverage Diff             @@
##             main     #431      +/-   ##
==========================================
- Coverage   66.73%   66.52%   -0.22%     
==========================================
  Files          15       15              
  Lines        1876     1882       +6     
==========================================
  Hits         1252     1252              
- Misses        487      491       +4     
- Partials      137      139       +2     
```


| [Impacted Files](https://codecov.io/gh/goreleaser/nfpm/pull/431?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=goreleaser) | Coverage Δ | |
|---|---|---|
| [apk/apk.go](https://codecov.io/gh/goreleaser/nfpm/pull/431/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=goreleaser#diff-YXBrL2Fway5nbw==) | `72.07% <0.00%> (-0.63%)` | :arrow_down: |
| [deb/deb.go](https://codecov.io/gh/goreleaser/nfpm/pull/431/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=goreleaser#diff-ZGViL2RlYi5nbw==) | `70.16% <0.00%> (-0.44%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/goreleaser/nfpm/pull/431?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=goreleaser).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=goreleaser)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/goreleaser/nfpm/pull/431?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=goreleaser). Last update [ebe9ddf...303c70b](https://codecov.io/gh/goreleaser/nfpm/pull/431?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=goreleaser). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=goreleaser).

caarlos0: indeed, should we maybe sort the files in a different place then (in each impl)?

or add a godoc saying "if you use this directly you'll need to sort the files first"?

thoughts?
erikgeiser: Well we already create the tree of the `dir` entries already, just in the next loop.

I think the best solution would require some refactoring: We should build the actual tar contents first without writing them. Then each content entry can decide if it should overwrite existing entries or not. Like `createTree` would do nothing if an directory already exists and a `dir` entry would override an existing dir that may have been created by `createTree` before. Then we write it all afterwards. In this case order would be just cosmetics.
caarlos0: that seems like a rather big refactor... I think I'll release this as-is to fix the bug, and create an issue so we can investigate it in the future

Patch:
diff --git a/apk/apk.go b/apk/apk.go
index c5eb590..87fe03b 100644
--- a/apk/apk.go
+++ b/apk/apk.go
@@ -414,6 +414,10 @@ func createFilesInsideTarGz(info *nfpm.Info, tw *tar.Writer, created map[string]
 			continue
 		}
 
+		if err := createTree(tw, file.Destination, created); err != nil {
+			return err
+		}
+
 		normalizedName := normalizePath(strings.Trim(file.Destination, "/")) + "/"
 
 		if created[normalizedName] {
diff --git a/deb/deb.go b/deb/deb.go
index d5f8455..93309b6 100644
--- a/deb/deb.go
+++ b/deb/deb.go
@@ -280,6 +280,10 @@ func createFilesInsideDataTar(info *nfpm.Info, tw *tar.Writer,
 			continue
 		}
 
+		if err := createTree(tw, file.Destination, created); err != nil {
+			return md5buf, 0, err
+		}
+
 		normalizedName := normalizePath(strings.Trim(file.Destination, "/")) + "/"
 
 		if created[normalizedName] {
@@ -316,7 +320,7 @@ func createFilesInsideDataTar(info *nfpm.Info, tw *tar.Writer,
 		var size int64 // declare early to avoid shadowing err
 		switch file.Type {
 		case "ghost":
-			// skip ghost files in apk
+			// skip ghost files in deb
 			continue
 		case "dir":
 			// already handled above


Test patch:
diff --git a/apk/apk_test.go b/apk/apk_test.go
index bf09992..b943ed5 100644
--- a/apk/apk_test.go
+++ b/apk/apk_test.go
@@ -100,7 +100,7 @@ func TestCreateBuilderData(t *testing.T) {
 
 	require.NoError(t, builderData(tw))
 
-	require.Equal(t, 12288, buf.Len())
+	require.Equal(t, 11784, buf.Len())
 }
 
 func TestCombineToApk(t *testing.T) {
@@ -509,6 +509,10 @@ func TestDirectories(t *testing.T) {
 			Destination: "/etc/baz",
 			Type:        "dir",
 		},
+		{
+			Destination: "/usr/lib/something/somethingelse",
+			Type:        "dir",
+		},
 	}
 
 	require.NoError(t, info.Validate())
@@ -518,20 +522,29 @@ func TestDirectories(t *testing.T) {
 	err := createFilesInsideTarGz(info, tar.NewWriter(&buf), make(map[string]bool), &size)
 	require.NoError(t, err)
 
+	require.Equal(t, []string{
+		"etc/",
+		"etc/bar/",
+		"etc/baz/",
+		"usr/",
+		"usr/lib/",
+		"usr/lib/something/",
+		"usr/lib/something/somethingelse/",
+		"etc/bar/file",
+		"etc/foo/",
+		"etc/foo/file",
+	}, getTree(t, buf.Bytes()))
+
 	// for apks all implicit or explicit directories are created in the tarball
 	h := extractFileHeaderFromTar(t, buf.Bytes(), "/etc")
-	require.NoError(t, err)
 	require.Equal(t, h.Typeflag, byte(tar.TypeDir))
 	h = extractFileHeaderFromTar(t, buf.Bytes(), "/etc/foo")
-	require.NoError(t, err)
 	require.Equal(t, h.Typeflag, byte(tar.TypeDir))
 	h = extractFileHeaderFromTar(t, buf.Bytes(), "/etc/bar")
-	require.NoError(t, err)
 	require.Equal(t, h.Typeflag, byte(tar.TypeDir))
 	require.Equal(t, h.Mode, int64(0o700))
 	require.Equal(t, h.Uname, "test")
 	h = extractFileHeaderFromTar(t, buf.Bytes(), "/etc/baz")
-	require.NoError(t, err)
 	require.Equal(t, h.Typeflag, byte(tar.TypeDir))
 }
 
@@ -669,6 +682,24 @@ func extractFileHeaderFromTar(tb testing.TB, tarFile []byte, filename string) *t
 	return nil
 }
 
+func getTree(tb testing.TB, tarFile []byte) []string {
+	tb.Helper()
+
+	var result []string
+	tr := tar.NewReader(bytes.NewReader(tarFile))
+	for {
+		hdr, err := tr.Next()
+		if errors.Is(err, io.EOF) {
+			break // End of archive
+		}
+		require.NoError(tb, err)
+
+		result = append(result, hdr.Name)
+	}
+
+	return result
+}
+
 func TestArches(t *testing.T) {
 	for k := range archToAlpine {
 		t.Run(k, func(t *testing.T) {
diff --git a/deb/deb_test.go b/deb/deb_test.go
index deee52d..1a2f3a1 100644
--- a/deb/deb_test.go
+++ b/deb/deb_test.go
@@ -806,6 +806,10 @@ func TestDirectories(t *testing.T) {
 			Destination: "/etc/baz",
 			Type:        "dir",
 		},
+		{
+			Destination: "/usr/lib/something/somethingelse",
+			Type:        "dir",
+		},
 	}
 
 	require.NoError(t, info.Validate())
@@ -814,20 +818,38 @@ func TestDirectories(t *testing.T) {
 	require.NoError(t, err)
 	dataTarball := inflate(t, dataTarballName, deflatedDataTarball)
 
+	require.Equal(t, []string{
+		"./etc/",
+		"./etc/bar/",
+		"./etc/baz/",
+		"./usr/",
+		"./usr/lib/",
+		"./usr/lib/something/",
+		"./usr/lib/something/somethingelse/",
+		"./etc/bar/file",
+		"./etc/foo/",
+		"./etc/foo/file",
+	}, getTree(t, dataTarball))
+
 	// for debs all implicit or explicit directories are created in the tarball
 	h := extractFileHeaderFromTar(t, dataTarball, "/etc")
-	require.NoError(t, err)
 	require.Equal(t, h.Typeflag, byte(tar.TypeDir))
 	h = extractFileHeaderFromTar(t, dataTarball, "/etc/foo")
-	require.NoError(t, err)
 	require.Equal(t, h.Typeflag, byte(tar.TypeDir))
 	h = extractFileHeaderFromTar(t, dataTarball, "/etc/bar")
-	require.NoError(t, err)
 	require.Equal(t, h.Typeflag, byte(tar.TypeDir))
 	require.Equal(t, h.Mode, int64(0o700))
 	require.Equal(t, h.Uname, "test")
 	h = extractFileHeaderFromTar(t, dataTarball, "/etc/baz")
-	require.NoError(t, err)
+	require.Equal(t, h.Typeflag, byte(tar.TypeDir))
+
+	h = extractFileHeaderFromTar(t, dataTarball, "/usr")
+	require.Equal(t, h.Typeflag, byte(tar.TypeDir))
+	h = extractFileHeaderFromTar(t, dataTarball, "/usr/lib")
+	require.Equal(t, h.Typeflag, byte(tar.TypeDir))
+	h = extractFileHeaderFromTar(t, dataTarball, "/usr/lib/something")
+	require.Equal(t, h.Typeflag, byte(tar.TypeDir))
+	h = extractFileHeaderFromTar(t, dataTarball, "/usr/lib/something/somethingelse")
 	require.Equal(t, h.Typeflag, byte(tar.TypeDir))
 }
 
@@ -1103,6 +1125,24 @@ func tarContents(tb testing.TB, tarFile []byte) []string {
 	return contents
 }
 
+func getTree(tb testing.TB, tarFile []byte) []string {
+	tb.Helper()
+
+	var result []string
+	tr := tar.NewReader(bytes.NewReader(tarFile))
+	for {
+		hdr, err := tr.Next()
+		if errors.Is(err, io.EOF) {
+			break // End of archive
+		}
+		require.NoError(tb, err)
+
+		result = append(result, hdr.Name)
+	}
+
+	return result
+}
+
 func extractFileHeaderFromTar(tb testing.TB, tarFile []byte, filename string) *tar.Header {
 	tb.Helper()
 
diff --git a/rpm/rpm_test.go b/rpm/rpm_test.go
index d51d9c3..a1393a5 100644
--- a/rpm/rpm_test.go
+++ b/rpm/rpm_test.go
@@ -806,12 +806,24 @@ func TestDirectories(t *testing.T) {
 				Mode:  0o700,
 			},
 		},
+		{
+			Destination: "/usr/lib/something/somethingelse",
+			Type:        "dir",
+		},
 	}
 
 	var rpmFileBuffer bytes.Buffer
 	err := Default.Package(info, &rpmFileBuffer)
 	require.NoError(t, err)
 
+	require.Equal(t, []string{
+		"/etc/bar",
+		"/etc/bar/file",
+		"/etc/baz",
+		"/etc/foo/file",
+		"/usr/lib/something/somethingelse",
+	}, getTree(t, rpmFileBuffer.Bytes()))
+
 	// the directory /etc/foo should not be implicitly created as that
 	// implies ownership of /etc/foo which should always be implicit
 	_, err = extractFileHeaderFromRpm(rpmFileBuffer.Bytes(), "/etc/foo")
@@ -871,6 +883,27 @@ func extraFileInfoSliceFromRpm(rpm []byte) ([]rpmutils.FileInfo, error) {
 	return rpmFile.Header.GetFiles()
 }
 
+func getTree(tb testing.TB, rpm []byte) []string {
+	tb.Helper()
+
+	rpmFile, err := rpmutils.ReadRpm(bytes.NewReader(rpm))
+	require.NoError(tb, err)
+	pr, err := rpmFile.PayloadReader()
+	require.NoError(tb, err)
+
+	var tree []string
+	for {
+		hdr, err := pr.Next()
+		if errors.Is(err, io.EOF) {
+			break // End of archive
+		}
+		require.NoError(tb, err)
+		tree = append(tree, hdr.Filename())
+	}
+
+	return tree
+}
+
 func extractFileHeaderFromRpm(rpm []byte, filename string) (*cpio.Cpio_newc_header, error) {
 	rpmFile, err := rpmutils.ReadRpm(bytes.NewReader(rpm))
 	if err != nil {
diff --git a/testdata/acceptance/apk.dockerfile b/testdata/acceptance/apk.dockerfile
index 728a205..6842484 100644
--- a/testdata/acceptance/apk.dockerfile
+++ b/testdata/acceptance/apk.dockerfile
@@ -47,6 +47,7 @@ RUN test -f /usr/share/whatever/folder/folder2/file1
 RUN test -f /usr/share/whatever/folder/folder2/file2
 RUN test -d /var/log/whatever
 RUN test -d /usr/share/foo
+RUN test -d /usr/foo/bar/something
 RUN test $(stat -c %a /usr/sbin/fake) -eq 4755
 RUN test -f /tmp/preinstall-proof
 RUN test -f /tmp/postinstall-proof
@@ -61,6 +62,7 @@ RUN test -f /tmp/preremove-proof
 RUN test -f /tmp/postremove-proof
 RUN test ! -d /var/log/whatever
 RUN test ! -d /usr/share/foo
+RUN test ! -d /usr/foo/bar/something
 
 
 # ---- signed test ----
diff --git a/testdata/acceptance/core.complex.yaml b/testdata/acceptance/core.complex.yaml
index 4c7f608..bd4ac98 100644
--- a/testdata/acceptance/core.complex.yaml
+++ b/testdata/acceptance/core.complex.yaml
@@ -31,6 +31,8 @@ contents:
   dst: /usr/sbin/fake
   file_info:
     mode: 04755
+- dst: /usr/foo/bar/something
+  type: dir
 empty_folders:
 - /var/log/whatever
 - /usr/share/foo
@@ -46,4 +48,4 @@ rpm:
 apk:
   scripts:
     preupgrade: ./testdata/acceptance/scripts/preupgrade.sh
-    postupgrade: ./testdata/acceptance/scripts/postupgrade.sh
\ No newline at end of file
+    postupgrade: ./testdata/acceptance/scripts/postupgrade.sh
diff --git a/testdata/acceptance/deb.dockerfile b/testdata/acceptance/deb.dockerfile
index b495d18..7a60954 100644
--- a/testdata/acceptance/deb.dockerfile
+++ b/testdata/acceptance/deb.dockerfile
@@ -50,6 +50,7 @@ RUN test -f /usr/share/whatever/folder/folder2/file1
 RUN test -f /usr/share/whatever/folder/folder2/file2
 RUN test -d /var/log/whatever
 RUN test -d /usr/share/foo
+RUN test -d /usr/foo/bar/something
 RUN test $(stat -c %a /usr/sbin/fake) -eq 4755
 RUN test -f /tmp/preinstall-proof
 RUN test -f /tmp/postinstall-proof
@@ -64,6 +65,7 @@ RUN test -f /tmp/preremove-proof
 RUN test -f /tmp/postremove-proof
 RUN test ! -d /var/log/whatever
 RUN test ! -d /usr/share/foo
+RUN test ! -d /usr/foo/bar/something
 
 # ---- signed test ----
 FROM test_base AS signed
diff --git a/testdata/acceptance/rpm.dockerfile b/testdata/acceptance/rpm.dockerfile
index 300cac0..6a23f20 100644
--- a/testdata/acceptance/rpm.dockerfile
+++ b/testdata/acceptance/rpm.dockerfile
@@ -49,6 +49,7 @@ RUN test -f /usr/share/whatever/folder/folder2/file1
 RUN test -f /usr/share/whatever/folder/folder2/file2
 RUN test -d /var/log/whatever
 RUN test -d /usr/share/foo
+RUN test -d /usr/foo/bar/something
 RUN test $(stat -c %a /usr/sbin/fake) -eq 4755
 RUN test -f /tmp/preinstall-proof
 RUN test -f /tmp/postinstall-proof
@@ -65,6 +66,7 @@ RUN test -f /tmp/preremove-proof
 RUN test -f /tmp/postremove-proof
 RUN test ! -d /var/log/whatever
 RUN test ! -d /usr/share/foo
+RUN test ! -d /usr/foo/bar/something
 
 
 # ---- signed test ----