feat: add TAR parser #33975
feat: add TAR parser #33975
Conversation
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 suggestion.
Comments skipped due to low confidence (1)
packages/playwright-core/src/utils/tar.ts:78
- The chunk method does not handle the case where the fileStream is null properly. Add a check to ensure fileStream is not null before writing to it.
assert(this.fileStream);
This comment has been minimized.
This comment has been minimized.
|
@mxschmitt i've added another test that uses one of our chromium builds. Looks like the extractor is working well! |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
I had to make the following 3 changes in order to make it work with chrome-mac-arm64.tar.br. Not a huge fan of downloading the entire browser during the test-run - imo its fine to not have a unit test-coverage on it - since when we use it the bots will give us enough confidence / coverage. Lets maybe squeeze it into the actual PR when we implement br/tar file format? Also thinking if we should verify the checksums.
| } | ||
|
|
||
| async writeToDisk(outputPath: (path: string) => string) { | ||
| const fullPath = outputPath(this.name); |
There was a problem hiding this comment.
why is outputPath a callback? Ideally its a string and we should add a check so that users can't write 'outside' of the outputPath.
There was a problem hiding this comment.
I made this a callback instead of a raw string, so that the calling side can implement logic like this:
https://gist.github.com/mxschmitt/0807f729ba68358e58bf5210b4607fdc#file-har-extract-mjs-L71-L76
It seemed important that we can do that, but at the same time it also didn't seem like a property of the TAR extraction, so it can live outside of it now.
|
Good catch on the "prefix" field. Reading through the spec, this seemed like a legacy field to me so I omitted it initially. Added it back in 2c9bbf7. |
I'd prefer merging it individually, so the TAR PR doesn't become insanely big.
Makes sense! I'll remove the unit tests before merging then.
The TAR spec says |
Test results for "tests 1"10 flaky37298 passed, 662 skipped Merge workflow run. |
|
we discovered some problems of this approach around tars with long filenames, and chose to go with a 3rd-party implementation for the time being. |
Adds a TAR parser + basic test based on Max's draft. We'll use this in the Brotli-downloader, so we don't have to add dependencies.