-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Basics: add support for .tar.gz
archives
#6368
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
`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`.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
@swift-ci smoke test |
This comment was marked as duplicate.
This comment was marked as duplicate.
1 similar comment
This comment was marked as duplicate.
This comment was marked as duplicate.
@swift-ci smoke test linux |
This comment was marked as duplicate.
This comment was marked as duplicate.
5b27501
to
c6809a3
Compare
@swift-ci test windows |
Sources/Basics/Archiver+Tar.swift
Outdated
import class TSCBasic.Process | ||
|
||
/// An `Archiver` that handles Tar archives using the command-line `tar` tool. | ||
public final class TarArchiver: Archiver { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think this should be a reference type. It holds no state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It holds references to Cancellator
and FileSystem
, both of which are reference types. Cancellator
in particular holds mutable state with a cancellation handlers registry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, but that does not mean it also needs to be a reference type
Sources/Basics/Archiver+Tar.swift
Outdated
) | ||
|
||
guard let registrationKey = self.cancellator.register(process) else { | ||
throw StringError("cancellation") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nicer error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This duplicates the error message from ZipArchiver
, would it be ok to update the error message there as well in the same PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a new static func
on CancellationError
that populates its description
with a more detailed message, now used across both TarArchiver
and ZipArchiver
.
Sources/Basics/Archiver+Tar.swift
Outdated
} | ||
|
||
let process = TSCBasic.Process( | ||
arguments: [self.tarCommand, "xzf", archivePath.pathString, "-C", destinationPath.pathString] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true on all platforms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that should work on all platforms (okay, well, at least Linux, macOS, and Windows). I personally prefer zxf
because logically its filters first before extracting, but that doesn't matter.
let process = TSCBasic.Process( | ||
arguments: [self.tarCommand, "acf", destinationPath.pathString, directory.basename], | ||
workingDirectory: directory.parentDirectory | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true on all platforms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, as clarified above by Saleem. I also checked macOS manpages that these arguments have the same meaning. AFAIU tar.exe
on Windows follows the convention.
@swift-ci smoke test |
@swift-ci test windows |
self.description = description | ||
} | ||
|
||
static func failedToRegisterProcess(_ process: TSCBasic.Process) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
@swift-ci smoke test |
@swift-ci smoke test |
@swift-ci test windows |
1 similar comment
@swift-ci test windows |
@swift-ci test windows |
`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 a typical scenario we see that `.tar.gz` archives are at least 4x better than `.zip`, since with `.zip` files are compressed individually, while with `.tar.gz` compression is applied to the whole archive at once. On macOS `tar` is always available, and we can also assume it's also available on Linux Docker images, since `tar` is used to unpack `.tar.gz` distributions of Swift itself. Since `ZipArchiver` uses `.tar.exe` on Windows, we make the same assumption that command is available, as we did for `ZipArchiver`. Added new `TarArchiver` class, `TarArchiverTests`, and corresponding test files. This is technically NFC, since `TarArchiver` is not called from anywhere yet.
Motivation:
swift experimental-destination install
subcommand fails when pointed to a destination bundle compressed with.tar.gz
extension, compressed withtar
andgzip
. This archival format is vastly superior tozip
in the context of cross-compilation destination bundles. In a typical scenario we see that.tar.gz
archives are at least 4x better than.zip
, since with.zip
files are compressed individually, while with.tar.gz
compression is applied to the whole archive at once.On macOS
tar
is always available, and we can also assume it's also available on Linux Docker images, sincetar
is used to unpack.tar.gz
distributions of Swift itself. SinceZipArchiver
uses.tar.exe
on Windows, we make the same assumption that command is available, as we did forZipArchiver
.Modifications:
Added new
TarArchiver
class,TarArchiverTests
, and corresponding test files.Result:
This is technically NFC, since
TarArchiver
is not called from anywhere yet.