Merge pull request #30 from SwiftPackageIndex/use-zipfoundation-for-unzip

finestructure · web-flow · commit 4887d1026695 · 2024-07-11T11:43:57.000+02:00
Use zipfoundation for unzip
diff --git a/Package.resolved b/Package.resolved
@@ -251,6 +251,15 @@
         "revision" : "67fa55813b9e7b3b9acee9c0ae501def28746d76",
         "version" : "2.1.2"
       }
+    },
+    {
+      "identity" : "zipfoundation",
+      "kind" : "remoteSourceControl",
+      "location" : "https://github.com/weichsel/ZIPFoundation.git",
+      "state" : {
+        "revision" : "02b6abe5f6eef7e3cbd5f247c5cc24e246efcfe0",
+        "version" : "0.9.19"
+      }
     }
   ],
   "version" : 2
diff --git a/Package.swift b/Package.swift
@@ -41,6 +41,7 @@ let package = Package(
         .package(url: "https://github.com/swift-server/swift-aws-lambda-runtime.git", from: "1.0.0-alpha.1"),
         .package(url: "https://github.com/soto-project/soto-s3-file-transfer.git", from: "1.2.0"),
         .package(url: "https://github.com/marmelroy/Zip.git", from: "2.1.2"),
+        .package(url: "https://github.com/weichsel/ZIPFoundation.git", from: "0.9.19"),
         .package(url: "https://github.com/pointfreeco/swift-dependencies.git", from: "1.0.0")
     ],
     targets: [
@@ -57,6 +58,7 @@ let package = Package(
         ),
         .target(name: "DocUploadBundle", dependencies: [
             .product(name: "Zip", package: "Zip"),
+            .product(name: "ZIPFoundation", package: "zipfoundation"),
             .product(name: "Dependencies", package: "swift-dependencies")
         ]),
         .testTarget(name: "DocUploadBundleTests", dependencies: ["DocUploadBundle"], exclude: ["Fixtures"]),
diff --git a/README.md b/README.md
@@ -60,7 +60,7 @@ Instead, use the `dev` environment to validate a new release as follows:
     ```
   Use the doc bundle included in this repo or download a `dev-` doc bundle from `spi-docs-inbox` on S3.
 
-- Check the [`DocUploaderLambda-Test` CloudWatch log group](https://us-east-2.console.aws.amazon.com/cloudwatch/home?region=us-east-2#logsV2:log-groups/log-group/$252Faws$252Flambda$252FDocUploaderLambda-Test-UploadFunction-3D3w0QTh1l6H) to confirm the new version has been triggered and processed the file without errors.
+- Check the [`DocUploaderLambda-Test` CloudWatch log group](https://us-east-2.console.aws.amazon.com/cloudwatch/home?region=us-east-2#logsV2:log-groups/log-group/$252Faws$252Flambda$252FDocUploaderLambda-Test-UploadFunction-A0zLSxC2sCV5) to confirm the new version has been triggered and processed the file without errors.
 
     ```bash
     open 'https://us-east-2.console.aws.amazon.com/cloudwatch/home?region=us-east-2#logsV2:log-groups/log-group/$252Faws$252Flambda$252FDocUploaderLambda-Test-UploadFunction-3D3w0QTh1l6H'
diff --git a/Sources/DocUploadBundle/DocUploadBundle.swift b/Sources/DocUploadBundle/DocUploadBundle.swift
@@ -105,12 +105,12 @@ public struct DocUploadBundle {
         )
     }
 
-    public func zip(to workDir: String, method: Zipper.Method = .library) throws -> String {
+    public func zip(to workDir: String) 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, method: method)
+        try Zipper.zip(paths: [metadataURL, URL(fileURLWithPath: sourcePath)], to: archiveURL)
 
         return archiveURL.path
     }
diff --git a/Sources/DocUploadBundle/Zipper.swift b/Sources/DocUploadBundle/Zipper.swift
@@ -15,76 +15,19 @@
 import Foundation
 
 import Zip
