-
Notifications
You must be signed in to change notification settings - Fork 506
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
feat: extractor support for .pkg and .zst packages #1580
Conversation
@terriko @BreadGenie I understand that this is not the complete PR but I wanted to confirm if I'm working in the right direction! |
Codecov Report
@@ Coverage Diff @@
## main #1580 +/- ##
==========================================
- Coverage 81.44% 81.02% -0.43%
==========================================
Files 290 290
Lines 5811 5834 +23
Branches 957 961 +4
==========================================
- Hits 4733 4727 -6
- Misses 856 884 +28
- Partials 222 223 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it's working well! (Tried it with openssl)
But do we need 2 approaches for extraction? I think the python implementation works fine and fast since zstandard python package is just an interface for the C library.
@BreadGenie how do the extraction functions look now? |
raise Exception("tar is required to extract .pkg files") | ||
else: | ||
stdout, stderr, return_code = await aio_run_command( | ||
["tar", "xf", filename, "-C", extraction_path] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be replaced by tarfile
? If tarfile
is a slower approach then let's not do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How's this going? This is still marked as a draft but it looks like it might be fairly close to complete. Looks like there's still a section to convert to using tarfile, then some docs updates to indicate that .pkg and .zst are supported.
@BreadGenie @terriko tarfile seems to be having a problem handling these .pkg files. Should I add 7z support so that the extractor works on windows too? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'm on board with the plan of merging this as is and refining later if we can figure out how to use tarfile. But let's get a comment explaining why tar is being used instead of tarfile right beside the code so that it's clear after this PR gets merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I'm going to update this branch to see how CI runs before merging, but I think it's ready to go. Thanks for working with us through trying some different implementation ideas!
fixes #1555