weichsel
Sort symlinks in order required for unzip
Changes proposed in this PR
- Fix extraction order of symlinks
Tests performed
- Existing tests
- Extracting the AltServer download from https://altstore.io/
Further info for the reviewer
This needs more tests to ensure full coverage, but also to prove that this was previously broken and is now fixed.
The added function is public because I'm using it to extract a subset of the entries in the archive that contains symlinks.
Open Issues
#185, possibly #62.
This PR fixes an issue I was having in using ZIPFoundation to extract a Cocoa app from a zip file. Is there anything I can do to help speed along the review and merge?
This PR fixes an issue I was having in using ZIPFoundation to extract a Cocoa app from a zip file. Is there anything I can do to help speed along the review and merge?
I think there are going to need to be tests added but looking at the existing tests I'm not sure how to write one to test this. @marcprux If you're familiar with it maybe you could help me with this?
@weichsel Would you be able to look at this PR please?
@JosephDuffy I have this PR on my agenda and I already skimmed over it - the approach looks well thought trough and promising. Thank you for submitting it.
I won't have time for a proper review for another week, but I have it on my TODO list. There's one other bigger PR I have to review first, but I'll move to this one right thereafter.
/cc @marcprux
FWIW, I've been using my own patched fork for a few days and have used it to unpack a variety of very large zip files with symbolic links, and everything has been working perfectly (i.e., identical to the output of unzip) so far.
@JosephDuffy I have this PR on my agenda and I already skimmed over it - the approach looks well thought trough and promising. Thank you for submitting it.
I won't have time for a proper review for another week, but I have it on my TODO list. There's one other bigger PR I have to review first, but I'll move to this one right thereafter.
Thank you for the confirmation -- I can use my fork for the time being. Knowing that it has a chance to get merged in the future is more than enough for me for now :)
FWIW, I've been using my own patched fork for a few days and have used it to unpack a variety of very large zip files with symbolic links, and everything has been working perfectly (i.e., identical to the output of
unzip) so far.
Have you made any changes on top of the changes in this PR? I've also not ran in to any issues yet but wanted to make sure you haven't fixed an error I've not run in to yet.
Have you made any changes on top of the changes in this PR
No, I don't think so. You can see my fork at https://github.com/glimpseio/ZIPFoundation
I finally found some time to look into this - sorry for the long delay!
The code looks good and it's a great improvement over my existing implementation for symlink sorting (which was a bit too naive).
I directly pushed some changes onto your branch - I hope this is OK. If not, please revert my commits and I'll branch-off another branch.
There are some minor issues open (the way destinationPath is built during sorting is not correct for all use cases and test coverage for some of the sorting conditions is still missing). I hope I find some time to do some more work on this today or tomorrow.
I finally found some time to look into this - sorry for the long delay!
Thank you for taking a look!
I directly pushed some changes onto your branch - I hope this is OK. If not, please revert my commits and I'll branch-off another branch.
Not a problem, I'm happy to keep all the changes in this branch/PR.
There are some minor issues open (the way
destinationPathis built during sorting is not correct for all use cases and test coverage for some of the sorting conditions is still missing). I hope I find some time to do some more work on this today or tomorrow.
Is this because of the way the path is create (with split etc.) and would be fixed by using URL/URLComponents, or another issue?
One thing I'll note about the changes: being able to sort a subset of the entries is quite useful for my use case. I'm finding all apps inside ZIPs and then extracting them separately. I'm already iterating over all the entries but only have to sort a subset of them, but with this I would always have to sort of them.
If possible I'd like to keep a method of sorting an array of entries externally.
Feel free to reject this idea if you feel it's of scope; I'm only trying to explain the reason why I initially designed it like this.
@weichsel Do you think it would be possible to get this merged and released? Got stung by this issue today.
Hey @weichsel @JosephDuffy, would love to see this merged, what's currently missing?
I am more than happy to add finishing touches if there is any need.
btw, thank you so much for this great project, I am amazed by the quality 👏
Hey @weichsel @JosephDuffy, would love to see this merged, what's currently missing?
I am more than happy to add finishing touches if there is any need.
btw, thank you so much for this great project, I am amazed by the quality 👏
I never managed to add tests, so I'm sure they'd be welcomed.
I don't currently plan to update this PR; I need to use my fork because it exposes the function to sort an array of the Entrys .
I just merged PR #263 into development. The changes there make symlink sorting obsolete and also fix all problems that were addressed by this PR.
Thanks to @JosephDuffy for providing a viable workaround for all ZIP Foundation users running into problems with nested symlinks. Sorry again for taking so long to get this fixed.