pack: include contents of directories in files field (#3175)

* Use tar-fs instead of tar-stream in `yarn pack` (and fix packed emojis) This lets tar-fs do the [header construction] for us. [header construction]: https://github.com/mafintosh/tar-fs/blob/b79d82a79c5e21f6187462d7daaba1fc03cdd1de/index.js#L101-L127 I tested this by comparing the output of this command before and after the change: ./bin/yarn.js pack >/dev/null && tar tvf yarn-v0.24.0-0.tgz | sort && wc -c < yarn-v0.24.0-0.tgz && rm *tgz Here's the diff between the outputs: ```diff diff --git a/before.txt b/after.txt index 5e7f370e..5565a808 100644 --- a/before.txt +++ b/after.txt @@ -7,13 +7,13 @@ -rw-r--r-- 0 0 0 657 Mar 4 07:19 package/Dockerfile.dev -rw-r--r-- 0 0 0 1346 Mar 4 07:19 package/LICENSE -rw-r--r-- 0 0 0 1789 Apr 17 15:10 package/jenkins_jobs.groovy --rw-r--r-- 0 0 0 3061 Mar 4 07:19 package/README.md --rw-r--r-- 0 0 0 3438 Apr 17 16:18 package/package.json +-rw-r--r-- 0 0 0 3057 Mar 4 07:19 package/README.md +-rw-r--r-- 0 0 0 3430 Apr 17 16:18 package/package.json -rwxr-xr-x 0 0 0 42 Mar 4 07:19 package/bin/yarnpkg -rwxr-xr-x 0 0 0 172 Mar 4 07:19 package/bin/node-gyp-bin/node-gyp -rwxr-xr-x 0 0 0 906 Mar 4 07:19 package/bin/yarn -rwxr-xr-x 0 0 0 929 Apr 10 15:59 package/bin/yarn.js drwxr-xr-x 0 0 0 0 Apr 10 15:59 package/bin drwxr-xr-x 0 0 0 0 Apr 17 17:04 package drwxr-xr-x 0 0 0 0 Mar 4 07:19 package/bin/node-gyp-bin - 6206 + 6177 ``` I extracted the tarballs into `./package-master` and `./package-feature`, then diffed them to find that this change has the side effect of fixing emojis in the tarball. You can see examples of the broken emoji here: * https://unpkg.com/[email protected]/package.json * https://unpkg.com/[email protected]/README.md ```diff diff --git a/package-master/README.md b/package-feature/README.md index aabfc24f..6aff13d8 100644 --- a/package-master/README.md +++ b/package-feature/README.md @@ -30,7 +30,7 @@ * **Network Performance.** Yarn efficiently queues up requests and avoids request waterfalls in order to maximize network utilization. * **Network Resilience.** A single request failing won't cause an install to fail. Requests are retried upon failure. * **Flat Mode.** Yarn resolves mismatched versions of dependencies to a single version to avoid creating duplicates. -* **More emojis.** ð�� +* **More emojis.** 🐈 ## Installing Yarn diff --git a/package-master/package.json b/package-feature/package.json index c89ad7a6..8e7e3bc7 100644 --- a/package-master/package.json +++ b/package-feature/package.json @@ -4,7 +4,7 @@ "version": "0.24.0-0", "license": "BSD-2-Clause", "preferGlobal": true, - "description": "ð��¦ð�� Fast, reliable, and secure dependency management.", + "description": "📦🐈 Fast, reliable, and secure dependency management.", "dependencies": { "babel-runtime": "^6.0.0", "bytes": "^2.4.0", ``` * When testing `yarn pack`, use fs.walk instead of fs.readdir This ensures that files inside directories are listed too. * Add failing test for packing directories recursively yarnpkg/yarn#2498 * `pack`: include contents of directories in `files` field This makes it so that you don't have to put '/**' after a directory in the `files` field of package.json to ensure that the contents of the directory will be published. Fixes yarnpkg/yarn#2498 Fixes yarnpkg/yarn#2942 Fixes yarnpkg/yarn#2851 Includes and closes yarnpkg/yarn#3170 * `pack` test: Use path.join() to create nested path * `path` test: Make output easier to understand Now, we can see just what the expected/actual difference is, rather than just getting a -1 vs 0 from an indexOf test. * `pack`: transform each [ "file-name" ] into [ "file-name", "file-name/**" ], whether it's a file or a folder See yarnpkg/yarn#3175 (comment) * Account for backslashes in paths when filtering files See yarnpkg/yarn#3175 (comment) * Use `path.sep` instead of slashes See yarnpkg/yarn#3175 (comment) * Revert "Use `path.sep` instead of slashes" This reverts commit c2df043343092dcc408f8792ad16eb86cad6ba3a. It caused an additional test to fail: https://ci.appveyor.com/project/kittens/yarn/build/2195/job/q5u26f85qlroy533#L3011 * Revert "Account for backslashes in paths when filtering files" This reverts commit 20646f5d4ac5207dbf929af4e96acebeca29d07f. I don't think it actually helps, see yarnpkg/yarn#3175 (comment) * Keep pattern in IgnoreFilter, use with minimatch() in matchesFilter This should help with Windows support. See yarnpkg/yarn#3175 (comment) * Update ignoreLinesToRegex tests
lovelypuppy0607 · Apr 28, 2017 · ebdeb7f · ebdeb7f
1 parent 2163759
commit ebdeb7f
Show file tree

Hide file tree

Showing 6 changed files with 55 additions and 75 deletions.
diff --git a/__tests__/commands/pack.js b/__tests__/commands/pack.js
@@ -92,7 +92,7 @@ export async function getFilesFromArchive(source, destination): Promise<Array<st
       .on('error', reject);
   });
   await unzip;
