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

AR Format Support #31

Open
Lakr233 opened this issue Jan 22, 2022 · 7 comments
Open

AR Format Support #31

Lakr233 opened this issue Jan 22, 2022 · 7 comments

Comments

@Lakr233
Copy link

Lakr233 commented Jan 22, 2022

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
Copy link
Owner

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
Copy link
Author

Lakr233 commented Jan 22, 2022

  • I've noticed that you add a new error type ArError, so my question: is it possible to use instead DataError?
    yes

  • What did you find particularly complicated about my setup/pipeline?
    The main reason is that we do not have permission to access your submodule repo and to avoid road blocks in development. Don't feel like to make changes to your workflow.

@tsolomko
Copy link
Owner

tsolomko commented Jan 23, 2022

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
Copy link
Author

Lakr233 commented Jan 24, 2022

  • They consent to contribution to this project

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.

  • 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).

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

@mickeyl
Copy link

mickeyl commented Jun 14, 2023

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

@tsolomko
Copy link
Owner

tsolomko commented Jun 15, 2023

@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
Copy link
Author

Lakr233 commented Jun 17, 2023

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

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

No branches or pull requests

3 participants