Fix swift metrics unzip

Sources/DocUploadBundle/DocUploadBundle.swift

@@ -105,12 +105,19 @@ public struct DocUploadBundle {
         )
     }
 
-    public func zip(to workDir: String) throws -> String {
+    public func zip(to workDir: String, method: Zipper.Method = .library) throws -> String {
         let archiveURL = URL(fileURLWithPath: "\(workDir)/\(archiveName)")
         let metadataURL = URL(fileURLWithPath: "\(workDir)/metadata.json")
         try JSONEncoder().encode(metadata).write(to: metadataURL)
 
-        try Zipper.zip(paths: [metadataURL, URL(fileURLWithPath: sourcePath)], to: archiveURL)
+        switch method {
+            case .library, .zipTool(workingDirectory: .some(_)):
+                try Zipper.zip(paths: [metadataURL, URL(fileURLWithPath: sourcePath)], to: archiveURL, method: method)
+
+            case .zipTool(.none):
+                // By default, run the zip tool in the working directory
+                try Zipper.zip(paths: [metadataURL, URL(fileURLWithPath: sourcePath)], to: archiveURL, method: .zipTool(workingDirectory: workDir))
+        }
 
         return archiveURL.path
     }

Sources/DocUploadBundle/Zipper.swift

@@ -17,22 +17,51 @@ import Foundation
 import Zip
 
 
-enum Zipper {
-    static func zip(paths inputPaths: [URL], to outputPath: URL) throws {
-        try Zip.zipFiles(paths: inputPaths, zipFilePath: outputPath, password: nil, progress: nil)
-    }
+public enum Zipper {
+    public static func zip(paths inputPaths: [URL], to outputPath: URL, method: Method = .library) throws {
+        switch method {
+            case .library:
+                do { try Zip.zipFiles(paths: inputPaths, zipFilePath: outputPath, password: nil, progress: nil) } 
+                catch ZipError.fileNotFound { throw Error.fileNotFound }
+                catch ZipError.unzipFail { throw Error.unzipFail }
+                catch ZipError.zipFail { throw Error.zipFail }
+                catch { throw Error.generic(reason: "\(error)") }
 
-    static func unzip(from inputPath: URL, to outputPath: URL, fileOutputHandler: ((_ unzippedFile: URL) -> Void)? = nil) throws {
-        do {
-            try Zip.unzipFile(inputPath, destination: outputPath, overwrite: true, password: nil, fileOutputHandler: fileOutputHandler)
-        } catch ZipError.unzipFail {
-            // Try OS level unzip as a fallback
-            // See https://github.com/SwiftPackageIndex/SwiftPackageIndex-Server/issues/3069
-            let unzip = URL(fileURLWithPath: "/usr/bin/unzip")
-            let process = try Process.run(unzip, arguments: ["-q", inputPath.path, "-d", outputPath.path])
-            process.waitUntilExit()
+            case let .zipTool(cwd):
+                do {
+                    let process = Process()
+                    process.executableURL = zip
+                    process.arguments = ["-q", "-r", outputPath.path] + inputPaths.map(\.lastPathComponent)
+                    process.currentDirectoryURL = cwd.map(URL.init(fileURLWithPath:))
+                    try process.run()
+                    process.waitUntilExit()
+                } catch {
+                    throw Error.generic(reason: "\(error)")
+                }
         }
     }
+
+    public static func unzip(from inputPath: URL, to outputPath: URL, fileOutputHandler: ((_ unzippedFile: URL) -> Void)? = nil) throws {
+        do { try Zip.unzipFile(inputPath, destination: outputPath, overwrite: true, password: nil, fileOutputHandler: fileOutputHandler) }
+        catch ZipError.fileNotFound { throw Error.fileNotFound }
+        catch ZipError.unzipFail { throw Error.unzipFail }
+        catch ZipError.zipFail { throw Error.zipFail }
+        catch { throw Error.generic(reason: "\(error)") }
+    }
+
+    static let zip = URL(fileURLWithPath: "/usr/bin/zip")
+
+    public enum Method {
+        case library
+        case zipTool(workingDirectory: String? = nil)
+    }
+
+    public enum Error: Swift.Error {
+        case generic(reason: String)
+        case fileNotFound
+        case unzipFail
+        case zipFail
+    }
 }
 
 

Tests/DocUploadBundleTests/DocUploadBundleTests.swift

@@ -100,34 +100,4 @@ final class DocUploadBundleTests: XCTestCase {
                        .init(bucket: "spi-prod-docs", path: "owner/name/feature-2.0.0"))
     }
 
-    func test_issue_3069() async throws {
-        // https://github.com/SwiftPackageIndex/SwiftPackageIndex-Server/issues/3069
-        try await withTempDir { tempDir in
-            let url = fixtureUrl(for: "prod-apple-swift-metrics-main-e6a00d36.zip")
-            XCTAssertNoThrow(
-                try DocUploadBundle.unzip(bundle: url.path, outputPath: tempDir)
-            )
-            for pathComponent in ["metadata.json",
-                                  "main/index.html",
-                                  "main/index/index.json"] {
-                let path = tempDir + "/" + pathComponent
-                XCTAssertTrue(FileManager.default.fileExists(atPath: path), "does not exist: \(path)")
-            }
-            // test roundtrip, to ensure the zip library can zip/unzip its own product
-            // zip
-            let urls = [tempDir + "/metadata.json",
-                        tempDir + "/main"].map(URL.init(fileURLWithPath:))
-            let zipped = URL(fileURLWithPath: tempDir + "/out.zip")
-            try Zipper.zip(paths: urls, to: zipped)
-            XCTAssertTrue(FileManager.default.fileExists(atPath: zipped.path))
-            // unzip
-            let out = URL(fileURLWithPath: tempDir + "/out")
-            try Zipper.unzip(from: zipped, to: out)
-            XCTAssertTrue(FileManager.default.fileExists(atPath: out.path))
-            XCTAssertTrue(FileManager.default.fileExists(atPath: out.appendingPathComponent("metadata.json").path))
-            XCTAssertTrue(FileManager.default.fileExists(atPath: out.appendingPathComponent("main/index.html").path))
-            XCTAssertTrue(FileManager.default.fileExists(atPath: out.appendingPathComponent("main/index/index.json").path))
-        }
-    }
-
 }

Tests/DocUploadBundleTests/ZipTests.swift

@@ -57,6 +57,14 @@ final class ZipTests: XCTestCase {
             let fileB = subdir.appendingPathComponent("b.txt")
             try "b".write(to: fileB, atomically: true, encoding: .utf8)
 
+            // temp/subdir/subsubdir
+            let subsubdir = subdir.appendingPathComponent("subsubdir")
+            try FileManager.default.createDirectory(at: subsubdir, withIntermediateDirectories: false)
+
+            // temp/subdir/subdir/c.txt
+            let fileC = subsubdir.appendingPathComponent("c.txt")
+            try "c".write(to: fileC, atomically: true, encoding: .utf8)
+
             let zipFile = tempURL.appendingPathComponent("out.zip")
             try Zipper.zip(paths: [fileA, subdir], to: zipFile)
             XCTAssert(FileManager.default.fileExists(atPath: zipFile.path))
@@ -69,10 +77,64 @@ final class ZipTests: XCTestCase {
                 // roundtrip/subdir/b.txt
                 let fileA = roundtrip.appendingPathComponent("a.txt")
                 let fileB = roundtrip.appendingPathComponent("subdir").appendingPathComponent("b.txt")
+                let fileC = roundtrip.appendingPathComponent("subdir").appendingPathComponent("subsubdir").appendingPathComponent("c.txt")
+                XCTAssert(FileManager.default.fileExists(atPath: fileA.path))
+                XCTAssert(FileManager.default.fileExists(atPath: fileB.path))
+                XCTAssert(FileManager.default.fileExists(atPath: fileC.path))
+                XCTAssertEqual(try String(contentsOf: fileA), "a")
+                XCTAssertEqual(try String(contentsOf: fileB), "b")
+                XCTAssertEqual(try String(contentsOf: fileC), "c")
+            }
+        }
+    }
+
+    func test_zip_roundtrip_shellTool() async throws {
+        try XCTSkipIf(!FileManager.default.fileExists(atPath: Zipper.zip.path))
+
+        // Test basic zip roundtrip with the shellTool method
+        try await withTempDir { tempDir in
+            //  temp
+            let tempURL = URL(fileURLWithPath: tempDir)
+
+            // temp/a.txt
+            let fileA = tempURL.appendingPathComponent("a.txt")
+            try "a".write(to: fileA, atomically: true, encoding: .utf8)
+
+            // temp/subdir/
+            let subdir = tempURL.appendingPathComponent("subdir")
+            try FileManager.default.createDirectory(at: subdir, withIntermediateDirectories: false)
+
+            // temp/subdir/b.txt
+            let fileB = subdir.appendingPathComponent("b.txt")
+            try "b".write(to: fileB, atomically: true, encoding: .utf8)
+
+            // temp/subdir/subsubdir
+            let subsubdir = subdir.appendingPathComponent("subsubdir")
+            try FileManager.default.createDirectory(at: subsubdir, withIntermediateDirectories: false)
+
+            // temp/subdir/subdir/c.txt
+            let fileC = subsubdir.appendingPathComponent("c.txt")
+            try "c".write(to: fileC, atomically: true, encoding: .utf8)
+
+            let zipFile = tempURL.appendingPathComponent("out.zip")
+            try Zipper.zip(paths: [fileA, subdir], to: zipFile, method: .zipTool(workingDirectory: tempDir))
+            XCTAssert(FileManager.default.fileExists(atPath: zipFile.path))
+
+            do { // unzip what we zipped and check results
+                let roundtrip = tempURL.appendingPathComponent("roundtrip")
+                try Zipper.unzip(from: zipFile, to: roundtrip)
+                XCTAssert(FileManager.default.fileExists(atPath: roundtrip.path))
+                // roundtrip/a.txt
+                // roundtrip/subdir/b.txt
+                let fileA = roundtrip.appendingPathComponent("a.txt")
+                let fileB = roundtrip.appendingPathComponent("subdir").appendingPathComponent("b.txt")
+                let fileC = roundtrip.appendingPathComponent("subdir").appendingPathComponent("subsubdir").appendingPathComponent("c.txt")
                 XCTAssert(FileManager.default.fileExists(atPath: fileA.path))
                 XCTAssert(FileManager.default.fileExists(atPath: fileB.path))
+                XCTAssert(FileManager.default.fileExists(atPath: fileC.path))
                 XCTAssertEqual(try String(contentsOf: fileA), "a")
                 XCTAssertEqual(try String(contentsOf: fileB), "b")
+                XCTAssertEqual(try String(contentsOf: fileC), "c")
             }
         }
     }