`pack`: include contents of directories in `files` field #3175

josephfrazier · 2017-04-18T03:59:15Z

Summary

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 #2498
Fixes #2942
Fixes #2851

Includes and closes #3170

Test plan

I modified one of the pre-existing pack tests to also include and check
for a file inside a directory listed in package.json.

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", ```

This ensures that files inside directories are listed too.

yarnpkg#2498

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#2498 Fixes yarnpkg#2942 Fixes yarnpkg#2851 Includes and closes yarnpkg#3170

TrySound · 2017-04-18T07:20:24Z

Fixes #2986
Fixes #3166

arcanis · 2017-04-19T16:02:31Z

A test seems to be failing on Windows, can you check?

Now, we can see just what the expected/actual difference is, rather than just getting a -1 vs 0 from an indexOf test.

arcanis · 2017-04-20T10:05:36Z

The Windows test still breaks.

Btw, do you support this edge case: { "files": [ "test/*" ] }? Since it doesn't evaluate to a valid stat call it will get added as-is, so if it happens to match a directory it probably won't pack any file inside.

Maybe a better way to handle this would be to transform each [ "file-name" ] into [ "file-name", "file-name/**" ], whether it's a file or a folder. Glob will filter them accordingly anyway.

…/**" ], whether it's a file or a folder See yarnpkg#3175 (comment)

josephfrazier · 2017-04-20T11:40:07Z

Yeah.... I don't have a Windows machine readily available to develop on, so I've been relying on AppVeyor as a bit of debugging tool (hence 517d7df) :P

I just pushed a commit that tries the [ "file-name" ] into [ "file-name", "file-name/**" ] approach, and it looks like that gets a little closer to passing CI (only one missing file, rather than two). Thanks for the suggestion!

As for the { "files": [ "test/*" ] } edge case, I tried changing the dir/ into dir/* here, and the tests still pass locally, so hopefully that won't be an issue, especially after switching to your recommended way of handling this.

I'm going to be mostly unavailable until next week, but I should be able to take another look at this then. In the mean time, feel free to push more commits onto this branch if you have some insight on how to fix the Windows tests.

bestander · 2017-04-20T23:06:13Z

Thanks a lot for sending the fix, @josephfrazier.
Looking forward to see the Windows tests pass.

EnoahNetzach · 2017-04-24T14:34:50Z

The main problem with Windows is the path separator: in places like this one it is assumed it's / (in the regexs).

See yarnpkg#3175 (comment)

EnoahNetzach · 2017-04-24T14:50:53Z

src/util/filter.js

@@ -99,6 +99,7 @@ export function matchesFilter(filter: IgnoreFilter, basename: string, loc: strin
  }
  return filter.regex.test(loc) ||
         filter.regex.test(`/${loc}`) ||
+         filter.regex.test(`\\${loc}`) ||


why not using path.sep?

uh, wait it produces \something, and it's interpreted as an escape token... Sorry..

maybe path.sep.replace('\\', '\\\\')?

The problem I see with keeping both / and \\ is that the backslash has a meaning in unix systems, so false-positives could occur.

The argument to test is a string rather than a RegExp pattern, so it shouldn't be interpreting backslashes as the escape character in this context. Note that the following expression (on Windows) should produce a string whose first character is a backslash:

`${path.sep}${loc}`

That said, I'm not sure why c2df043 seemed to make things worse, test-wise (an additional test failed)

See yarnpkg#3175 (comment)

EnoahNetzach · 2017-04-24T15:22:01Z

Printing all the regexs that are tested, I see a lot of unix separators:

(1) /^(?:\/(?=.)package\.json)$/i
(2) /^(?:\/(?=.)readme[^\/]*?)$/i
(3) /^(?:\/(?!\.)(?=.)(?:license|licence)+[^\/]*?)$/i 
(4) /^(?:\/(?!\.)(?=.)(?:changes|changelog|history)+[^\/]*?)$/i
(5) /^(?:(?=.)index\.js)$/i
(6) /^(?:(?=.)a\.js)$/i
(7) /^(?:(?=.)b\.js)$/i
(8) /^(?:(?=.)dir\/)$/i
(9) /^(?:(?=.)index\.js\/(?:(?!(?:\/|^)\.).)*?)$/i
(10) /^(?:(?=.)a\.js\/(?:(?!(?:\/|^)\.).)*?)$/i
(11) /^(?:(?=.)b\.js\/(?:(?!(?:\/|^)\.).)*?)$/i
(12) /^(?:(?=.)dir\/(?:(?!(?:\/|^)\.).)*?)$/i
(13) /^(?:(?!\.)(?=.)[^\/]*?)$/i

2, 3, 4, 8, 9, 10, 11, 12, and 13 all have a \/, whereas probably there should be a \${path.sep}, again..

Or am I overcomplicating things?

(Debugging in Windows is such a cumbersome and weird experience..)

josephfrazier · 2017-04-24T15:47:49Z

The regexes are being generated in ignoreLinesToRegex, which uses minimatch to build the regex, which normalizes paths to POSIX-style:

Accordingly, maybe we should change the logic in matchesFilter to use Minimatch#match instead of manually operating on the regexes.

This reverts commit c2df043. It caused an additional test to fail: https://ci.appveyor.com/project/kittens/yarn/build/2195/job/q5u26f85qlroy533#L3011

This reverts commit 20646f5. I don't think it actually helps, see yarnpkg#3175 (comment)

This should help with Windows support. See yarnpkg#3175 (comment)

josephfrazier · 2017-04-24T16:48:48Z

@bestander, @arcanis: The (relevant, non-flaky) tests are passing on all platforms now. Can you have another look when you get a chance? For reference, here are the changes since last week: https://github.com/yarnpkg/yarn/pull/3175/files/cbdfee66af0b4f16d999af31eebb66a486a463e7..37fc30aaf6f006a0a5f7a6fb731cca20c3d197a8

arcanis · 2017-04-28T16:42:03Z

Merged! Thanks! :)

josephfrazier · 2017-04-28T17:49:25Z

Thanks! I think this also fixes #3166 (thanks for noticing, @TrySound!)

TrySound · 2017-04-28T19:02:25Z

@josephfrazier Thanks for PR :)

`pack`: include contents of directories in `files` field (yarnpkg#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

josephfrazier added 4 commits April 17, 2017 17:08

When testing yarn pack, use fs.walk instead of fs.readdir

e78247d

This ensures that files inside directories are listed too.

Add failing test for packing directories recursively

6626c5e

yarnpkg#2498

arcanis mentioned this pull request Apr 19, 2017

Use tar-fs instead of tar-stream in yarn pack (and fix packed emojis) #3170

Closed

josephfrazier added 2 commits April 19, 2017 14:50

pack test: Use path.join() to create nested path

362f71a

path test: Make output easier to understand

517d7df

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…

cbdfee6

…/**" ], whether it's a file or a folder See yarnpkg#3175 (comment)

Account for backslashes in paths when filtering files

20646f5

See yarnpkg#3175 (comment)

EnoahNetzach reviewed Apr 24, 2017

View reviewed changes

Use path.sep instead of slashes

c2df043

See yarnpkg#3175 (comment)

josephfrazier added 4 commits April 24, 2017 12:02

Revert "Use path.sep instead of slashes"

64fb412

This reverts commit c2df043. 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"

fcedf60

This reverts commit 20646f5. I don't think it actually helps, see yarnpkg#3175 (comment)

Keep pattern in IgnoreFilter, use with minimatch() in matchesFilter

d621ef2

This should help with Windows support. See yarnpkg#3175 (comment)

Update ignoreLinesToRegex tests

37fc30a

arcanis approved these changes Apr 28, 2017

View reviewed changes

arcanis merged commit 3bd3f17 into yarnpkg:master Apr 28, 2017

bestander mentioned this pull request Apr 28, 2017

improvement behavior of files field in package.json #3166

Closed

DanBuild mentioned this pull request Apr 28, 2017

yarn-homebrew #4 failed #3275

Closed

GulajavaMinistudio mentioned this pull request Apr 29, 2017

pack: include contents of directories in files field (#3175) GulajavaMinistudio/yarn#24

Merged

GulajavaMinistudio added a commit to GulajavaMinistudio/yarn that referenced this pull request Apr 29, 2017

Merge pull request #24 from yarnpkg/master

de56bcb

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

DanBuild mentioned this pull request May 2, 2017

yarn-homebrew #5 failed #3304

Closed

josephfrazier deleted the pack-dir branch May 23, 2017 18:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`pack`: include contents of directories in `files` field #3175

`pack`: include contents of directories in `files` field #3175

josephfrazier commented Apr 18, 2017 •

edited

Loading

TrySound commented Apr 18, 2017

arcanis commented Apr 19, 2017

arcanis commented Apr 20, 2017 •

edited

Loading

josephfrazier commented Apr 20, 2017

bestander commented Apr 20, 2017

EnoahNetzach commented Apr 24, 2017

EnoahNetzach Apr 24, 2017

EnoahNetzach Apr 24, 2017

EnoahNetzach Apr 24, 2017

josephfrazier Apr 24, 2017

EnoahNetzach commented Apr 24, 2017 •

edited

Loading

josephfrazier commented Apr 24, 2017

josephfrazier commented Apr 24, 2017

arcanis commented Apr 28, 2017

josephfrazier commented Apr 28, 2017

TrySound commented Apr 28, 2017

pack: include contents of directories in files field #3175

pack: include contents of directories in files field #3175

Conversation

josephfrazier commented Apr 18, 2017 • edited Loading

TrySound commented Apr 18, 2017

arcanis commented Apr 19, 2017

arcanis commented Apr 20, 2017 • edited Loading

josephfrazier commented Apr 20, 2017

bestander commented Apr 20, 2017

EnoahNetzach commented Apr 24, 2017

EnoahNetzach Apr 24, 2017

Choose a reason for hiding this comment

EnoahNetzach Apr 24, 2017

Choose a reason for hiding this comment

EnoahNetzach Apr 24, 2017

Choose a reason for hiding this comment

josephfrazier Apr 24, 2017

Choose a reason for hiding this comment

EnoahNetzach commented Apr 24, 2017 • edited Loading

josephfrazier commented Apr 24, 2017

josephfrazier commented Apr 24, 2017

arcanis commented Apr 28, 2017

josephfrazier commented Apr 28, 2017

TrySound commented Apr 28, 2017

`pack`: include contents of directories in `files` field #3175

`pack`: include contents of directories in `files` field #3175

josephfrazier commented Apr 18, 2017 •

edited

Loading

arcanis commented Apr 20, 2017 •

edited

Loading

EnoahNetzach commented Apr 24, 2017 •

edited

Loading