search

tsolomko / SWCompression

A Swift framework for working with compression, archives and containers.
MIT License
233 stars 39 forks source link

AR Format Support #31

Open Lakr233 opened 2 years ago

Lakr233 commented 2 years ago

We have implemented support for BSD AR file format at: https://github.com/Lessica/SWCompression/tree/feature-bsd-ar

The reason not opening a pull request is that we have noticed you are submoduling a git repo for test files and your pipeline seems to be a little heavy for configurations. During our development, we simplified these process and we make all our AR file test work locally.

We would like yourself to merge the codes if you are ok with it. Estimated more than one repo are required to update for this merge.

The package structure are a little different from the upstream, follow are the changes we made.

Sources/SWCompression/AR
.
├── ArContainer.swift
├── ArEntry.swift
├── ArEntryInfo.swift
├── ArError.swift
├── ArHeader.swift
├── ArParser.swift
├── ArReader.swift
├── ArWriter.swift
├── Data+Ar.swift
└── LittleEndianByteReader+Ar.swift

Are the sources for AR API interface, supports reading and writing.

Sources/SWCompression/Common/Extensions.swift

Is a extension for ourself.

SWCompression/TAR/TarReader.swift

Is a bug fix for Apple stuff returning mismatch item from their document. If a nil is returned, do not throw an error.

Tests/SWCompressionTests/
.
├── ArCreateTests.swift
├── ArReaderTests.swift
├── ArTests.swift
└── ArWriterTests.swift

Tests/SWCompressionTests/Test Files/AR

Are tests we made.

Following files are changed but not required for merge, generally speaking, we move the unicode test string payload to files.

Tests/SWCompressionTests/Constants.swift
SevenZipTests.swift
TarReaderTests.swift 
TarTests.swift
ZipTests.swift
...

With above changes applied to, we can make another check in README~

|       | Zlib | GZip | XZ  | ZIP | TAR | 7-Zip | AR  |
| ----- | ---- | ---- | --- | --- | --- | ----- | --- |
| Read  | ✅   | ✅    | ✅  | ✅  | ✅   | ✅    | ✅  |
| Write | ✅   | ✅    | TBD | TBD | ✅   | TBD   | ✅ |

Have a nice day~

tsolomko commented 2 years ago

Hi @Lakr233,

This is very interesting! I will have a closer look at the implementation and merging it when I have more time, but I have two questions nevertheless.

  1. My current plan is to move away in the future from the "algorithm"-specific error types, such as TarError, DeflateError, etc. As part of this plan all new additions use a more general error type, DataError. I've noticed that you add a new error type ArError, so my question: is it possible to use instead DataError?

  2. With regards to this issue:

The reason not opening a pull request is that we have noticed you are submoduling a git repo for test files and your pipeline seems to be a little heavy for configurations. During our development, we simplified these process and we make all our AR file test work locally.

What did you find particularly complicated about my setup/pipeline? I genuinely curious about this, maybe there is something I can do to simplify it in the future.

Lakr233 commented 2 years ago
tsolomko commented 2 years ago

Hi again,

I've been thinking about this and I don't think I can accept these changes without an actual pull request.

The problem is that as it currently stands what you're essentially asking me here is to copy someone else's code (the repository that you've linked here belongs to a different user, not to you) without their express permission.

With the PR from the author I will be assured that:

  1. They consent to contribution to this project;
  2. On what terms this contribution happens (to be precise, this would be on the terms of MIT license due to the inbound=outbound default Github's policy described in its Terms of Service).
Lakr233 commented 2 years ago

I’m the collaborator of that repo. I have add a notice there in the README to authorize this action. It’s a little bit complicated to explain why not a PR but it should explain the right for you to do so. The author is my friend and we work together for many.

The original code license(I see you did with MIT License) could be applied.

mickeyl commented 1 year ago

Would love to see the support for AR. What's holding this back?

tsolomko commented 1 year ago

@mickeyl

Well, my concerns with accepting the contribution discussed in this issue have never been addressed.

At the same time, I do not see adding support for AR as a high priority for this project, and unfortunately I do not have enough time even for higher priority stuff. So as I result, I have never got around to implementing AR by myself.

Lakr233 commented 1 year ago

I dont understand what you stated as "never been addressed".