Merge pull request #1621 from KereMath/fix/skel-appstream-publish

publish: fix duplicated skel/AppStream files and missing by-hash entries

AUTHORS

@@ -84,3 +84,4 @@ List of contributors, in chronological order:
 * Zhang Xiao (https://github.com/xzhang1)
 * Tom Nguyen (https://github.com/lecafard)
 * Philip Cramer (https://github.com/PhilipCramer)
+* Kerem Gur (https://github.com/KereMath)

deb/index_files.go

@@ -397,11 +397,12 @@ func (files *indexFiles) SkelIndex(component, path string) *indexFile {
 		relativePath := filepath.Join(component, path)
 
 		file = &indexFile{
-			parent:       files,
-			discardable:  false,
-			compressable: false,
-			onlyGzip:     false,
-			relativePath: relativePath,
+			parent:        files,
+			discardable:   false,
+			compressable:  false,
+			onlyGzip:      false,
+			acquireByHash: files.acquireByHash,
+			relativePath:  relativePath,
 		}
 
 		files.indexes[key] = file

deb/publish.go

@@ -1043,62 +1043,6 @@ func (p *PublishedRepo) Publish(packagePool aptly.PackagePool, publishedStorageP
 			}
 		}
 
-		for component := range p.sourceItems {
-			skelFiles, err := p.GetSkelFiles(skelDir, component)
-			if err != nil {
-				return fmt.Errorf("unable to get skeleton files: %v", err)
-			}
-
-			for relPath, absPath := range skelFiles {
-				bufWriter, err := indexes.SkelIndex(component, relPath).BufWriter()
-				if err != nil {
-					return fmt.Errorf("unable to generate skeleton index: %v", err)
-				}
-
-				file, err := os.Open(absPath)
-				if err != nil {
-					return fmt.Errorf("unable to read skeleton file: %v", err)
-				}
-
-				_, err = bufio.NewReader(file).WriteTo(bufWriter)
-				if err != nil {
-					return fmt.Errorf("unable to write skeleton file: %v", err)
-				}
-			}
-		}
-
-		// Pass-through AppStream (DEP-11) files from snapshots
-		for component, item := range p.sourceItems {
-			if item.snapshot == nil || len(item.snapshot.AppStreamFiles) == 0 {
-				continue
-			}
-
-			prefix := component + "/"
-			for relPath, poolPath := range item.snapshot.AppStreamFiles {
-				if !strings.HasPrefix(relPath, prefix) {
-					continue
-				}
-				withinComponent := strings.TrimPrefix(relPath, prefix)
-
-				poolFile, err := packagePool.Open(poolPath)
-				if err != nil {
-					return fmt.Errorf("unable to open AppStream file from pool: %v", err)
-				}
-
-				bufWriter, err := indexes.SkelIndex(component, withinComponent).BufWriter()
-				if err != nil {
-					_ = poolFile.Close()
-					return fmt.Errorf("unable to generate AppStream index: %v", err)
-				}
-
-				_, err = bufio.NewReader(poolFile).WriteTo(bufWriter)
-				_ = poolFile.Close()
-				if err != nil {
-					return fmt.Errorf("unable to write AppStream file: %v", err)
-				}
-			}
-		}
-
 		udebs := []bool{false}
 		if hadUdebs {
 			udebs = append(udebs, true)
@@ -1144,6 +1088,66 @@ func (p *PublishedRepo) Publish(packagePool aptly.PackagePool, publishedStorageP
 		}
 	}
 
+	// Skeleton and AppStream (DEP-11) files are keyed by component and already
+	// iterate over every component, so they must be emitted once for the whole
+	// publish -- not once per component, which would append each payload again.
+	for component := range p.sourceItems {
+		skelFiles, err := p.GetSkelFiles(skelDir, component)
+		if err != nil {
+			return fmt.Errorf("unable to get skeleton files: %v", err)
+		}
+
+		for relPath, absPath := range skelFiles {
+			bufWriter, err := indexes.SkelIndex(component, relPath).BufWriter()
+			if err != nil {
+				return fmt.Errorf("unable to generate skeleton index: %v", err)
+			}
+
+			file, err := os.Open(absPath)
+			if err != nil {
+				return fmt.Errorf("unable to read skeleton file: %v", err)
+			}
+
+			_, err = bufio.NewReader(file).WriteTo(bufWriter)
+			_ = file.Close()
+			if err != nil {
+				return fmt.Errorf("unable to write skeleton file: %v", err)
+			}
+		}
+	}
+
+	// Pass-through AppStream (DEP-11) files from snapshots
+	for component, item := range p.sourceItems {
+		if item.snapshot == nil || len(item.snapshot.AppStreamFiles) == 0 {
+			continue
+		}
+
+		prefix := component + "/"
+		for relPath, poolPath := range item.snapshot.AppStreamFiles {
+			if !strings.HasPrefix(relPath, prefix) {
+				continue
+			}
+			withinComponent := strings.TrimPrefix(relPath, prefix)
+
+			poolFile, err := packagePool.Open(poolPath)
+			if err != nil {
+				return fmt.Errorf("unable to open AppStream file from pool: %v", err)
+			}
+
+			bufWriter, err := indexes.SkelIndex(component, withinComponent).BufWriter()
+			if err != nil {
+				_ = poolFile.Close()
+				return fmt.Errorf("unable to generate AppStream index: %v", err)
+			}
+
+			_, err = bufio.NewReader(poolFile).WriteTo(bufWriter)
+			_ = poolFile.Close()
+			if err != nil {
+				return fmt.Errorf("unable to write AppStream file: %v", err)
+			}
+		}
+	}
+
 	for _, arch := range p.Architectures {
 		for _, udeb := range []bool{true, false} {
 			index := legacyContentIndexes[fmt.Sprintf("%s-%v", arch, udeb)]
@@ -1208,8 +1212,8 @@ func (p *PublishedRepo) Publish(packagePool aptly.PackagePool, publishedStorageP
 		release["Valid-Until"] = publishDate.AddDate(100, 0, 0).Format(datetimeformat)
 	}
 	if p.Version != "" {
-        release["Version"] = p.Version
-    }
+		release["Version"] = p.Version
+	}
 	release["Description"] = " Generated by aptly\n"
 	release["MD5Sum"] = ""
 	release["SHA1"] = ""

deb/publish_test.go

@@ -6,8 +6,11 @@ import (
 	"errors"
 	"fmt"
 	"os"
+	"os/exec"
 	"path/filepath"
+	"runtime"
 	"sort"
+	"strings"
 
 	"github.com/aptly-dev/aptly/aptly"
 	"github.com/aptly-dev/aptly/database"
@@ -501,6 +504,240 @@ func (s *PublishedRepoSuite) TestPublishAppStream(c *C) {
 	c.Assert(err, ErrorMatches, "unable to open AppStream file from pool.*")
 }
 
+func (s *PublishedRepoSuite) importAppStreamFile(c *C, name string, content []byte) string {
+	tmpFile := filepath.Join(c.MkDir(), name)
+	c.Assert(os.WriteFile(tmpFile, content, 0644), IsNil)
+
+	checksums := utils.ChecksumInfo{Size: int64(len(content))}
+	poolPath, err := s.packagePool.Import(tmpFile, name, &checksums, false, s.cs)
+	c.Assert(err, IsNil)
+
+	return poolPath
+}
+
+func (s *PublishedRepoSuite) TestPublishAppStreamMultipleComponents(c *C) {
+	mainContent := []byte("DEP-11 content for main\n")
+	contribContent := []byte("DEP-11 content for contrib\n")
+
+	s.snapshot.AppStreamFiles = map[string]string{
+		"main/dep11/Components-amd64.yml.gz": s.importAppStreamFile(c, "Components-main.yml.gz", mainContent),
+	}
+	s.snapshot2.AppStreamFiles = map[string]string{
+		"contrib/dep11/Components-amd64.yml.gz": s.importAppStreamFile(c, "Components-contrib.yml.gz", contribContent),
+	}
+
+	err := s.repo3.Publish(s.packagePool, s.provider, s.factory, &NullSigner{}, nil, false, "")
+	c.Assert(err, IsNil)
+
+	base := filepath.Join(s.publishedStorage.PublicPath(), "linux/dists/natty")
+
+	published, err := os.ReadFile(filepath.Join(base, "main/dep11/Components-amd64.yml.gz"))
+	c.Assert(err, IsNil)
+	c.Check(published, DeepEquals, mainContent)
+
+	published, err = os.ReadFile(filepath.Join(base, "contrib/dep11/Components-amd64.yml.gz"))
+	c.Assert(err, IsNil)
+	c.Check(published, DeepEquals, contribContent)
+}
+
+func (s *PublishedRepoSuite) TestPublishAppStreamAcquireByHash(c *C) {
+	s.snapshot.AppStreamFiles = map[string]string{
+		"main/dep11/Components-amd64.yml.gz": s.importAppStreamFile(c, "Components-amd64.yml.gz", []byte("DEP-11 content\n")),
+	}
+
+	s.repo.AcquireByHash = true
+	err := s.repo.Publish(s.packagePool, s.provider, s.factory, &NullSigner{}, nil, false, "")
+	c.Assert(err, IsNil)
+
+	base := filepath.Join(s.publishedStorage.PublicPath(), "ppa/dists/squeeze")
+	c.Check(filepath.Join(base, "main/dep11/by-hash/SHA256"), PathExists)
+}
+
+// setupSmallTempFS points TMPDIR at a ~1MB filesystem so that Publish's
+// temporary index files run out of space mid-write. Follows the DiskFullSuite
+// convention: /smallfs is pre-mounted in CI (see docker-unit-test), a loopback
+// mount is created when running as root, otherwise the test is skipped.
+func (s *PublishedRepoSuite) setupSmallTempFS(c *C) func() {
+	if runtime.GOOS != "linux" {
+		c.Skip("disk full tests only run on Linux")
+	}
+
+	mountPoint := "/smallfs"
+	mounted := false
+	if os.Geteuid() == 0 {
+		mountPoint = filepath.Join(c.MkDir(), "smallfs")
+		c.Assert(os.MkdirAll(mountPoint, 0777), IsNil)
+		fsImage := filepath.Join(c.MkDir(), "small.img")
+		c.Assert(exec.Command("dd", "if=/dev/zero", "of="+fsImage, "bs=1M", "count=1").Run(), IsNil)
+		c.Assert(exec.Command("mkfs.ext4", "-F", fsImage).Run(), IsNil)
+		c.Assert(exec.Command("mount", "-o", "loop", fsImage, mountPoint).Run(), IsNil)
+		mounted = true
+	} else if _, err := os.Stat(mountPoint); err != nil {
+		c.Skip("/smallfs is not mounted")
+	}
+
+	// remove leftovers from other tests sharing the mount
+	entries, err := os.ReadDir(mountPoint)
+	c.Assert(err, IsNil)
+	for _, entry := range entries {
+		if entry.Name() == "lost+found" {
+			continue
+		}
+		c.Assert(os.RemoveAll(filepath.Join(mountPoint, entry.Name())), IsNil)
+	}
+
+	oldTMPDIR, hadTMPDIR := os.LookupEnv("TMPDIR")
+	c.Assert(os.Setenv("TMPDIR", mountPoint), IsNil)
+
+	return func() {
+		if hadTMPDIR {
+			_ = os.Setenv("TMPDIR", oldTMPDIR)
+		} else {
+			_ = os.Unsetenv("TMPDIR")
+		}
+		if mounted {
+			_ = exec.Command("umount", mountPoint).Run()
+		}
+	}
+}
+
+func (s *PublishedRepoSuite) TestPublishSkelFiles(c *C) {
+	skelDir := c.MkDir()
+	dir := filepath.Join(skelDir, "ppa", "dists", "squeeze", "main")
+	c.Assert(os.MkdirAll(filepath.Join(dir, "extra"), 0755), IsNil)
+	c.Assert(os.WriteFile(filepath.Join(dir, "top.txt"), []byte("top-level skel file\n"), 0644), IsNil)
+	c.Assert(os.WriteFile(filepath.Join(dir, "extra", "metadata.json"), []byte("{\"nested\": true}\n"), 0644), IsNil)
+
+	err := s.repo.Publish(s.packagePool, s.provider, s.factory, &NullSigner{}, nil, false, skelDir)
+	c.Assert(err, IsNil)
+
+	base := filepath.Join(s.publishedStorage.PublicPath(), "ppa/dists/squeeze/main")
+
+	published, err := os.ReadFile(filepath.Join(base, "top.txt"))
+	c.Assert(err, IsNil)
+	c.Check(string(published), Equals, "top-level skel file\n")
+
+	published, err = os.ReadFile(filepath.Join(base, "extra", "metadata.json"))
+	c.Assert(err, IsNil)
+	c.Check(string(published), Equals, "{\"nested\": true}\n")
+
+	// Release file should reference skel files
+	rf, err := os.Open(filepath.Join(s.publishedStorage.PublicPath(), "ppa/dists/squeeze/Release"))
+	c.Assert(err, IsNil)
+	defer func() { _ = rf.Close() }()
+
+	cfr := NewControlFileReader(rf, true, false)
+	st, err := cfr.ReadStanza()
+	c.Assert(err, IsNil)
+
+	c.Check(st["SHA256"], Matches, "(?s).*main/top\\.txt.*")
+	c.Check(st["SHA256"], Matches, "(?s).*main/extra/metadata\\.json.*")
+}
+
+func (s *PublishedRepoSuite) TestPublishSkelFilesMultipleComponents(c *C) {
+	// regression test: skel files used to be emitted once per component,
+	// appending each payload to the index again (issue fixed in this change)
+	skelDir := c.MkDir()
+	mainDir := filepath.Join(skelDir, "linux", "dists", "natty", "main")
+	contribDir := filepath.Join(skelDir, "linux", "dists", "natty", "contrib")
+	c.Assert(os.MkdirAll(mainDir, 0755), IsNil)
+	c.Assert(os.MkdirAll(contribDir, 0755), IsNil)
+	c.Assert(os.WriteFile(filepath.Join(mainDir, "main.txt"), []byte("skel for main\n"), 0644), IsNil)
+	c.Assert(os.WriteFile(filepath.Join(contribDir, "contrib.txt"), []byte("skel for contrib\n"), 0644), IsNil)
+
+	err := s.repo3.Publish(s.packagePool, s.provider, s.factory, &NullSigner{}, nil, false, skelDir)
+	c.Assert(err, IsNil)
+
+	base := filepath.Join(s.publishedStorage.PublicPath(), "linux/dists/natty")
+
+	// each payload must be published exactly once, not duplicated per component
+	published, err := os.ReadFile(filepath.Join(base, "main/main.txt"))
+	c.Assert(err, IsNil)
+	c.Check(string(published), Equals, "skel for main\n")
+
+	published, err = os.ReadFile(filepath.Join(base, "contrib/contrib.txt"))
+	c.Assert(err, IsNil)
+	c.Check(string(published), Equals, "skel for contrib\n")
+}
+
+func (s *PublishedRepoSuite) TestPublishSkelFilesWalkError(c *C) {
+	if runtime.GOOS == "windows" {
+		c.Skip("a file in the middle of a path is reported as not-exist on Windows")
+	}
+
+	// "dists" is a regular file, so walking skelDir/ppa/dists/squeeze/main
+	// fails with ENOTDIR, which is not swallowed as a not-exist error
+	skelDir := c.MkDir()
+	c.Assert(os.MkdirAll(filepath.Join(skelDir, "ppa"), 0755), IsNil)
+	c.Assert(os.WriteFile(filepath.Join(skelDir, "ppa", "dists"), []byte("not a directory"), 0644), IsNil)
+
+	err := s.repo.Publish(s.packagePool, s.provider, s.factory, &NullSigner{}, nil, false, skelDir)
+	c.Assert(err, ErrorMatches, "unable to get skeleton files: .*")
+}
+
+func (s *PublishedRepoSuite) TestPublishSkelFilesIndexError(c *C) {
+	// each name stays within NAME_MAX, but BufWriter flattens the relative
+	// path into a single temporary file name that exceeds it
+	skelDir := c.MkDir()
+	dir := filepath.Join(skelDir, "ppa", "dists", "squeeze", "main", strings.Repeat("a", 200))
+	c.Assert(os.MkdirAll(dir, 0755), IsNil)
+	c.Assert(os.WriteFile(filepath.Join(dir, strings.Repeat("b", 200)), []byte("skel content"), 0644), IsNil)
+
+	err := s.repo.Publish(s.packagePool, s.provider, s.factory, &NullSigner{}, nil, false, skelDir)
+	c.Assert(err, ErrorMatches, "unable to generate skeleton index: .*")
+}
+
+func (s *PublishedRepoSuite) TestPublishSkelFilesOpenError(c *C) {
+	if runtime.GOOS == "windows" || os.Geteuid() == 0 {
+		c.Skip("requires POSIX permissions and a non-root user")
+	}
+
+	skelDir := c.MkDir()
+	dir := filepath.Join(skelDir, "ppa", "dists", "squeeze", "main")
+	c.Assert(os.MkdirAll(dir, 0755), IsNil)
+	skelFile := filepath.Join(dir, "InRelease")
+	c.Assert(os.WriteFile(skelFile, []byte("skel content"), 0644), IsNil)
+	c.Assert(os.Chmod(skelFile, 0), IsNil)
+
+	err := s.repo.Publish(s.packagePool, s.provider, s.factory, &NullSigner{}, nil, false, skelDir)
+	c.Assert(err, ErrorMatches, "unable to read skeleton file: .*")
+}
+
+func (s *PublishedRepoSuite) TestPublishSkelFilesWriteError(c *C) {
+	cleanup := s.setupSmallTempFS(c)
+	defer cleanup()
+
+	skelDir := c.MkDir()
+	dir := filepath.Join(skelDir, "ppa", "dists", "squeeze", "main")
+	c.Assert(os.MkdirAll(dir, 0755), IsNil)
+	c.Assert(os.WriteFile(filepath.Join(dir, "Contents-huge"), bytes.Repeat([]byte{'x'}, 2*1024*1024), 0644), IsNil)
+
+	err := s.repo.Publish(s.packagePool, s.provider, s.factory, &NullSigner{}, nil, false, skelDir)
+	c.Assert(err, ErrorMatches, "unable to write skeleton file: .*")
+}
+
+func (s *PublishedRepoSuite) TestPublishAppStreamIndexError(c *C) {
+	// flattened temporary file name for the index exceeds NAME_MAX
+	s.snapshot.AppStreamFiles = map[string]string{
+		"main/" + strings.Repeat("a", 200) + "/" + strings.Repeat("b", 200): s.importAppStreamFile(c, "Components-amd64.yml.gz", []byte("DEP-11 content\n")),
+	}
+
+	err := s.repo.Publish(s.packagePool, s.provider, s.factory, &NullSigner{}, nil, false, "")
+	c.Assert(err, ErrorMatches, "unable to generate AppStream index: .*")
+}
+
+func (s *PublishedRepoSuite) TestPublishAppStreamWriteError(c *C) {
+	cleanup := s.setupSmallTempFS(c)
+	defer cleanup()
+
+	s.snapshot.AppStreamFiles = map[string]string{
+		"main/dep11/Components-amd64.yml.gz": s.importAppStreamFile(c, "Components-amd64.yml.gz", bytes.Repeat([]byte{'x'}, 2*1024*1024)),
+	}
+
+	err := s.repo.Publish(s.packagePool, s.provider, s.factory, &NullSigner{}, nil, false, "")
+	c.Assert(err, ErrorMatches, "unable to write AppStream file: .*")
+}
+
 func (s *PublishedRepoSuite) TestPublishNoSigner(c *C) {
 	err := s.repo.Publish(s.packagePool, s.provider, s.factory, nil, nil, false, "")
 	c.Assert(err, IsNil)

system/t06_publish/PublishRepo40Test_gold

@@ -0,0 +1,14 @@
+Loading packages...
+Generating metadata files and linking package files...
+Finalizing metadata files...
+Signing file 'Release' with gpg, please enter your passphrase when prompted:
+Clearsigning file 'Release' with gpg, please enter your passphrase when prompted:
+
+Local repos repo1, repo2 have been successfully published.
+Please setup your webserver to serve directory '${HOME}/.aptly/public' with autoindexing.
+Now you can add following line to apt sources:
+  deb http://your-server/ maverick contrib main
+  deb-src http://your-server/ maverick contrib main
+Don't forget to add your GPG key to apt with apt-key.
+
+You can also use `aptly serve` to publish your repositories over HTTP quickly.

system/t06_publish/repo.py

@@ -1098,3 +1098,34 @@ class PublishRepo39Test(BaseTest):
         self.check_equal(first_release, self.read_file('public/dists/maverick/Release'))
         self.check_equal(first_release_gpg, self.read_file('public/dists/maverick/Release.gpg'))
         self.check_equal(first_inrelease, self.read_file('public/dists/maverick/InRelease'))
+
+
+class PublishRepo40Test(BaseTest):
+    """
+    publish repo: skeleton files with multiple components
+    """
+    fixtureCmds = [
+        "aptly repo create repo1",
+        "aptly repo create repo2",
+        "aptly repo add repo1 ${files}/libboost-program-options-dev_1.49.0.1_i386.deb",
+        "aptly repo add repo2 ${files}/pyspi-0.6.1-1.3.stripped.dsc",
+    ]
+    runCmd = "aptly publish repo -keyring=${files}/aptly.pub -secret-keyring=${files}/aptly.sec -component=main,contrib -distribution=maverick -skip-contents repo1 repo2"
+    gold_processor = BaseTest.expand_environ
+
+    def prepare_fixture(self):
+        super(PublishRepo40Test, self).prepare_fixture()
+
+        self.write_file(os.path.join('skel', 'dists', 'maverick', 'main', 'dep11',
+                                     'Components-amd64.yml'), 'main dep11 payload')
+        self.write_file(os.path.join('skel', 'dists', 'maverick', 'contrib', 'dep11',
+                                     'Components-amd64.yml'), 'contrib dep11 payload')
+
+    def check(self):
+        super(PublishRepo40Test, self).check()
+
+        # each skeleton file is emitted once per publish, not once per component
+        self.check_equal(self.read_file('public/dists/maverick/main/dep11/Components-amd64.yml'),
+                         'main dep11 payload')
+        self.check_equal(self.read_file('public/dists/maverick/contrib/dep11/Components-amd64.yml'),
+                         'contrib dep11 payload')