Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfixes to yarn pack, retry of #1464 #2062

Merged
merged 3 commits into from
Nov 29, 2016
Merged

Conversation

tkloht
Copy link
Contributor

@tkloht tkloht commented Nov 29, 2016

This is an updated version of #1464 which had to be reverted because of ci errors.
The errors were caused by changes in the yarn code after the PR was created (util/fs.copy() has an additional parameter).
This is fixed in this PR.

@bestander you asked me to ping you when this is done.

below is the original message of #1464:

Summary
Contains two fixes for yarn pack if "files" is used in package.json:

  • some files (package.json, readme, license, changelog) should always be included, even if not listed in the files field. This was not the case, fixed by 13e7b42
  • all other files should be excluded, but the exclude filter did not match files starting with a dot (this is the default behaviour of minimatch it seems). fixed by 8851acb

Please note that this only partly fixes #755 .
As far as I can see what happens in #755 is the following:

  • some files which should be included we not included, in my example it was package.json
  • some files which should be excluded were included, in my example .gitignore and others.

While working on this I was wondering why a missing file would create a corrupt archive, but it seemed to be fixed at first. Then I added some more testcases and noticed that it is still possible to create corrupt archives, but this is not limited to the usage of "files" - see #1442.
Basically I think some directory was falsely included, and that invoked the bug described in #1442 .
But a missing or included file does not directly result in a corrupt archive.
Hope this makes sense, I find it difficult to explain right now.

Test plan
I have added some tests for this, just run yarn test.
If you want to see the failing tests checkout 6612ec6.

npm pack includes some files even if they are not included in the “files” field in package.json. This commit creates the same behaviour in yarn pack.
if files is used in package.json all other files should be excluded. Files with dot as first character have to be excluded explicitly because minimatch will not match them by default.
@tkloht tkloht changed the title Pack files bugfixes to yarn pack, retry of #1464 Nov 29, 2016
@bestander
Copy link
Member

Great job!

@bestander bestander merged commit 8779130 into yarnpkg:master Nov 29, 2016
@tkloht
Copy link
Contributor Author

tkloht commented Nov 29, 2016

thanks!

@garthk
Copy link

garthk commented Jan 5, 2017

Just spotted this or a close relative in 0.18.1; will revisit when 0.19 drops.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

published package is corrupted if "files" is used in package.json
3 participants