-  const files = await fs.readdir(destination);
+  const files = (await fs.walk(destination)).map(({relative}) => relative);
   return files;
 }
 
@@ -115,10 +115,14 @@ test.concurrent('pack should include all files listed in the files array', (): P
       path.join(cwd, 'files-include-v1.0.0.tgz'),
       path.join(cwd, 'files-include-v1.0.0'),
     );
-    const expected = ['index.js', 'a.js', 'b.js'];
-    expected.forEach((filename) => {
-      expect(files.indexOf(filename)).toBeGreaterThanOrEqual(0);
-    });
+    expect(files.sort()).toEqual([
+      'a.js',
+      'b.js',
+      'dir',
+      path.join('dir', 'nested.js'),
+      'index.js',
+      'package.json',
+    ]);
   });
 });
 

diff --git a/__tests__/fixtures/pack/files-include/dir/nested.js b/__tests__/fixtures/pack/files-include/dir/nested.js
@@ -0,0 +1,2 @@
+/* @flow */
+console.log('hello world');
diff --git a/__tests__/fixtures/pack/files-include/package.json b/__tests__/fixtures/pack/files-include/package.json
@@ -3,5 +3,5 @@
   "version": "1.0.0",
   "main": "index.js",
   "license": "MIT",
-  "files": ["index.js", "a.js", "b.js"]
+  "files": ["index.js", "a.js", "b.js", "dir/"]
 }
