Skip to content
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

Fix tar.gz extraction for regressions testsuite file #1759

Closed
wants to merge 1 commit into from

Conversation

strarsis
Copy link
Contributor

@strarsis strarsis commented Mar 24, 2023

The SVG test suite tar.gz file appears to be also gunzip-compressed, hence tar-stream alone used in the current regression test setup fails to extract the file, hence the whole regressions test fails:
Download W3C SVG 1.1 Test Suite and extract svg files Error: Invalid tar header. Maybe the tar is corrupted or it needs to be gunzipped?

This PR adds gunzip-maybe in between that can handle gunzip-compressed archives (as recommended by the tar-stream docs).

@strarsis strarsis changed the title Fix tar.gz extraction for regressions tests testsuite file Fix tar.gz extraction for regressions testsuite file Mar 24, 2023
Copy link
Member

@SethFalco SethFalco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think instead of adding the dev dependency, it'd be better for us to just take the one function call that actually matters.

        swap(null, pumpify(zlib.createGunzip(), gunzip(maxRecursion - 1)))

https://github.com/mafintosh/gunzip-maybe/blob/master/index.js#L21

The rest is just various checks or determining whether calling zlib#createGunzip is required or not. As we're hard-coding the URL, I think we can handle things ourselves.

Based on this, both review comments are redundant, but I thought it was worth sharing the FYI anyway! As I'd already tested this approach locally, I've created a separate PR for it here:

@@ -110,6 +110,7 @@
"css-select": "^5.1.0",
"css-tree": "^2.2.1",
"csso": "^5.0.5",
"gunzip-maybe": "^1.4.2",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This belongs in devDependencies!
When adding it, you'd do:

yarn add gunzip-maybe --dev`

@@ -0,0 +1,4872 @@
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing you used npm to install the new dependency. This project uses Yarn, so normally you should manage dependencies with Yarn instead!

You'll be able to tell because there is a yarn.lock file in the root of the repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants