Use gunzip-maybe instead of reimplementing

[victornoel]

Summary

Following the PR to use tar-fs (#2826), I was looking at ways to solve #2629, and noticed that yarn had its own ad-hoc implementation of gunzipping the archives while there exists an available implementation already, by the same person who made tar-fs.

Even though it does not improve the situation for #2629 (of course), I thought it would make sense to use an existing library instead of an adhoc implementation.

Test plan

I ran yarn in a project with lots of dependencies, no node_modules and an empty cache.
I used the default network concurrency of 8.
Everything worked as expected.

[bestander]

Looks like the right idea but there is a test error
TypeError: Cannot read property 'ended' of undefined
at Transform.Object..Writable.write (/home/ubuntu/yarn/node_modules/peek-stream/node_modules/readable-stream/lib/_stream_writable.js:191:12)
at Duplexify.Object..Duplexify._write (/home/ubuntu/yarn/node_modules/duplexify/index.js:201:22)
at doWrite (/home/ubuntu/yarn/node_modules/readable-stream/lib/_stream_writable.js:347:64)
at writeOrBuffer (/home/ubuntu/yarn/node_modules/readable-stream/lib/_stream_writable.js:336:5)
at Duplexify.Object..Writable.write (/home/ubuntu/yarn/node_modules/readable-stream/lib/_stream_writable.js:274:11)
at Gunzip.ondata (_stream_readable.js:555:20)
at emitOne (events.js:96:13)
at Gunzip.emit (events.js:188:7)
at readableAddChunk (_stream_readable.js:176:18)
at Gunzip.Readable.push (_stream_readable.js:134:10)
at Gunzip.Transform.push (_stream_transform.js:128:32)
FAIL tests/commands/install/integration.js

[victornoel]

is there any way to see which test is failing? the test file is huge…

[bestander]

Just scroll to the bottom, it says which file it is

…

On Thu, 23 Mar 2017 at 09:18, Victor Noël ***@***.***> wrote: is there any way to see which test is failing? the test file is huge… — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2971 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWPuN9dRXi_EWwY9E0F8e2qUoCJ_Wks5rojjlgaJpZM4MlRpn> .

[victornoel]

@bestander it says which file it is (__tests__/commands/install/integration.js) but not which test it is in this file, and there is a bunch of tests there…

[bestander]

You are right, it is not able to run the whole test suite because of the crash.
Can you reproduce this locally?
If yes then try running them one by one by replacing all test.concurrent with test, it will show on which test it is failing

[victornoel]

Ok, so I never managed to have jest print the name of the failing test, but with a bit of help of dichotomy magic, I narrowed the problem to the third test changes the cache directory when bumping the cache version.

I will investigate

[victornoel]

Ok, so, I'm not clear on the real reason why it doesn't work.

To me the problem is coming from readable-stream which is a transitive dependencies of gunzip-maybe (via peek-stream and through2) in a very old version.

If I force the version of readable-stream/through2 to be the latest, then things work without problem. I will open an issue for the right package (I think peek-stream) and until then let's put this PR on hold.

[bestander]

Thanks for investigating, @victornoel.
Ping me when there is some resolution

[bestander]

@victornoel, any luck?
Should we close the PR while it is not in a mergeable state or you want to give it another try soon?

[victornoel]

@bestander I just took a look at it again. Basically the problem is that some dependencies of gunzip-maybe are too old and buggy (readable-stream precisely). I opened an issue for peek-stream (the one depending on an old version of through2 that bring the old readable-stream with it) but I got no answer from its maintainer (I pinged him again today).

So either there is any way to force the version of a dependency's dependency with yarn (except via editing the yarn.lock file by hand :), but I couldn't find one documented, or we simply need to wait. In the latter case, you can close this if you prefer, and I will reopen another PR when needed :) Thanks for your patience!

[victornoel]

@bestander don't close, I should be pushing soon a working PR :)

[victornoel]

not sure why it fails... seems there is some timeout issues

[victornoel]

anyway, my PR is good as it is, the dependencies were updated in gunzip-maybe's dependencies and the test that was failing before does not fail anymore.

[victornoel]

@bestander I've rebased with latest master. The travis build passes, but the other two seems to timeout on one of the test (the same one "root install with optional deps", once with node 6, once with node 4). It runs fine on my machine (both with node 6 and 4), so I'm not sure what to do about that...

[bestander]

Sometimes CI fails, I am working on making e2e tests more stable

…

On Sat, 15 Apr 2017 at 09:36, Victor Noël ***@***.***> wrote: @bestander <https://github.com/bestander> I've rebased with latest master. The travis build passes, but the other two seems to timeout on one of the test (the same one "root install with optional deps", once with node 6, once with node 4). It runs fine on my machine (both with node 6 and 4), so I'm not sure what to do about that... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2971 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWIC9KLXYRTISdUDFa2QxFtxA3sMqks5rwIF_gaJpZM4MlRpn> .

[arcanis]

Since the failing tests seem unrelated and there's already a few other PRs related to tar streams, I'll merge this one before it requires any more rebase :)

[victornoel]

cool, thanks :)