From 3bd3f17bf17e943bdd62101773c99fa0973cb66c Mon Sep 17 00:00:00 2001 From: Joseph Frazier <1212jtraceur@gmail.com> Date: Fri, 28 Apr 2017 12:41:51 -0400 Subject: [PATCH] `pack`: include contents of directories in `files` field (#3175) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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/yarn@0.22.0/package.json * https://unpkg.com/yarn@0.22.0/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 https://github.com/yarnpkg/yarn/issues/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 https://github.com/yarnpkg/yarn/issues/2498 Fixes https://github.com/yarnpkg/yarn/issues/2942 Fixes https://github.com/yarnpkg/yarn/issues/2851 Includes and closes https://github.com/yarnpkg/yarn/pull/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 https://github.com/yarnpkg/yarn/pull/3175#issuecomment-295663363 * Account for backslashes in paths when filtering files See https://github.com/yarnpkg/yarn/pull/3175#issuecomment-296687878 * Use `path.sep` instead of slashes See https://github.com/yarnpkg/yarn/pull/3175#discussion_r112967037 * 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 https://github.com/yarnpkg/yarn/pull/3175#issuecomment-296714233 * Keep pattern in IgnoreFilter, use with minimatch() in matchesFilter This should help with Windows support. See https://github.com/yarnpkg/yarn/pull/3175#issuecomment-296714233 * Update ignoreLinesToRegex tests --- __tests__/commands/pack.js | 14 ++-- .../fixtures/pack/files-include/dir/nested.js | 2 + .../fixtures/pack/files-include/package.json | 2 +- __tests__/util/filter.js | 34 ++++----- src/cli/commands/pack.js | 73 ++++++------------- src/util/filter.js | 5 +- 6 files changed, 55 insertions(+), 75 deletions(-) create mode 100644 __tests__/fixtures/pack/files-include/dir/nested.js diff --git a/__tests__/commands/pack.js b/__tests__/commands/pack.js index 935eee2a0d..ff136445b7 100644 --- a/__tests__/commands/pack.js +++ b/__tests__/commands/pack.js @@ -92,7 +92,7 @@ export async function getFilesFromArchive(source, destination): Promise 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 new file mode 100644 index 0000000000..66a7302f5c --- /dev/null +++ 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 index 917dfeb6d2..f6819044fe 100644 --- 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 index 8690f0c1f8..65e11f4e17 100644 --- 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 index 81ddcc8c9e..658206dba1 100644 --- 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 { - 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 { const pkg = await config.readRootManifest(); const {bundledDependencies, main, files: onlyFiles} = pkg; @@ -97,6 +85,7 @@ export async function pack(config: Config, dir: string): Promise ]; 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 // 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 index 915323c1de..a479bf3c10 100644 --- 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, base: string = '.'): Array { @@ -131,6 +133,7 @@ export function ignoreLinesToRegex(lines: Array, base: string = '.'): Ar base, isNegation, regex, + pattern, }; } else { return null;