switched to tar-fs from node-tar to unpack tars #2826

bestander · 2017-03-02T19:44:31Z

Summary

A mitigation for #2629
Also fixes #961

Based on a few ad-hoc tests (//gitlab.com/linagora/petals-cockpit.git#075bac4c54fee466568c000c7ffe8025f593e212) tar-fs seems to make less IO thus is more stable and a bit faster.

I noticed that we never tested git repos that support archive.
Did they ever work?

Test data:

Using offline-mirror (no download), on my MBPro 13", clean cache and using node-tar to unzip files.
Concurrency 12 - fails
Concurrency 8 - 18 seconds
Concurrency 4 - 18 seconds
Concurrency 2 - 21 seconds
Using offline-mirror (no download), on my MBPro 13", clean cache and using tar-fs to unzip files.
Concurrency 12 - 15 seconds
Concurrency 8 - 15 seconds
Concurrency 4 - 17 seconds
Concurrency 2 - 18 seconds
Downloading packages from internet, on my MBPro 13", clean cache and using tar-fs to unzip files.
Concurrency 12 - failed once
Concurrency 8 - 21 seconds
Concurrency 4 - 23 seconds
Concurrency 2 - 34 seconds

Test plan

All changes are covered with tests (verified every change)
Could not properly test git archives (see git archive capability works? #2831) but did an ad-hoc test via yarn add ssh://[email protected]/bestander/test-repo.git and made sure that archive was accessed and installed.

…f node-tar

bestander · 2017-03-03T11:33:29Z

@arcanis, could you review this please?

arcanis · 2017-03-03T16:53:02Z

__tests__/commands/pack.js

+      .pipe(tarFs.extract(destination, {
+        strip: 1,
+        dmode: parseInt(555, 8), // all dirs should be readable
+        fmode: parseInt(444, 8), // all files should be readable


0o555 and 0o444 ?

Just noticed - you forgot this instance :)

arcanis · 2017-03-03T16:53:35Z

src/fetchers/git-fetcher.js

-      const untarStream = tar.Extract({path: this.dest});
+      const untarStream = tarFs.extract(this.dest, {
+        dmode: parseInt(555, 8), // all dirs should be readable
+        fmode: parseInt(444, 8), // all files should be readable


arcanis · 2017-03-03T16:54:16Z

src/fetchers/tarball-fetcher.js

+    const untarStream = tarFs.extract(this.dest, {
+      strip: 1,
+      dmode: parseInt(555, 8), // all dirs should be readable
+      fmode: parseInt(444, 8), // all files should be readable


Same - shouldn't the unpack routine be splitted into an helper, since it used in a few places?

Sometimes we need to strip first folder, sometimes not.
And moving dmode,fbmode into a separate function looks like an overkill

arcanis · 2017-03-03T16:56:26Z

src/util/git.js

+          parser.on('entry', (header, stream, next) => {
+            let string = '';
+            stream.on('data', (buffer) => {
+              string += buffer.toString();


iirc, this pattern might break when the buffer contains part of an unicode character but not the others. It might be better to concat the buffers together (possibly by storing them in a list and then concatenating everything at once with Buffer.concat) rather than concatenating strings.

Oh, smart!
I'll do that

arcanis · 2017-03-03T16:56:37Z

src/util/git.js

-        const extractor = tar.Extract({path: dest});
+        const extractor = tarFs.extract(dest, {
+          dmode: parseInt(555, 8), // all dirs should be readable
+          fmode: parseInt(444, 8), // all files should be readable


arcanis · 2017-03-03T16:56:43Z

src/util/git.js

-        const extractor = tar.Extract({path: dest});
+        const extractor = tarFs.extract(dest, {
+          dmode: parseInt(555, 8), // all dirs should be readable
+          fmode: parseInt(444, 8), // all files should be readable


bestander · 2017-03-07T00:14:51Z

ping @arcanis, ready for another round

arcanis · 2017-03-07T00:39:25Z

src/util/git.js

            stream.on('end', () => {
-              update(string);
+              fileContent += decoder.end();
+              update(fileContent);


Since it isn't really used anymore, maybe fileContent should be removed completely, using update(decoded.end()) instead

decoded.end() won't return a full string.
It would just return remaining bytes if there were any.
So I needed to accumulate them in a string

Oh I see, my bad ! I initially thought the decoder was holding the whole buffer 👍

This reverts commit 157a34a.

victornoel · 2017-03-21T16:33:22Z

@bestander it could make sense to also use gunzip-maybe (like tar-fs, it is by @mafintosh) instead of reimplementing it in utils/stream.

**Summary** Fixes: #992. This issue was supposed to be fixed by #2826 but it was not setting directory permissions wide enough. This patch uses the `readable` and `writable` options provided by the `tar-fs` package which essentially sets everything to `0o777`. **Test plan** N/A

* Fix: make sure all extracted tarballs are r/w enabled **Summary** Fixes: #992. This issue was supposed to be fixed by #2826 but it was not setting directory permissions wide enough. This patch uses the `readable` and `writable` options provided by the `tar-fs` package which essentially sets everything to `0o777`. **Test plan** N/A * fewer perms

* Fix: make sure all extracted tarballs are r/w enabled **Summary** Fixes: yarnpkg#992. This issue was supposed to be fixed by yarnpkg#2826 but it was not setting directory permissions wide enough. This patch uses the `readable` and `writable` options provided by the `tar-fs` package which essentially sets everything to `0o777`. **Test plan** N/A * fewer perms

bestander changed the title ~~Set default network concurrency to 8 and switched to tar-fs instead of node-tar~~ [WIP] Set default network concurrency to 8 and switched to tar-fs instead of node-tar Mar 2, 2017

bestander changed the title ~~[WIP] Set default network concurrency to 8 and switched to tar-fs instead of node-tar~~ [WIP] switched to tar-fs from node-tar to unpack tars Mar 3, 2017

bestander changed the title ~~[WIP] switched to tar-fs from node-tar to unpack tars~~ switched to tar-fs from node-tar to unpack tars Mar 3, 2017

bestander added 8 commits March 3, 2017 11:19

Set default network concurrency to 8 and switched to tar-fs instead o…

421d3c2

…f node-tar

migrated pack test

4d17831

another file migrated to tar-fs

78ed488

changed concurrency in another PR

e1fd3e0

wip migrating git to use tar-fs

c73e718

removed tar dependency

ff73f31

bump tar-stream

a80f9a7

reverted changes to url in hasArchiveCapability

c89e855

bestander force-pushed the safer-tar-lib branch from 569e89e to c89e855 Compare March 3, 2017 11:19

arcanis reviewed Mar 3, 2017

View reviewed changes

made changes according to feedback

f8d7f02

arcanis approved these changes Mar 7, 2017

View reviewed changes

bestander added 2 commits March 7, 2017 01:09

0o444

91b5857

added flow suppress

d7ae065

arcanis approved these changes Mar 7, 2017

View reviewed changes

bestander merged commit 157a34a into yarnpkg:master Mar 7, 2017

This was referenced Mar 8, 2017

yarn-e2e/label=docker,os=ubuntu-14.04 #123 failed #2866

Closed

yarn-e2e/label=docker,os=ubuntu-16.04 #123 failed #2867

Closed

yarn-e2e/label=docker,os=ubuntu-12.04 #123 failed #2868

Closed

skevy pushed a commit to expo/yarn that referenced this pull request Mar 8, 2017

Revert "switched to tar-fs from node-tar to unpack tars (yarnpkg#2826)"

7b9ef13

This reverts commit 157a34a.

lawliet89 mentioned this pull request Mar 13, 2017

Fix User Namespace issues with untaring #2898

Merged

This was referenced Mar 14, 2017

yarn-e2e/label=docker,os=ubuntu-14.04 #129 failed #2907

Closed

yarn-e2e/label=docker,os=ubuntu-16.04 #129 failed #2908

Closed

yarn-e2e/label=docker,os=ubuntu-12.04 #129 failed #2909

Closed

yarn-e2e/label=docker,os=ubuntu-14.04 #130 failed #2910

Closed

This was referenced Mar 14, 2017

yarn-e2e/label=docker,os=ubuntu-16.04 #130 failed #2911

Closed

yarn-e2e/label=docker,os=ubuntu-12.04 #130 failed #2912

Closed

victornoel mentioned this pull request Mar 22, 2017

Use gunzip-maybe instead of reimplementing #2971

Merged

nicoschoenmaker mentioned this pull request May 12, 2017

No read permission for group and others in a package yarn installed #1143

Closed

BYK mentioned this pull request Sep 4, 2017

Fix: make sure all extracted tarballs are r/w enabled #4301

Merged

switched to tar-fs from node-tar to unpack tars #2826

switched to tar-fs from node-tar to unpack tars #2826

Uh oh!

Conversation

bestander commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bestander commented Mar 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bestander commented Mar 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

victornoel commented Mar 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bestander commented Mar 2, 2017 •

edited

Loading