feat!: use .npmignore file to limit which files are published

[lukekarrys]

Fixes: #2372

There was discussion about this being a breaking change in #2372, so I think v10.0.0 is a good time to make this change out of caution. I did err on the side of caution by not removing any gyp/pylib/generator files.

Here is the diff of files that will now be excluded from the published tarball with this change:

diff --git a/old.txt b/new.txt
index 712dc94..29b5fbf 100644
--- a/old.txt
+++ b/new.txt
@@ -1,29 +1,7 @@
-
-.github/ISSUE_TEMPLATE.md
-.github/PULL_REQUEST_TEMPLATE.md
-.github/scripts/check-engines.js
-.github/workflows/release-please.yml
-.github/workflows/tests.yml
-.github/workflows/visual-studio.yml
 addon.gypi
 bin/node-gyp.js
 CHANGELOG.md
 CONTRIBUTING.md
-docs/binding.gyp-files-in-the-wild.md
-docs/Error-pre-versions-of-node-cannot-be-installed.md
-docs/Force-npm-to-use-global-node-gyp.md
-docs/Home.md
-docs/Linking-to-OpenSSL.md
-docs/README.md
-docs/Updating-npm-bundled-node-gyp.md
-gyp/.github/workflows/node-gyp.yml
-gyp/.github/workflows/nodejs-windows.yml
-gyp/.github/workflows/Python_tests.yml
-gyp/.github/workflows/release-please.yml
-gyp/AUTHORS
-gyp/CHANGELOG.md
-gyp/CODE_OF_CONDUCT.md
-gyp/CONTRIBUTING.md
 gyp/data/win/large-pdb-shim.cc
 gyp/gyp
 gyp/gyp_main.py
@@ -91,20 +69,6 @@ gyp/pylib/packaging/version.py
 gyp/pyproject.toml
 gyp/README.md
 gyp/test_gyp.py
-gyp/tools/emacs/gyp-tests.el
-gyp/tools/emacs/gyp.el
-gyp/tools/emacs/README
-gyp/tools/emacs/run-unit-tests.sh
-gyp/tools/emacs/testdata/media.gyp
-gyp/tools/emacs/testdata/media.gyp.fontified
-gyp/tools/graphviz.py
-gyp/tools/pretty_gyp.py
-gyp/tools/pretty_sln.py
-gyp/tools/pretty_vcproj.py
-gyp/tools/README
-gyp/tools/Xcode/README
-gyp/tools/Xcode/Specifications/gyp.pbfilespec
-gyp/tools/Xcode/Specifications/gyp.xclangspec
 lib/build.js
 lib/clean.js
 lib/configure.js
@@ -124,33 +88,8 @@ lib/remove.js
 lib/util.js
 LICENSE
 macOS_Catalina_acid_test.sh
 package.json
 README.md
 SECURITY.md
 src/win_delay_load_hook.cc
-test/common.js
-test/fixtures/certs.js
-test/fixtures/nodedir/include/node/config.gypi
-test/fixtures/test-charmap.py
-test/fixtures/VS_2017_BuildTools_minimal.txt
-test/fixtures/VS_2017_Community_workload.txt
-test/fixtures/VS_2017_Express.txt
-test/fixtures/VS_2017_Unusable.txt
-test/fixtures/VS_2019_BuildTools_minimal.txt
-test/fixtures/VS_2019_Community_workload.txt
-test/fixtures/VS_2019_Preview.txt
-test/fixtures/VS_2022_Community_workload.txt
-test/simple-proxy.js
-test/test-addon.js
-test/test-configure-python.js
-test/test-create-config-gypi.js
-test/test-download.js
-test/test-find-accessible-sync.js
-test/test-find-node-directory.js
-test/test-find-python.js
-test/test-find-visualstudio.js
-test/test-install.js
-test/test-options.js
-test/test-process-release.js
-update-gyp.py

[DeeDeeG]

Looking at: What are the main exclusions with this change?

Unlike what the diff shows above, I think one of the bigger things this change would do is avoid shipping node-gyp's own test/ subdir. (As well as the gyp/tools subdir being the other big exclusion.) Between the two of those, that saves about ~450-550 kB, depending on how you count it... And saves 48 files+folders.

---

Looking into: which exact files are excluded? How many, and how big are they?

For what it's worth, I get a larger diff than the PR body text above indicates, when running npm pack with npm@10.2.1.

After this change:

Full diff of files I see being no-longer included in the npm packed tarball after this change (click to expand):

- .github/ISSUE_TEMPLATE.md
- .github/PULL_REQUEST_TEMPLATE.md
- .github/workflows/release-please.yml
- .github/workflows/tests.yml
- .github/workflows/visual-studio.yml
- CHANGELOG.md
- CONTRIBUTING.md
  LICENSE
  README.md
- SECURITY.md
  addon.gypi
  bin/node-gyp.js
- docs/Error-pre-versions-of-node-cannot-be-installed.md
- docs/Force-npm-to-use-global-node-gyp.md
- docs/Home.md
- docs/Linking-to-OpenSSL.md
- docs/README.md
- docs/Updating-npm-bundled-node-gyp.md
- docs/binding.gyp-files-in-the-wild.md
- gyp/.github/workflows/Python_tests.yml
- gyp/.github/workflows/node-gyp.yml
- gyp/.github/workflows/nodejs-windows.yml
- gyp/.github/workflows/release-please.yml
- gyp/AUTHORS
- gyp/CHANGELOG.md
- gyp/CODE_OF_CONDUCT.md
- gyp/CONTRIBUTING.md
  gyp/LICENSE
  gyp/README.md
  gyp/data/win/large-pdb-shim.cc
