-
Notifications
You must be signed in to change notification settings - Fork 18
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
[Bugfix] Switch tar crate to make tar.gz decompression work #28
base: main
Are you sure you want to change the base?
Conversation
happy to add tests. The fixtures currently come from some temp repo with no instructions. Im not sure what is a good way to generate a test fixture for the PAX 1.0 file format - https://www.gnu.org/software/tar/manual/html_node/PAX-1.html |
I just ran into #27. Is there anything preventing this from being merged? This is a pretty important fix since it's somewhat impossible to tell ahead of time what tarfiles will work and what won't. /cc @bigfootjon I suppose? |
I suspect tests are the long pole here? /cc @zertosh Also, the fixtures still point to a personal repo (which is something we needed to do when this repo was still private prior to launch so it didn't break the internal Meta CI), but ideally that would also be changed now, e.g.: dotslash/tests/generate_fixtures.py Line 28 in 76bbc25
dotslash/tests/dotslash_tests.rs Line 704 in 76bbc25
|
@zertosh has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@kageiit can you rebase this PR? |
@zertosh has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
I am conflicted here. I can repro the problem, and I can also repro that switching to @bolinfest, wdyt? |
FWIW, looks like alexcrichton/tar-rs will be maintained again: alexcrichton/tar-rs#379 |
@abhinav so is a bump to the latest https://crates.io/crates/tar (currently 0.4.42) all that is needed now? |
Yes, I believe so, but @kageiit or @zertosh would have to confirm whether their repros are fixed with the upgrade. |
Let me confirm |
Unfortunately, tar 0.4.42 does not fix this. I just tried it and it fails the same way. I'm still going to update to 0.4.42 since I did the work internally for that anyway |
Summary: Thought this maybe fixed facebook/dotslash#28 but it doesn't, but since I already have the changes, might as well keep them. Reviewed By: diliop Differential Revision: D64220404 fbshipit-source-id: 3495e432d3b8c5aa82bd9fbd6f01877619a32a03
This pull request has been merged in 4ee240e. |
No no... I just referenced this PR saying that the tar 0.4.42 update doesn't fix the issue |
Fixes #27