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

Destinations: use UniversalArchiver in install command #6392

Merged
merged 6 commits into from
Apr 11, 2023

Conversation

MaxDesiatov
Copy link
Contributor

@MaxDesiatov MaxDesiatov commented Apr 5, 2023

Fixes .tar.gz archives support in experimental-destination install subcommand.

Tweaks multi-component extensions calculation so that at most last 2 components are taken into consideration. This fixes an issue with bundle names that have dots. For example, for 5.7.3-RELEASE_ubuntu_22.04_aarch64.tar.gz previously allExtensions returned 7.3-RELEASE_ubuntu_22.04_aarch64.tar.gz, and that didn't match any of the archivers, even though the actual extension was valid.

rdar://107485048

@MaxDesiatov MaxDesiatov requested a review from abertelrud as a code owner April 5, 2023 15:48
@MaxDesiatov MaxDesiatov self-assigned this Apr 5, 2023
@MaxDesiatov
Copy link
Contributor Author

@swift-ci smoke test

1 similar comment
@MaxDesiatov
Copy link
Contributor Author

@swift-ci smoke test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

2 similar comments
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@@ -41,7 +41,7 @@ extension AbsolutePath {

/// Unlike ``AbsolutePath//extension``, this property returns all characters after the first `.` character in a
/// `````. If no dot character is present in the filename or first dot is the last character, `nil` is returned.
var allExtensions: String? {
var allExtensions: [Substring]? {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think String is more appropriate here. we can cast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that holding on to Substring is fine since the original string is present as a property of AbsolutePath anyway, but I don't mind changing to String, which was done in the latest commit

@@ -47,7 +47,7 @@ public struct InstallDestination: DestinationCommand {
bundlePathOrURL: bundlePathOrURL,
destinationsDirectory: destinationsDirectory,
self.fileSystem,
ZipArchiver(fileSystem: self.fileSystem),
UniversalArchiver(self.fileSystem, Cancellator(observabilityScope: observabilityScope)),
Copy link
Contributor

Choose a reason for hiding this comment

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

ideally you pass an existing cancelator, eg from swiftTool

Copy link
Contributor Author

@MaxDesiatov MaxDesiatov Apr 5, 2023

Choose a reason for hiding this comment

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

We don't use SwiftTool in experimental-destination as that pulls in a lot of dependencies that aren't needed in these subcommands, so I hope an ad-hoc executor is fine here?

Copy link
Contributor

Choose a reason for hiding this comment

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

how do you handle cancelling? swift tool has a signal handling routine that then calls the cancellator. ie what happens when users ctrl+c during an install / download sequence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, I thought Cancellator supported it out of the box. Will have a closer look

Copy link
Contributor Author

@MaxDesiatov MaxDesiatov Apr 11, 2023

Choose a reason for hiding this comment

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

Addressed in #6396 and e33a167

@MaxDesiatov MaxDesiatov requested a review from tomerd April 5, 2023 18:38
@MaxDesiatov
Copy link
Contributor Author

@swift-ci smoke test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

2 similar comments
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor Author

@swift-ci smoke test macos

@MaxDesiatov MaxDesiatov force-pushed the maxd/use-universal-archiver branch from 1ae861c to e33a167 Compare April 11, 2023 11:31
@MaxDesiatov
Copy link
Contributor Author

@swift-ci smoke test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

1 similar comment
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor Author

@swift-ci smoke test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

1 similar comment
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor Author

@swift-ci smoke test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

1 similar comment
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor Author

@swift-ci smoke test macos

@MaxDesiatov MaxDesiatov enabled auto-merge (squash) April 11, 2023 20:59
@MaxDesiatov MaxDesiatov merged commit ee386f3 into main Apr 11, 2023
@MaxDesiatov MaxDesiatov deleted the maxd/use-universal-archiver branch April 13, 2023 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants