Nuke node modules #43

PetarKirov · 2021-02-08T22:31:45Z

[.editorconfig]: Enable trim_trailing_whitespace
[package.json]: Add @vercel/ncc@^0.27.0 dev dependency
[build]: Replace node_modules with a single-file bundle produced by ncc

Detailed description of changes:
- Update the build script in package.json so that it builds and "links" the TypeScript code along with all of its (transitive) dependencies in a single index.js file using @vercel/ncc.
- Add / copy scripts/{Invoke-7zdec.ps1,externals/7zdec.exe} from node_modules/@actions/tool-cache/scripts as its needed by the aforementioned dependency on Windows and it used to be included in the node_modules folder which will be deleted by the next commit.
- Change the main file in package.json and action.yml to dist/index.js. ncc allows only the out dir can be specified - the filename is always index.js.
- Add lib/ and dist/* to .gitingore, but exclude dist/index.js from the list so it will be committed.
- Delete the lib/ folder
- Add the dist/index.js file produced by running npm run build.
- Add a .gitattributes file in order to set mark dist/** as binary for git diff and merge purposes. A wildcard is used as a sourcemap file could also be included later if necessary, which should also be considered a "binary" file.
- Update .gitignore to the latest version from here: https://github.com/github/gitignore/blob/master/Node.gitignore
- Set the target in tsconfig.json to es2017, which is supported by current Node.js version - v12, as should make for cleaner generated "down-leveled" code as it has native async/await suppor
[node_modules]: Delete the folder from git
[package.json]: Upgrade dependencies to their latest available versions
[ci]: Verify that dist/index.js is correctly generated
[package.json]: Bump the version to 1.1.0
And also update the description field.

Geod24 · 2021-02-09T00:38:38Z

Maybe you want to do some of those things in a separate PR ? E.g. bumping tsconfig (did you revert to ES2015?).
We could also use a more recent version of node in the process. Node 14 is the currently active LTS.

PetarKirov · 2021-02-09T06:03:31Z

Maybe you want to do some of those things in a separate PR ?

My plan is to get the CI green and then I can split the changes in separate PR if necessary.

bumping tsconfig

I wanted to go straight for the final version, since I don't to pollute the git history even more unused binary artifacts :D

did you revert to ES2015?

I'm trying to debug why it fails on Windows.

We could also use a more recent version of node in the process. Node 14 is the currently active LTS

I actually locally upgraded Node.js from v12 to v14, but in VSCode, I got a warning saying that according to the schema for action.yml files only node12 is supported, so I backed off from that change. I can try it anyway though.

Edit: Indeed only node12 is supported currently: actions/runner#772

PetarKirov · 2021-02-09T13:26:57Z

Committing node_modules, especially when using npm is so broken... while debugging locally, I decided to checkout the v1 branch, delete node_modules and install again. Now I have giant diff in node_modules which looks like this:

diff --git a/node_modules/@actions/core/package.json b/node_modules/@actions/core/package.json
index 5b66e79..ca8275c 100644
--- a/node_modules/@actions/core/package.json
+++ b/node_modules/@actions/core/package.json
@@ -2,7 +2,7 @@
   "_args": [
     [
       "@actions/[email protected]",
-      "/Users/geod24/projects/dlang/setup-dlang"
+      "/home/zlx/code/repos/dlang/dlang-community/setup-dlang"
     ]
   ],

--- a/node_modules/@actions/exec/package.json
+++ b/node_modules/@actions/exec/package.json
@@ -1,36 +1,39 @@
{
-  "_from": "@actions/exec@^1.0.0",
+  "_args": [
+    [
+      "@actions/[email protected]",
+      "/home/zlx/code/repos/dlang/dlang/dlang-community/setup-dlang"
+    ]
+  ],
+  "_from": "@actions/[email protected]",
 "_id": "@actions/[email protected]",
 "_inBundle": false,
 "_integrity": "sha512-4DPChWow9yc9W3WqEbUj8Nr86xkpyE29ZzWjXucHItclLbEW6jr80Zx4nqv18QL6KK65+cifiQZXvnqgTV6oHw==",
 "_location": "/@actions/exec",
 "_phantomChildren": {},
 "_requested": {
-    "type": "range",
+    "type": "version",
   "registry": true,
-    "raw": "@actions/exec@^1.0.0",
+    "raw": "@actions/[email protected]",
   "name": "@actions/exec",
   "escapedName": "@actions%2fexec",
   "scope": "@actions",
-    "rawSpec": "^1.0.0",
+    "rawSpec": "1.0.4",
   "saveSpec": null,
-    "fetchSpec": "^1.0.0"
+    "fetchSpec": "1.0.4"
 },
 "_requiredBy": [
   "/@actions/tool-cache"
 ],
 "_resolved": "https://registry.npmjs.org/@actions/exec/-/exec-1.0.4.tgz",
-  "_shasum": "99d75310e62e59fc37d2ee6dcff6d4bffadd3a5d",
-  "_spec": "@actions/exec@^1.0.0",
-  "_where": "/Users/geod24/projects/dlang/setup-dlang/node_modules/@actions/tool-cache",
+  "_spec": "1.0.4",
+  "_where": "/home/zlx/code/repos/dlang/dlang/dlang-community/setup-dlang",
 "bugs": {
   "url": "https://github.com/actions/toolkit/issues"
 },
-  "bundleDependencies": false,
 "dependencies": {
   "@actions/io": "^1.0.1"
 },
-  "deprecated": false,

🤦

PetarKirov · 2021-02-09T14:44:44Z

Enabling ldc-1.19.0
Downloading https://github.com/ldc-developers/ldc/releases/download/v1.19.0/ldc2-1.19.0-windows-multilib.7z
Error: The process 'C:\windows\System32\WindowsPowerShell\v1.0\powershell.exe' failed with exit code 1
    at ExecState._setResult (D:\a\setup-dlang\setup-dlang\dist\index.js:1:22124)
    at ExecState.CheckComplete (D:\a\setup-dlang\setup-dlang\dist\index.js:1:21686)
    at ChildProcess.<anonymous> (D:\a\setup-dlang\setup-dlang\dist\index.js:1:20534)
    at ChildProcess.emit (events.js:210:5)
    at maybeClose (internal/child_process.js:1021:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:283:5)
Error: The process 'C:\windows\System32\WindowsPowerShell\v1.0\powershell.exe' failed with exit code 1

Despite the completely unhelpful error message, I think I found the issue - it tries to execute the script Invoke-7zdec.ps1, which is now missing:
https://github.com/actions/toolkit/tree/%40actions/tool-cache%401.1.1/packages/tool-cache/scripts

I see two possible solutions:

Don't use @actions/tool-cache for extracting *.7z files, and directly call 7z like the setup-ruby does: https://github.com/ruby/setup-ruby/blob/fdcfbcf14ec9672f6f615cb9589a1bc5dd69d262/windows.js#L71
Copy the scripts folder from the link above here. Actually it was already present under: ${REPO_ROOT}/node_modules/@actions/tool-cache/scripts, so we can just move it to ${REPO_ROOT}/.

It seems to me that 2. is the simplest one and I will go with it. The disadvantage is that updating @actions/tool-cache in theory could break the setup, since a new version of @actions/tool-cache could rely on an also different version of the Invoke-7zdec.ps1 script. That said, this problem can easily be detected with an additional CI check in this repo.

mihails-strasuns · 2021-02-09T15:19:57Z

Hm, so the bundling tool doesn't take care of any additional resources that can be provided by the package? It only bundles JS and discards the rest?

PetarKirov · 2021-02-09T15:37:02Z

Hm, so the bundling too doesn't take care of any additional resources that can be provided by the package? It only bundles JS and discards the rest?

A JS bundler (like webpack, which ncc uses internally) simply follows the import statements / require calls. That's why in frontend projects, if you want to bundle your assets along with your code in a single bundle you're expected to import them - see e.g https://create-react-app.dev/docs/adding-images-fonts-and-files/.

In this case, @actions/tool-cache simply expects to find the script under ${__dirname}/../scripts/Invoke-7zdec.ps1:
https://github.com/actions/toolkit/blob/228a9534d1f2e5a31363a5ffa9ab47629c22d337/packages/tool-cache/src/tool-cache.ts#L181-L203 and does not import it in any way - it simply executes it as a Powershell script file.

The following files and folders are intensionally not committed: * `node_modules/.bin/ncc` * `node_modules/@vercel/` Since doing so would be pointless, because the next commit is going to delete them.

PetarKirov · 2021-02-09T16:01:43Z

The disadvantage is that updating @actions/tool-cache in theory could break the setup, since a new version of @actions/tool-cache could rely on an also different version of the Invoke-7zdec.ps1 script.

@mihails-strasuns if you're worried about this, if you prefer, I can simply change the build script to also do this:

git diff --no-index -- ./scripts ./node_modules/@actions/tool-cache/scripts

PetarKirov · 2021-02-09T16:09:23Z

@Geod24 @mihails-strasuns This PR should be finally ready for review - please take a look!

Geod24 · 2021-02-09T17:29:31Z

package.json

  },
  "devDependencies": {
-    "@types/node": "^12.7.4",
+    "@types/node": "^14.14.25",


Shouldn't that still be 12 if we're using node12, or it's unrelated?

You can think of @types/node as the druntime bindings in core.sys, but less important. It's less important, since it doesn't influence codegen / ABI, like core.sys, but simply type checking - it tells the TypeScript compiler which functions are available and what type of parameters they expect. Upgrading to a new version of @types/node is generally safe as almost always it means that they added new bindings or fixed existing ones.

So, unlike say freebsd, where you really need to match definitions in core.sys with version of freebsd that the final application will run on, @types/node can be used with both older and newer Node.js versions.

In the worst case, using a new version @types/node may allow us to use an API that is not available in older Node.js, but we have a CI pipeline to check for that ;)

… `ncc` Detailed description of changes: * Update the `build` script in `package.json` so that it builds and "links" the TypeScript code along with all of its (transitive) dependencies in a single `index.js` file using `@vercel/ncc`. * Add / copy `scripts/{Invoke-7zdec.ps1,externals/7zdec.exe}` from `node_modules/@actions/tool-cache/scripts` as its needed by the aforementioned dependency on Windows and it used to be included in the `node_modules` folder which will be deleted by the next commit. * Change the `main` file in `package.json` and `action.yml` to `dist/index.js`. `ncc` allows only the out dir can be specified - the filename is always `index.js`. * Add `lib/` and `dist/*` to .gitingore, but exclude `dist/index.js` from the list so it will be committed. * Delete the `lib/` folder * `node_modules` will be deleted in the next commit to make make the reviewing easier. * Add the `dist/index.js` file produced by running `npm run build`. * Add a `.gitattributes` file in order to set mark `dist/**` as `binary` for git diff and merge purposes. A wildcard is used as a sourcemap file could also be included later if necessary, which should also be considered a "binary" file. * Update `.gitignore` to the latest version from here: `https://github.com/github/gitignore/blob/master/Node.gitignore` * Set the `target` in `tsconfig.json` to `es2017`, which is [supported by current Node.js version - v12][1], as should make for cleaner generated "down-leveled" code as it has native async/await support [1]: https://node.green/

PetarKirov · 2021-02-09T17:59:10Z

I split the deletion of node_modules in its own commit, so now reviewing the PR commit-by-commit should be much easier. I also updated the commit message of commit 5361e1b to better explain what's going on.

And also update the `description` field.

PetarKirov · 2021-02-09T18:43:03Z

I added commit 7ab9fc9 which adds a final step to our CI pipeline which installs Node.js 12, and runs:

set -euxo pipefail
npm ci
npm run build
git diff --stat --exit-code HEAD

The goal is to ensure that no contributor is able to have his pull request merged, unless they correctly update the dist/index.js and package-lock.json files. Also since dist/index.js is essentially a "binary" file as changes to it are almost impossible to review, this also prevents a possible attack vector, where someone checks-in a dist/index.js file containing malicious code.

To verify that this catches modified files I push this commit 76539ab which simply adds a few newlines to the end of the file. As you can see here this discrepancy is correctly caught:

https://github.com/dlang-community/setup-dlang/pull/43/checks?check_run_id=1865501855

This adds an extras 40 seconds to each of our test matrix permutations, but I think it's worth it for the piece of mind ;)

PetarKirov · 2021-02-09T18:46:34Z

95 git hops since git clone according to git reflog | wc -l later I think this PR does everything I wanted it to do ;)

mihails-strasuns

The only improvement that comes to my mind is automatically generating ìndex.js` in the CI upon merge (i.e. using auto-merge label) rather than requiring contributors to do that on their side. This can be done as a separate PR though.

Geod24

Thanks! Only thing missing from my side would be a note for contributors in the README.

PetarKirov · 2021-02-11T14:20:31Z

The only improvement that comes to my mind is automatically generating ìndex.js` in the CI upon merge (i.e. using auto-merge label) rather than requiring contributors to do that on their side.

Yes, that's a good idea, but it would require giving the CI git credentials to push commits to the repo, so I prefer to leave this for a separate PR.

Thanks! Only thing missing from my side would be a note for contributors in the README.

Good point - I will add later today.

Geod24 · 2021-02-11T14:28:31Z

Yes, that's a good idea, but it would require giving the CI git credentials to push commits to the repo, so I prefer to leave this for a separate PR.

It already has it. Look for persist-credentials. But yeah, separate PR LGTM.

Geod24 · 2021-02-11T14:29:26Z

Actually, if we automatize it, there's no point in adding instructions that will be deleted, so let's go ahead with this.

PetarKirov · 2021-02-11T16:52:01Z

Thanks for the review guys!

PetarKirov force-pushed the nuke-node_modules branch from fe7ee3b to 45e2819 Compare February 8, 2021 22:42

PetarKirov force-pushed the nuke-node_modules branch 4 times, most recently from 1a5373a to fc426c1 Compare February 9, 2021 13:05

PetarKirov force-pushed the nuke-node_modules branch 3 times, most recently from 4287ead to e28e1d2 Compare February 9, 2021 14:24

PetarKirov force-pushed the nuke-node_modules branch from e28e1d2 to f1435d2 Compare February 9, 2021 14:49

PetarKirov added 2 commits February 9, 2021 17:43

[.editorconfig]: Enable trim_trailing_whitespace

13de175

[package.json]: Add @vercel/ncc@^0.27.0 dev dependency

3f46347

The following files and folders are intensionally not committed: * `node_modules/.bin/ncc` * `node_modules/@vercel/` Since doing so would be pointless, because the next commit is going to delete them.

PetarKirov force-pushed the nuke-node_modules branch from f1435d2 to 4d4dc25 Compare February 9, 2021 16:01

Geod24 reviewed Feb 9, 2021

View reviewed changes

PetarKirov added 3 commits February 9, 2021 19:54

[node_modules]: Delete the folder from git

eb3e666

[package.json]: Upgrade dependencies to their latest available versions

c331127

PetarKirov force-pushed the nuke-node_modules branch from 4d4dc25 to 360d27f Compare February 9, 2021 17:56

PetarKirov added 2 commits February 9, 2021 20:28

[ci]: Verify that dist/index.js is correctly generated

7ab9fc9

[package.json]: Bump the version to 1.1.0

1dd7ff7

And also update the `description` field.

PetarKirov force-pushed the nuke-node_modules branch from 360d27f to 1dd7ff7 Compare February 9, 2021 18:28

PetarKirov force-pushed the nuke-node_modules branch from 76539ab to 1dd7ff7 Compare February 9, 2021 18:43

PetarKirov added Build Process Changes that affect the build process CI dependencies Pull requests that update a dependency file enhancement New feature or request labels Feb 9, 2021

PetarKirov requested review from Geod24 and mihails-strasuns February 10, 2021 20:10

mihails-strasuns approved these changes Feb 11, 2021

View reviewed changes

Geod24 approved these changes Feb 11, 2021

View reviewed changes

Geod24 merged commit b4b60d7 into dlang-community:v1 Feb 11, 2021

PetarKirov deleted the nuke-node_modules branch February 11, 2021 14:45

PetarKirov mentioned this pull request Feb 17, 2021

binaries: remove generated node_modules binaries and compiled .js files #42

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nuke node modules #43

Nuke node modules #43

PetarKirov commented Feb 8, 2021 •

edited

Loading

Geod24 commented Feb 9, 2021

PetarKirov commented Feb 9, 2021 •

edited

Loading

PetarKirov commented Feb 9, 2021 •

edited

Loading

PetarKirov commented Feb 9, 2021 •

edited

Loading

mihails-strasuns commented Feb 9, 2021 •

edited

Loading

PetarKirov commented Feb 9, 2021 •

edited

Loading

PetarKirov commented Feb 9, 2021 •

edited

Loading

PetarKirov commented Feb 9, 2021

Geod24 Feb 9, 2021

PetarKirov Feb 9, 2021

PetarKirov Feb 9, 2021

PetarKirov Feb 9, 2021

PetarKirov commented Feb 9, 2021

PetarKirov commented Feb 9, 2021

PetarKirov commented Feb 9, 2021 •

edited

Loading

mihails-strasuns left a comment

Geod24 left a comment

PetarKirov commented Feb 11, 2021 •

edited

Loading

Geod24 commented Feb 11, 2021

Geod24 commented Feb 11, 2021

PetarKirov commented Feb 11, 2021

Nuke node modules #43

Nuke node modules #43

Conversation

PetarKirov commented Feb 8, 2021 • edited Loading

Geod24 commented Feb 9, 2021

PetarKirov commented Feb 9, 2021 • edited Loading

PetarKirov commented Feb 9, 2021 • edited Loading

PetarKirov commented Feb 9, 2021 • edited Loading

mihails-strasuns commented Feb 9, 2021 • edited Loading

PetarKirov commented Feb 9, 2021 • edited Loading

PetarKirov commented Feb 9, 2021 • edited Loading

PetarKirov commented Feb 9, 2021

Geod24 Feb 9, 2021

Choose a reason for hiding this comment

PetarKirov Feb 9, 2021

Choose a reason for hiding this comment

PetarKirov Feb 9, 2021

Choose a reason for hiding this comment

PetarKirov Feb 9, 2021

Choose a reason for hiding this comment

PetarKirov commented Feb 9, 2021

PetarKirov commented Feb 9, 2021

PetarKirov commented Feb 9, 2021 • edited Loading

mihails-strasuns left a comment

Choose a reason for hiding this comment

Geod24 left a comment

Choose a reason for hiding this comment

PetarKirov commented Feb 11, 2021 • edited Loading

Geod24 commented Feb 11, 2021

Geod24 commented Feb 11, 2021

PetarKirov commented Feb 11, 2021

PetarKirov commented Feb 8, 2021 •

edited

Loading

PetarKirov commented Feb 9, 2021 •

edited

Loading

PetarKirov commented Feb 9, 2021 •

edited

Loading

PetarKirov commented Feb 9, 2021 •

edited

Loading

mihails-strasuns commented Feb 9, 2021 •

edited

Loading

PetarKirov commented Feb 9, 2021 •

edited

Loading

PetarKirov commented Feb 9, 2021 •

edited

Loading

PetarKirov commented Feb 9, 2021 •

edited

Loading

PetarKirov commented Feb 11, 2021 •

edited

Loading