Add Interactions between On-Disk Files and In-Memory Archives

[WowbaggersLiquidLunch]

Fixes #173

Changes proposed in this PR

• Add a method for unzipping an archive that's not necessarily on disk to a location on disk.

let inMemoryArchive = Archive(someData, .read)
let destinationOnDisk = URL(fileURLWithPath: "path/to/destination")
FileManager.default.unzip(inMemoryArchive, to destinationOnDisk)

• Add a method for zipping a file on disk to an archive that's not necessarily on disk.

let inMemoryArchive = Archive(someData, .create)
let sourceOnDisk = URL(fileURLWithPath: "path/to/destination")
FileManager.default.zipItem(at sourceOnDisk, to inMemoryArchive)

• Add a method for zipping a file on disk and returning an in-memory archive containing the zipped file.

let sourceOnDisk = URL(fileURLWithPath: "path/to/destination")
let archive: Archive = FileManager.default.itemZipped(from sourceOnDisk)

Tests performed

• Added unit tests for the 3 new methods.
• Tests passed on macOS 10.15.5 with Xcode 11.5 (11E608c).

Further info for the reviewer

• Refactored existing file-to-file zipping and unzipping methods. They now delegate the majority of work to the first 2 new methods.
• Fixed a bug where unzipItem(at:to:skipCRC32:progress:preferredEncoding:) didn't check if destinationURL was writable.

Open Issues

For some reason, sourceURL has to be checked twice in zipItem(at:to:shouldKeepParent:compressionMethod:progress:). This is marked with a FIXME in FileManager+ZIP.swift.

[weichsel]

Thank you for your contribution and sorry for the delayed reply.

The changes look good, but there are some minor issues we need to fix before merging this.
The Linux build currently fails and there are also some swiftlint warnings.

I saw, that you added a FIXME comment regarding a test case that fails if no fileExists check is performed. This is expected behavior. We try to also cover the error code paths with tests and this test case exercises the condition where the source archive does not exist and we therefore expect an error to be thrown.

[WowbaggersLiquidLunch]

The Linux build currently fails

I found out the root problem: a newly initialised archive wasn't unwrapped in itemZipped(from:shouldKeepParent:compressionMethod:progress), so I changed it.

This problem wasn't Linux specific, but it hasn't been caught on Apple platforms in testing. I'm not completely sure why this is the case, yet. It seems like all test cases that are conditionally compiled at Swift ≥ 5.0 are skipped by Xcode in testing.

I fixed as much as I could, and don't know how to deal with the remaining errors. Somehow XCTFail allows guard to fall through when the entire guard statement is conditionally compiled.

some swiftlint warnings

I think I fixed most of the warnings. The rest are complaints about either functions or files having too many lines. There aren't many empty lines in the entire codebase. I think one way to fix it is through some heavy refactoring that gathers together some common parts, and make better use of setUp() and teadDown().

I find these linter rules quite arbitrary, and even a bit silly, though. They could inadvertently force developers to make code less readable, just to conform to the forms.

I saw, that you added a FIXME comment regarding a test case that fails if no fileExists check is performed. This is expected behavior. We try to also cover the error code paths with tests and this test case exercises the condition where the source archive does not exist and we therefore expect an error to be thrown.

This occurs in zipItem(at sourceURL: URL, to destinationURL: URL, shouldKeepParent: Bool = true, compressionMethod: CompressionMethod = .none, progress: Progress? = nil), which delegates everything to try zipItem(at: sourceURL, to: archive, shouldKeepParent: shouldKeepParent, compressionMethod: compressionMethod, progress: progress). The latter already checks if the source exists for the former. So, it seems strange to me that it needs to be checked again.

[WowbaggersLiquidLunch]

While trying to figure out why some of the tests failed, I modified the CI script. Mostly expanded and updated the test platform versions.

[weichsel]

Thanks for looking into that so quickly!

This problem wasn't Linux specific, but it hasn't been caught on Apple platforms in testing. I'm not completely sure why this is the case, yet. It seems like all test cases that are conditionally compiled at Swift ≥ 5.0 are skipped by Xcode in testing.

