Skip to content

[Bugfix] Switch tar crate to make tar.gz decompression work#28

Closed
kageiit wants to merge 1 commit into
facebook:mainfrom
kageiit:switch_tar
Closed

[Bugfix] Switch tar crate to make tar.gz decompression work#28
kageiit wants to merge 1 commit into
facebook:mainfrom
kageiit:switch_tar

Conversation

@kageiit

@kageiit kageiit commented May 25, 2024

Copy link
Copy Markdown

Fixes #27

@kageiit

kageiit commented May 25, 2024

Copy link
Copy Markdown
Author

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

@thoughtpolice

thoughtpolice commented Aug 22, 2024

Copy link
Copy Markdown

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?

@bolinfest

Copy link
Copy Markdown
Contributor

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.:

GITHUB_REPO = "https://github.com/zertosh/dotslash_fixtures"

.arg("https://github.com/zertosh/dotslash_fixtures/raw/462625c6bf2671439dce66bd5bc40b05f2ed8819/pack.tar.gz")

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@zertosh has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@bigfootjon

Copy link
Copy Markdown
Contributor

@kageiit can you rebase this PR?

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@zertosh has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@zertosh

zertosh commented Sep 11, 2024

Copy link
Copy Markdown
Contributor

I am conflicted here. I can repro the problem, and I can also repro that switching to binstall_tar fixes at least the ruff extracting issue. BUT, I don't have a lot of confidence in binstall_tar. It looks like they just grabbed the latest alexcrichton/tar-rs and applied (blindly?) many outstanding PRs. I don't know about the quality or how vetted anything has been. I also tried composefs/tar-rs#298 by itself and it addresses the ruff issue too.

@bolinfest, wdyt?

@abhinav

abhinav commented Oct 3, 2024

Copy link
Copy Markdown
Contributor

FWIW, looks like alexcrichton/tar-rs will be maintained again: composefs/tar-rs#379
The sparse files PR was merged 2 weeks ago: composefs/tar-rs#375

@bolinfest

Copy link
Copy Markdown
Contributor

@abhinav so is a bump to the latest https://crates.io/crates/tar (currently 0.4.42) all that is needed now?

@abhinav

abhinav commented Oct 10, 2024

Copy link
Copy Markdown
Contributor

@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.

@kageiit

kageiit commented Oct 10, 2024

Copy link
Copy Markdown
Author

@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

@zertosh

zertosh commented Oct 11, 2024

Copy link
Copy Markdown
Contributor

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

facebook-github-bot pushed a commit to facebookincubator/below that referenced this pull request Oct 11, 2024
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
@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request has been merged in 4ee240e.

@zertosh zertosh reopened this Oct 11, 2024
@zertosh

zertosh commented Oct 11, 2024

Copy link
Copy Markdown
Contributor

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

@kageiit kageiit closed this Jan 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Some tar.gz archives dont decompress correctly

7 participants