diff --git a/__tests__/util/filter.js b/__tests__/util/filter.js
@@ -29,22 +29,22 @@ test('ignoreLinesToRegex', () => {
     '! F # # ',
     '#! G',
   ])).toEqual([
-    {base: '.', isNegation: false, regex: /^(?:(?=.)a)$/i},
-    {base: '.', isNegation: false, regex: /^(?:(?=.)b)$/i},
-    {base: '.', isNegation: false, regex: /^(?:(?=.)c)$/i},
-    {base: '.', isNegation: false, regex: /^(?:(?=.)d #)$/i},
-    {base: '.', isNegation: false, regex: /^(?:(?=.)e#)$/i},
-    {base: '.', isNegation: false, regex: /^(?:(?=.)f #)$/i},
-    {base: '.', isNegation: false, regex: /^(?:(?=.)g#)$/i},
-    {base: '.', isNegation: false, regex: /^(?:(?=.)h # foo)$/i},
-    {base: '.', isNegation: false, regex: /^(?:(?=.)i# foo)$/i},
-    {base: '.', isNegation: false, regex: /^(?:(?=.)j # foo #)$/i},
-    {base: '.', isNegation: false, regex: /^(?:(?=.)k # foo # #)$/i},
-    {base: '.', isNegation: true, regex: /^(?:(?=.)A)$/i},
-    {base: '.', isNegation: true, regex: /^(?:(?=.)B)$/i},
-    {base: '.', isNegation: true, regex: /^(?:(?=.)C)$/i},
-    {base: '.', isNegation: true, regex: /^(?:(?=.)D #)$/i},
-    {base: '.', isNegation: true, regex: /^(?:(?=.)E #)$/i},
-    {base: '.', isNegation: true, regex: /^(?:(?=.)F # #)$/i},
+    {base: '.', isNegation: false, pattern: 'a', regex: /^(?:(?=.)a)$/i},
+    {base: '.', isNegation: false, pattern: 'b ', regex: /^(?:(?=.)b)$/i},
+    {base: '.', isNegation: false, pattern: ' c ', regex: /^(?:(?=.)c)$/i},
+    {base: '.', isNegation: false, pattern: 'd #', regex: /^(?:(?=.)d #)$/i},
+    {base: '.', isNegation: false, pattern: 'e#', regex: /^(?:(?=.)e#)$/i},
+    {base: '.', isNegation: false, pattern: 'f # ', regex: /^(?:(?=.)f #)$/i},
+    {base: '.', isNegation: false, pattern: 'g# ', regex: /^(?:(?=.)g#)$/i},
+    {base: '.', isNegation: false, pattern: 'h # foo', regex: /^(?:(?=.)h # foo)$/i},
+    {base: '.', isNegation: false, pattern: 'i# foo', regex: /^(?:(?=.)i# foo)$/i},
+    {base: '.', isNegation: false, pattern: 'j # foo #', regex: /^(?:(?=.)j # foo #)$/i},
+    {base: '.', isNegation: false, pattern: 'k # foo # #', regex: /^(?:(?=.)k # foo # #)$/i},
+    {base: '.', isNegation: true, pattern: 'A', regex: /^(?:(?=.)A)$/i},
+    {base: '.', isNegation: true, pattern: ' B', regex: /^(?:(?=.)B)$/i},
+    {base: '.', isNegation: true, pattern: ' C ', regex: /^(?:(?=.)C)$/i},
+    {base: '.', isNegation: true, pattern: ' D #', regex: /^(?:(?=.)D #)$/i},
+    {base: '.', isNegation: true, pattern: ' E # ', regex: /^(?:(?=.)E #)$/i},
+    {base: '.', isNegation: true, pattern: ' F # # ', regex: /^(?:(?=.)F # #)$/i},
   ]);
 });
diff --git a/src/cli/commands/pack.js b/src/cli/commands/pack.js
@@ -9,7 +9,7 @@ import {MessageError} from '../../errors.js';
 
 const zlib = require('zlib');
 const path = require('path');
-const tar = require('tar-stream');
+const tar = require('tar-fs');
 const fs2 = require('fs');
 
 const IGNORE_FILENAMES = [
@@ -54,18 +54,6 @@ const NEVER_IGNORE = ignoreLinesToRegex([
   '!/+(changes|changelog|history)*',
 ]);
 
-function addEntry(packer: any, entry: Object, buffer?: ?Buffer): Promise<void> {
-  return new Promise((resolve, reject) => {
-    packer.entry(entry, buffer, function(err) {
-      if (err) {
-        reject(err);
-      } else {
-        resolve();
-      }
-    });
-  });
-}
-
 export async function pack(config: Config, dir: string): Promise<stream$Duplex> {
   const pkg = await config.readRootManifest();
   const {bundledDependencies, main, files: onlyFiles} = pkg;
@@ -97,6 +85,7 @@ export async function pack(config: Config, dir: string): Promise<stream$Duplex>
     ];
     lines = lines.concat(
       onlyFiles.map((filename: string): string => `!${filename}`),
+      onlyFiles.map((filename: string): string => `!${path.join(filename, '**')}`),
     );
     const regexes = ignoreLinesToRegex(lines, '.');
     filters = filters.concat(regexes);
@@ -129,46 +118,28 @@ export async function pack(config: Config, dir: string): Promise<stream$Duplex>
   // apply filters
   sortFilter(files, filters, keepFiles, possibleKeepFiles, ignoredFiles);
 
-  const packer = tar.pack();
-  const compressor = packer.pipe(new zlib.Gzip());
-
-  await addEntry(packer, {
-    name: 'package',
-    type: 'directory',
+  const packer = tar.pack(config.cwd, {
+    ignore: (name) => {
+      const relative = path.relative(config.cwd, name);
+      // Don't ignore directories, since we need to recurse inside them to check for unignored files.
+      if (fs2.lstatSync(name).isDirectory()) {
+        const isParentOfKeptFile = Array.from(keepFiles).some((name) =>
+          !path.relative(relative, name).startsWith('..'));
+        return !isParentOfKeptFile;
+      }
+      // Otherwise, ignore a file if we're not supposed to keep it.
+      return !keepFiles.has(relative);
+    },
+    map: (header) => {
+      const suffix = header.name === '.' ? '' : `/${header.name}`;
+      header.name = `package${suffix}`;
+      delete header.uid;
+      delete header.gid;
+      return header;
+    },
   });
 
-  for (const name of keepFiles) {
-    const loc = path.join(config.cwd, name);
-    const stat = await fs.lstat(loc);
-
-    let type: ?string;
-    let buffer: ?Buffer;
-    let linkname: ?string;
-    if (stat.isDirectory()) {
-      type = 'directory';
-    } else if (stat.isFile()) {
-      buffer = await fs.readFileRaw(loc);
-      type = 'file';
-    } else if (stat.isSymbolicLink()) {
-      type = 'symlink';
-      linkname = await fs.readlink(loc);
-    } else {
-      throw new Error();
-    }
-
-    const entry = {
-      name: `package/${name}`,
-      size: stat.size,
-      mode: stat.mode,
-      mtime: stat.mtime,
-      type,
-      linkname,
-    };
-
-    await addEntry(packer, entry, buffer);
-  }
-
-  packer.finalize();
+  const compressor = packer.pipe(new zlib.Gzip());
 
   return compressor;
 }

diff --git a/src/util/filter.js b/src/util/filter.js
@@ -12,6 +12,7 @@ export type IgnoreFilter = {
   base: string,
   isNegation: boolean,
   regex: RegExp,
+  pattern: string,
 };
 
 export function sortFilter(
@@ -99,7 +100,8 @@ export function matchesFilter(filter: IgnoreFilter, basename: string, loc: strin
   }
   return filter.regex.test(loc) ||
          filter.regex.test(`/${loc}`) ||
-         filter.regex.test(basename);
+         filter.regex.test(basename) ||
+         minimatch(loc, filter.pattern);
 }
 
 export function ignoreLinesToRegex(lines: Array<string>, base: string = '.'): Array<IgnoreFilter> {
@@ -131,6 +133,7 @@ export function ignoreLinesToRegex(lines: Array<string>, base: string = '.'): Ar
           base,
           isNegation,
           regex,
+          pattern,
         };
       } else {
         return null;