Basics: support multiple formats with a single archiver #6369

MaxDesiatov · 2023-04-03T13:36:27Z

Motivation:

In certain function signatures only a single archiver instance can be taken as an argument. We need to support multiple archive formats, such as .zip and .tar.gz, for example in cross-compilation destination bundles. Instead of updating those functions to take multiple archivers, creating a single type that can support multiple formats would be a better solution.

Modifications:

Added MultiFormatArchiver that can handle multiple formats by delegating to other archivers it was initialized with. Since most uses of Archiver are either generic or take an existential, this allows us to pass MultiFormatArchiver to functions that previously supported only a single archive format.

rdar://107485048

Results

This is technically NFC as MultiFormatArchiver is not used anywhere yet.

`swift experimental-destination install` subcommand fails when pointed to a destination bundle compressed with `.tar.gz` extension, compressed with tar and gzip. This archival format is vastly superior to zip in the context of cross-compilation destination bundles. In our typical scenario we see that `.tar.gz` archives are at least 4x better than `.zip`.

compnerd · 2023-04-03T16:45:47Z

Sources/Basics/MultiFormatArchiver.swift

+
+        var description: String {
+            switch self {
+            case .unknownFormat(let ext, let path):


Why not consider using magic? https://github.com/file/file/tree/ec41083645689a787cdd00cb3b5bf578aa79e46c should be possible to use on Windows even.

Can we assume it's installed on Windows? It doesn't seem to be installed on swift Docker images for Ubuntu, so adding a dependency on it will require updating Dockerfiles in the swift-docker repository. This isn't supposed to be an incredibly resilient algorithm, but something that we can still land in 5.9 I hope. So distinguishing by extension should be good enough for now, and we can consider adding support for magic format detection later.

tomerd · 2023-04-03T18:31:28Z

Sources/Basics/Archiver+Zip.swift

@@ -14,7 +14,7 @@ import TSCBasic
 import Dispatch

 /// An `Archiver` that handles ZIP archives using the command-line `zip` and `unzip` tools.
-public struct ZipArchiver: Archiver, Cancellable {
+public final class ZipArchiver: Archiver, Cancellable {


ideally this remains a struct

Addressed in the base TarArchiver PR, both FileSystem and Cancellator are reference types, and Cancellator in particular holds mutable state, which transitively makes ZipArchiver stateful.

tomerd · 2023-04-03T18:33:09Z

Sources/Basics/MultiFormatArchiver.swift

+
+    private let formatMapping: [String: any Archiver]
+
+    public init(_ archivers: [any Archiver]) {


not convinced about this API. the client should not care to initialize the "multiplexing" archiver with concrete archivers. Instead, an API that takes the desired "extensions" or "archiveTypes" (enum) and then creates the concrete instances seems a more intent based API

If the archivers used by this new archiver are an implementation detail, wouldn't an enum for archive types be a premature generalization? I've changed it to take only any FileSystem and Cancellator to initialize instances of ZipArchiver and TarArchiver, like you suggested. Would that be ok for this initial implementation?

tomerd · 2023-04-03T18:33:38Z

Sources/Basics/MultiFormatArchiver.swift

+            throw Error.noFileNameExtension(archivePath)
+        }
+
+        var extensions = String(filename[firstDot ..< filename.endIndex])


dont we have an API to extract the extension from a path?

As mentioned in a comment above on the line 53, that AbsolutePath API doesn't support stacked extensions, it returns just gz for .tar.gz archives.

I'll expand the comment to mention the behavior explicitly.

We should probably add an API that can return multiple extensions?

exactly ^^ we can add that to basics

That makes sense, added in the latest commit

Sources/Basics/MultiFormatArchiver.swift

tomerd · 2023-04-04T16:24:57Z

Sources/Basics/Archiver/UniversalArchiver.swift

+
+/// An `Archiver` that handles multiple formats by delegating to other existing archivers each dedicated to its own
+/// format.
+public final class UniversalArchiver: Archiver {


this should be a value type

tomerd · 2023-04-04T16:25:28Z

Sources/Basics/Archiver/UniversalArchiver.swift

+        self.supportedExtensions = supportedExtensions
+    }
+
+    private func archiver(for archivePath: AbsolutePath) throws -> any Archiver {


tomerd · 2023-04-04T16:26:10Z

Sources/Basics/Archiver/ZipArchiver.swift

@@ -14,7 +14,7 @@ import TSCBasic
 import Dispatch

 /// An `Archiver` that handles ZIP archives using the command-line `zip` and `unzip` tools.
-public struct ZipArchiver: Archiver, Cancellable {
+public final class ZipArchiver: Archiver, Cancellable {


I dont think this change is necessary

Added `MultiFormatArchiver` that can handle multiple formats by delegating to other archivers it was initialized with. Since most uses of `Archiver` are either generic or take an existential, this allows us to pass `MultiFormatArchiver` that previously supported only a single archive format. rdar://107485048 # Conflicts: # Sources/Basics/MultiFormatArchiver.swift

…xd/multi-format-archiver # Conflicts: # Sources/Basics/CMakeLists.txt # Tests/BasicsTests/Archiver/Inputs/archive.tar.gz # Tests/BasicsTests/Archiver/Inputs/invalid_archive.tar.gz

MaxDesiatov · 2023-04-04T20:57:04Z

@swift-ci smoke test

Sources/Basics/Archiver/UniversalArchiver.swift

MaxDesiatov · 2023-04-04T21:38:34Z

@swift-ci smoke test

MaxDesiatov · 2023-04-04T21:38:50Z

@swift-ci test windows

MaxDesiatov · 2023-04-04T22:01:03Z

@swift-ci smoke test macos

MaxDesiatov · 2023-04-05T06:51:14Z

@swift-ci test windows

MaxDesiatov · 2023-04-05T06:53:16Z

@swift-ci smoke test macos

In certain function signatures only a single archiver instance can be taken as an argument. We need to support multiple archive formats, such as `.zip` and `.tar.gz`, for example in cross-compilation destination bundles. Instead of updating those functions to take multiple archivers, creating a single type that can support multiple formats would be a better solution. Added `MultiFormatArchiver` that can handle multiple formats by delegating to other archivers it was initialized with. Since most uses of `Archiver` are either generic or take an existential, this allows us to pass `MultiFormatArchiver` to functions that previously supported only a single archive format. This is technically NFC as `MultiFormatArchiver` is not used anywhere yet. rdar://107485048

MaxDesiatov added 4 commits April 2, 2023 21:40

TarArchiver is a reference type

2d5f542

Fix Linux test failure

3291425

Remove unused import

c6809a3

MaxDesiatov requested review from abertelrud and neonichu as code owners April 3, 2023 13:36

MaxDesiatov self-assigned this Apr 3, 2023

MaxDesiatov requested a review from tomerd as a code owner April 3, 2023 13:36

neonichu approved these changes Apr 3, 2023

View reviewed changes

compnerd reviewed Apr 3, 2023

View reviewed changes

tomerd reviewed Apr 3, 2023

View reviewed changes

MaxDesiatov commented Apr 3, 2023

View reviewed changes

Sources/Basics/MultiFormatArchiver.swift Outdated Show resolved Hide resolved

MaxDesiatov added 2 commits April 4, 2023 12:39

Clean up cancellation errors

cca0f60

Update tar archiver process arguments

2efc0b9

MaxDesiatov force-pushed the maxd/multi-format-archiver branch from 0592ba4 to cc88808 Compare April 4, 2023 15:18

MaxDesiatov requested a review from tomerd April 4, 2023 15:29

tomerd reviewed Apr 4, 2023

View reviewed changes

Address PR feedback

c00497e

MaxDesiatov force-pushed the maxd/multi-format-archiver branch from 844994c to d34bb91 Compare April 4, 2023 17:04

MaxDesiatov added 6 commits April 4, 2023 18:04

Formatting fix

2cb673b

Apply suggestions from code review

4039366

Address PR feedback

eeceee4

Add doc comments

a7ffeda

Address PR feeback

00fb477

MaxDesiatov force-pushed the maxd/multi-format-archiver branch from d34bb91 to 00fb477 Compare April 4, 2023 17:05

tomerd approved these changes Apr 4, 2023

View reviewed changes

Merge branch 'main' of github.com:apple/swift-package-manager into ma…

bda5a5b

…xd/multi-format-archiver # Conflicts: # Sources/Basics/CMakeLists.txt # Tests/BasicsTests/Archiver/Inputs/archive.tar.gz # Tests/BasicsTests/Archiver/Inputs/invalid_archive.tar.gz

MaxDesiatov changed the base branch from maxd/tar-archives to main April 4, 2023 20:55

Resolve conflict

8c9f707

MaxDesiatov enabled auto-merge (squash) April 4, 2023 20:58

MaxDesiatov commented Apr 4, 2023

View reviewed changes

Sources/Basics/Archiver/UniversalArchiver.swift Outdated Show resolved Hide resolved

Update Sources/Basics/Archiver/UniversalArchiver.swift

9aced75

MaxDesiatov merged commit e21df87 into main Apr 5, 2023

MaxDesiatov deleted the maxd/multi-format-archiver branch April 5, 2023 16:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basics: support multiple formats with a single archiver #6369

Basics: support multiple formats with a single archiver #6369

MaxDesiatov commented Apr 3, 2023 •

edited

Loading

compnerd Apr 3, 2023

MaxDesiatov Apr 3, 2023

tomerd Apr 3, 2023

MaxDesiatov Apr 4, 2023

tomerd Apr 3, 2023

MaxDesiatov Apr 4, 2023

tomerd Apr 3, 2023

MaxDesiatov Apr 3, 2023

MaxDesiatov Apr 3, 2023

neonichu Apr 3, 2023

tomerd Apr 4, 2023 •

edited

Loading

MaxDesiatov Apr 4, 2023

tomerd Apr 4, 2023

tomerd Apr 4, 2023

tomerd Apr 4, 2023

MaxDesiatov commented Apr 4, 2023

MaxDesiatov commented Apr 4, 2023

MaxDesiatov commented Apr 4, 2023

MaxDesiatov commented Apr 4, 2023

MaxDesiatov commented Apr 5, 2023

MaxDesiatov commented Apr 5, 2023


		private let formatMapping: [String: any Archiver]

		public init(_ archivers: [any Archiver]) {

Basics: support multiple formats with a single archiver #6369

Basics: support multiple formats with a single archiver #6369

Conversation

MaxDesiatov commented Apr 3, 2023 • edited Loading

Motivation:

Modifications:

Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomerd Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxDesiatov commented Apr 4, 2023

MaxDesiatov commented Apr 4, 2023

MaxDesiatov commented Apr 4, 2023

MaxDesiatov commented Apr 4, 2023

MaxDesiatov commented Apr 5, 2023

MaxDesiatov commented Apr 5, 2023

MaxDesiatov commented Apr 3, 2023 •

edited

Loading

tomerd Apr 4, 2023 •

edited

Loading