This is a shortcoming of the current test setup.
We mainly provide an .xcodeproj for Cathage support. SPM and CocoaPods don't use the .xcodeproj at all.
Sadly, the Xcode project file doesn't provide a mechanism to define a range of supported Swift versions. You can only set SWIFT_VERSION to a single value. As Swift 4.0 currently is the "baseline" language version we support, we have to set it to that. We currently use the Xcode-based test runners for Darwin platforms and they use the xcodeproj and therefore pick up the language version from there. Swift 5.0 codepaths are currently only exercised by the SPM based Linux test runs.

While trying to figure out why some of the tests failed, I modified the CI script. Mostly expanded and updated the test platform versions.

I plan to move to GitHub Actions for CI in the near future. While I think the changes you introduced (using the latest version & setting up a test matrix, ...) are a good idea, I'd rather do that in a dedicated PR, that also migrates to GH Actions.
Can you please revert the following changes for now:

• .xctestplan
• .xcscheme
• .azure-pipelines.yml

I find these linter rules quite arbitrary, and even a bit silly, though. They could inadvertently force developers to make code less readable, just to conform to the forms.

I totally understand that - I often run into swiftlint-imposed constraints myself. Of course that shouldn't lead to less readable code. Ideally it should point out locations where improvements can be made.
As a concrete example, we could move the Date helper methods in FileManager+ZIP into a dedicated file. That is a low hanging fruit and would fix the file length warning for that file.

No worries, if you don't have time for those cleanups - I can also look into that when I work on the project the next time. Would it be okay for you if I push directly onto your PR branch?

Add a method for zipping a file on disk and returning an in-memory archive containing the zipped file.

Providing this method seems a bit too convenient IMO. As far as I can see it only wraps zipItem(... to ...) and implicitly creates an in-memory archive. What happens if a user innocently passes in a large directory here? I think it'd be better to remove this method and only offer zipItem(... to ...), because the latter requires explicit creation of an in-memory archive. Therefore users have to manually indicate their intent.

[WowbaggersLiquidLunch]

You can only set SWIFT_VERSION to a single value.

Would using a .xcworkspace that refer to multiple .xcodeproj files, with each of them targeting a different Swift version, work? It will be very hacky tho.

Can you please revert the following changes for now:

• .xctestplan
• .xcscheme
• .azure-pipelines.yml

Just did. Sorry, I should have consulted you first before making those changes.

Would it be okay for you if I push directly onto your PR branch?

Yep.

What happens if a user innocently passes in a large directory here?

This is a problem I overlooked. I suppose the OS would just keep on paging until it crashes. What if we add some checks and have it throw an error if there isn't enough memory? Would it make the convenient method valuable to have?

Maybe I can refactor the methods further to make zipping and unzipping truly location-agnostic?

[weichsel]

Would using a .xcworkspace that refer to multiple .xcodeproj files, with each of them targeting a different Swift version, work? It will be very hacky tho.

I'd like to get rid of anything Xcode related altogether at some point and solely rely on the information provided by the SPM metadata.

Just did. Sorry, I should have consulted you first before making those changes.

Thank you. I will re-add some of your changes (esp. the CI test matrix 👍) in another PR when switching to GitHub Actions.

This is a problem I overlooked. I suppose the OS would just keep on paging until it crashes. What if we add some checks and have it throw an error if there isn't enough memory? Would it make the convenient method valuable to have?

An API user should consider the memory requirements of the code while writing the code. Hence I think that an error thrown at runtime is too late here.

I am currently juggling a lot of different tasks, so I might be slow to respond - But I will try to help out with this PR when I can.

[clementbarry]

Could this be merged for iOS/macOS/tvOS/watchOS first, and solve Linux_Swift as a separate PR?

[WowbaggersLiquidLunch]

It's been a while since I last worked on this. If I remember correctly, it's some of my ill-written test cases that are causing the CI failures. I will reinvestigate them.

I think I can also mark the new methods introduced in this pull request with @available or put them in #if conditional compilation blocks, if @weichsel thinks it's ok.

Add a method for zipping a file on disk and returning an in-memory archive containing the zipped file.

Providing this method seems a bit too convenient IMO. As far as I can see it only wraps zipItem(... to ...) and implicitly creates an in-memory archive. What happens if a user innocently passes in a large directory here? I think it'd be better to remove this method and only offer zipItem(... to ...), because the latter requires explicit creation of an in-memory archive. Therefore users have to manually indicate their intent.

^ There is also this thing I need to address first.