@@ -90,22 +67,6 @@
  gyp/pylib/packaging/tags.py
  gyp/pylib/packaging/utils.py
  gyp/pylib/packaging/version.py
- gyp/pyproject.toml
- gyp/test_gyp.py
- gyp/tools/README
- gyp/tools/Xcode/README
- gyp/tools/Xcode/Specifications/gyp.pbfilespec
- gyp/tools/Xcode/Specifications/gyp.xclangspec
- gyp/tools/emacs/README
- gyp/tools/emacs/gyp-tests.el
- gyp/tools/emacs/gyp.el
- gyp/tools/emacs/run-unit-tests.sh
- gyp/tools/emacs/testdata/media.gyp
- gyp/tools/emacs/testdata/media.gyp.fontified
- gyp/tools/graphviz.py
- gyp/tools/pretty_gyp.py
- gyp/tools/pretty_sln.py
- gyp/tools/pretty_vcproj.py
  lib/Find-VisualStudio.cs
  lib/build.js
  lib/clean.js
@@ -122,33 +83,5 @@
  lib/rebuild.js
  lib/remove.js
  lib/util.js
- macOS_Catalina_acid_test.sh
  package.json
  src/win_delay_load_hook.cc
- test/common.js
- test/fixtures/VS_2017_BuildTools_minimal.txt
- test/fixtures/VS_2017_Community_workload.txt
- test/fixtures/VS_2017_Express.txt
- test/fixtures/VS_2017_Unusable.txt
- test/fixtures/VS_2019_BuildTools_minimal.txt
- test/fixtures/VS_2019_Community_workload.txt
- test/fixtures/VS_2019_Preview.txt
- test/fixtures/VS_2022_Community_workload.txt
- test/fixtures/certs.js
- test/fixtures/nodedir/include/node/config.gypi
- test/fixtures/test-charmap.py
- test/process-exec-sync.js
- test/reporter.js
- test/simple-proxy.js
- test/test-addon.js
- test/test-configure-python.js
- test/test-create-config-gypi.js
- test/test-download.js
- test/test-find-accessible-sync.js
- test/test-find-node-directory.js
- test/test-find-python.js
- test/test-find-visualstudio.js
- test/test-install.js
- test/test-options.js
- test/test-process-release.js
- update-gyp.py

With ncdu I get this output when extracting the tarball as of before/after this change:

• Total disk usage: 2.5 MiB; Apparent size: 2.1 MiB; Items: 180 (BEFORE)
• Total disk usage: 1.7 MiB; Apparent size: 1.5 MiB; Items: 97 (AFTER)

I suppose it may be worthwhile from some folks' perspectives to shave off this bit of filesize and reduce I/O just slightly when installing this package.

---

Some feedback: Consider keeping CHANGELOG.md and SECURITY.md??

---

My personal thoughts:

I'm personally neutral on the topic of whether to exclude a bunch of files from the package as published to npm package registry.

(I'm sympathetic to the person who was worried about test/fixtures/server.key setting off automated security audit programs, but that file has indeed been removed from the repo since the time when #2372 was opened.)

Just reducing the size of node-gyp seems less pressing to me personally, since it's already fairly small IMO and doesn't shrink super drastically, though I can understand why some people would want it. It can reduce the overall size of anything that bundles node-gyp, such as npm, and by reducing the size of npm, thereby reduce the size of some very common distributions of node. So, a small improvement in filesize/count, but an improvement nonetheless. I don't think most of the excluded files would be missed in production or "in the wild", to to speak.

[lukekarrys]

One other thing I would like to do before landing this is to come up with a way to run the test suite against a packed and installed tarball in CI. That will give me more confidence that future changes to gyp-next don't exclude files needed to run.

[lukekarrys]

Consider keeping CHANGELOG.md and SECURITY.md

Those files were showing as still packed when I ran my tests, but I likely made a mistake and changed the files array further after grabbing the diff. I'll confirm through a test in CI that the files we think should be in the tarball are there.

[lukekarrys]

I made a few changes to this PR:

• I used an .npmignore file instead of package.json#files. I usually prefer to use an allowlist vs denylist pattern for packing, but in this case if new files are introduced by next-gyp they will be included by default, as long as they aren't in an already ignored directory.
• Added a new CI job that will
  1. pack node-gyp
  2. unpack it into a temp dir
  3. copy the tests into that temp dir
  4. run npm i && npm test in the temp dir

I also updated the diff in the PR body to show the latest changes to using the .npmignore file.

[lukekarrys]

It is also important to note that with the inclusion of an .npmignore file, npm will stop using the .gitignore file. Looking through the .gitignore there were some patterns for files that did not currently exist in the repo, but in order to be safe I copied those patterns from the gitignore to npmignore.

[lukekarrys]

@cclauss Do you have an opinion on running npx envinfo in CI? It takes around 4 minutes on Windows which does not seem worth it to me. I removed it from the workflows in my latest commit, but can add it back if you or others get value from it.

[cclauss]

I do not use Windows but… four minutes is a long time in CI test runs so I believe you made the right choice.

Sometimes I add an on demand option to GitHub workflows and then only on those runs add extra stuff. This allows repo maintainers to see the extra stuff whenever it suits them but does not slow down normal CI runs.

on:
    workflow_dispatch:
        …

Thanks massively for all your work to modernize node-gyp! Some of the changes might ruffle some feathers but modernization will help to make this software more reliable. You can’t make an omelet…