+import ZIPFoundation
 
 
-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 let error as ZipError {
-                    switch error {
-                        case .fileNotFound: throw Error.fileNotFound
-                        case .unzipFail: throw Error.unzipFail
-                        case .zipFail: throw Error.zipFail
-                    }
-                }
-                catch {
-                    throw Error.generic(reason: "\(error)")
-                }
-
-            case .zipTool:
-                do {
-                    try withTempDir { tempDir in
-                        let tempURL = URL(fileURLWithPath: tempDir)
-                        // Copy inputs to tempDir
-                        for source in inputPaths {
-                            let target = tempURL.appendingPathComponent(source.lastPathComponent)
-                            try FileManager.default.copyItem(at: source, to: target)
-                        }
-
-                        // Run zip
-                        let process = Process()
-                        process.executableURL = zip
-                        process.arguments = ["-q", "-r", outputPath.path] + inputPaths.map(\.lastPathComponent)
-                        process.currentDirectoryURL = tempURL
-                        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 let error as ZipError {
-            switch error {
-                case .fileNotFound: throw Error.fileNotFound
-                case .unzipFail: throw Error.unzipFail
-                case .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
+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 Error: Swift.Error {
-        case generic(reason: String)
-        case fileNotFound
-        case unzipFail
-        case zipFail
+    static func unzip(from inputPath: URL, to outputPath: URL, fileOutputHandler: ((_ unzippedFile: URL) -> Void)? = nil) throws {
+        // Use ZipFoundation to unzip because of an archive that can't be round-tripped with marmelroy/Zip
+        // https://github.com/SwiftPackageIndex/SwiftPackageIndex-Server/issues/3137
+        try FileManager.default.createDirectory(at: outputPath, withIntermediateDirectories: true)
+        try FileManager.default.unzipItem(at: inputPath, to: outputPath)
     }
 }
 
diff --git a/Sources/DocUploader/DocUploader.swift b/Sources/DocUploader/DocUploader.swift
@@ -83,7 +83,10 @@ public struct DocUploader: LambdaHandler {
         logger.info("record: \(record)")
 
         try await run {
-            let outputPath = "/tmp"
+            // When Lambdas are run in quick succession they seem to be seeing the same /tmp file system and unzipping can fail with
+            //   lambda handler returned an error: Error Domain=NSCocoaErrorDomain Code=516 "A file with the same name already exists."
+            let outputPath = "/tmp/\(UUID())"
+            try Foundation.FileManager.default.createDirectory(at: URL(fileURLWithPath: outputPath), withIntermediateDirectories: true)
 
             do {
                 logger.info("Copying \(s3Key.url) to \(outputPath)...")
diff --git a/Tests/DocUploadBundleTests/DocUploadBundleTests.swift b/Tests/DocUploadBundleTests/DocUploadBundleTests.swift
@@ -100,4 +100,34 @@ 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))
+        }
+    }
+
 }
diff --git a/Tests/DocUploadBundleTests/Fixtures/prod-apple-swift-metrics-main-e6a00d36.zip b/Tests/DocUploadBundleTests/Fixtures/prod-apple-swift-metrics-main-e6a00d36.zip
diff --git a/Tests/DocUploadBundleTests/TempDir.swift b/Tests/DocUploadBundleTests/TempDir.swift
@@ -32,8 +32,3 @@ func withTempDir<T>(body: (String) async throws -> T) async throws -> T {
     let tmp = try TempDir()
     return try await body(tmp.path)
 }
-
-func withTempDir<T>(body: (String) throws -> T) throws -> T {
-    let tmp = try TempDir()
-    return try body(tmp.path)
-}
diff --git a/Tests/DocUploadBundleTests/ZipTests.swift b/Tests/DocUploadBundleTests/ZipTests.swift
@@ -21,7 +21,7 @@ final class ZipTests: XCTestCase {
     
     func test_unzip() async throws {
         // Test basic unzip behaviour we expect from the library we use
-        try withTempDir { tempDir in
+        try await withTempDir { tempDir in
             let tempURL = URL(fileURLWithPath: tempDir)
             let zipFile = fixtureUrl(for: "out.zip")
             let outDir = tempURL.appendingPathComponent("out")
@@ -41,7 +41,7 @@ final class ZipTests: XCTestCase {
 
     func test_zip_roundtrip() async throws {
         // Test basic zip roundtrip
-        try withTempDir { tempDir in
+        try await withTempDir { tempDir in
             //  temp
             let tempURL = URL(fileURLWithPath: tempDir)
 
@@ -57,14 +57,6 @@ 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))
@@ -77,101 +69,10 @@ 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 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)
-            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")
-            }
-        }
-    }
-
-    func test_zip_roundtrip_shellTool_relative_paths() async throws {
-        try XCTSkipIf(!FileManager.default.fileExists(atPath: Zipper.zip.path))
-
-        // Test basic zip roundtrip with the shellTool method and relative paths
-        try withTempDir { tempDir in
-            // DocBundle components
-            // metadataURL: tempDir/metadata.json
-            // sourceURL:   tempDir/.docs/owner/repo/ref
-            // should be zipped as
-            //   - metadata.json
-            //   - ref
-            // at the top level as relative paths.
-            let tempURL = URL(fileURLWithPath: tempDir)
-            let metadataURL = tempURL.appendingPathComponent("metadata.json")
-            try "metadata".write(to: metadataURL, atomically: true, encoding: .utf8)
-            let sourceURL = tempURL.appendingPathComponent("docs/owner/repo/ref")
-            try FileManager.default.createDirectory(at: sourceURL, withIntermediateDirectories: true)
-            let indexHTML = sourceURL.appendingPathComponent("index.html")
-            try "index".write(to: indexHTML, atomically: true, encoding: .utf8)
-
-            // MUT
-            let zipFile = tempURL.appendingPathComponent("out.zip")
-            try Zipper.zip(paths: [metadataURL, sourceURL], to: zipFile, method: .zipTool)
-
-            do {  // validate
-                let unzipDir = tempURL.appendingPathComponent("unzip")
-                try Zipper.unzip(from: zipFile, to: unzipDir)
-                let metadataURL = unzipDir.appendingPathComponent("metadata.json")
-                let indexHTML = unzipDir.appendingPathComponent("ref/index.html")
-                XCTAssert(FileManager.default.fileExists(atPath: metadataURL.path))
-                XCTAssert(FileManager.default.fileExists(atPath: indexHTML.path))
-                XCTAssertEqual(try String(contentsOf: metadataURL), "metadata")
-                XCTAssertEqual(try String(contentsOf: indexHTML), "index")
             }
         }
     }

Original file line number	Diff line number	Diff line change
`@@ -105,12 +105,12 @@ public struct DocUploadBundle {`
`105`	`105`	`)`
`106`	`106`	`}`
`107`	`107`
`108`		`- public func zip(to workDir: String, method: Zipper.Method = .library) throws -> String {`
	`108`	`+ public func zip(to workDir: String) throws -> String {`
`109`	`109`	`let archiveURL = URL(fileURLWithPath: "\(workDir)/\(archiveName)")`
`110`	`110`	`let metadataURL = URL(fileURLWithPath: "\(workDir)/metadata.json")`
`111`	`111`	`try JSONEncoder().encode(metadata).write(to: metadataURL)`
`112`	`112`
`113`		`- try Zipper.zip(paths: [metadataURL, URL(fileURLWithPath: sourcePath)], to: archiveURL, method: method)`
	`113`	`+ try Zipper.zip(paths: [metadataURL, URL(fileURLWithPath: sourcePath)], to: archiveURL)`
`114`	`114`
`115`	`115`	`return archiveURL.path`
`116`	`116`	`}`
Original file line number	Diff line number	Diff line change
`@@ -32,8 +32,3 @@ func withTempDir<T>(body: (String) async throws -> T) async throws -> T {`
`32`	`32`	`let tmp = try TempDir()`
`33`	`33`	`return try await body(tmp.path)`
`34`	`34`	`}`
`35`		`-`
`36`		`-func withTempDir<T>(body: (String) throws -> T) throws -> T {`
`37`		`- let tmp = try TempDir()`
`38`		`- return try body(tmp.path)`
`39`		`-}`