Skip to content
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

Basics: add support for .tar.gz archives #6368

Merged
merged 8 commits into from Apr 4, 2023
Merged

Conversation

MaxDesiatov
Copy link
Member

@MaxDesiatov MaxDesiatov commented Apr 2, 2023

Motivation:

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.

Modifications:

Added new TarArchiver class, TarArchiverTests, and corresponding test files.

Result:

This is technically NFC, since TarArchiver is not called from 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`.
@MaxDesiatov

This comment was marked as duplicate.

@MaxDesiatov

This comment was marked as duplicate.

@MaxDesiatov

This comment was marked as duplicate.

@MaxDesiatov
Copy link
Member Author

@swift-ci smoke test

@MaxDesiatov

This comment was marked as duplicate.

1 similar comment
@MaxDesiatov

This comment was marked as duplicate.

@MaxDesiatov
Copy link
Member Author

@swift-ci smoke test linux

@MaxDesiatov

This comment was marked as duplicate.

2 similar comments
@MaxDesiatov

This comment was marked as duplicate.

@MaxDesiatov
Copy link
Member Author

@swift-ci test windows

@MaxDesiatov
Copy link
Member Author

@swift-ci test windows

import class TSCBasic.Process

/// An `Archiver` that handles Tar archives using the command-line `tar` tool.
public final class TarArchiver: Archiver {
Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

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

)

guard let registrationKey = self.cancellator.register(process) else {
throw StringError("cancellation")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nicer error message?

Copy link
Member Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please

Copy link
Member Author

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.

}

let process = TSCBasic.Process(
arguments: [self.tarCommand, "xzf", archivePath.pathString, "-C", destinationPath.pathString]
Copy link
Member

@tomerd tomerd Apr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true on all platforms?

Copy link
Collaborator

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
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true on all platforms?

Copy link
Member Author

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.

@MaxDesiatov
Copy link
Member Author

@swift-ci smoke test

@MaxDesiatov
Copy link
Member Author

@swift-ci test windows

self.description = description
}

static func failedToRegisterProcess(_ process: TSCBasic.Process) -> Self {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@MaxDesiatov
Copy link
Member Author

@swift-ci smoke test

@MaxDesiatov
Copy link
Member Author

@swift-ci smoke test

@MaxDesiatov
Copy link
Member Author

@swift-ci test windows

1 similar comment
@MaxDesiatov
Copy link
Member Author

@swift-ci test windows

@MaxDesiatov
Copy link
Member Author

@swift-ci test windows

@MaxDesiatov MaxDesiatov enabled auto-merge (squash) April 4, 2023 18:02
@MaxDesiatov MaxDesiatov merged commit addc8d2 into main Apr 4, 2023
4 checks passed
MaxDesiatov added a commit that referenced this pull request Apr 5, 2023
`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.
@MaxDesiatov MaxDesiatov deleted the maxd/tar-archives branch April 5, 2023 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants