Remove node_modules and compile typescript into minified scripts #2578
Conversation
Before
|
|
Thanks for your contribution. I appreciate the work you've put into this change. Before I can have a deeper look, there are some things I'd like to see:
|
|
jsoref
force-pushed
the
issue-2542
branch
2 times, most recently
from
594f2e3
to
fb48112
Compare
|
That second item is mostly to deal with annoying things like:
Of which there were a lot. Anyway, the commit is currently nowhere. I can make a PR for it, or a simpler one that just upgrades. But the current state is #2046 (comment). I think someone (me?) may have merged and then had the upgrade reverted when someone realized it broke GHES, but it's too late in my day to find that item... (In developing this PR, my goal was 0 errors and 0 warnings before opening it, which took a bit of poking.) |
There was a problem hiding this comment.
This looks reasonable on the surface. I think this could work in general, but I need to discuss more with the rest of the team and try this out in various locations.
- Can you update the
.gitattributesfile to include the new bundled files aslinguist-generated=true? - The
.github/workflows/update-dependencies.ymlworkflow can be removed.
| if command -v npm >/dev/null 2>/dev/null; then | ||
| npm ci | ||
| fi |
There was a problem hiding this comment.
Why is the if necessary? If npm isn't available, how are we going to install dependencies?
There was a problem hiding this comment.
This is for
codeql-action/.github/workflows/__test-proxy.yml
Lines 46 to 52 in 6deb596
because it uses an ubuntu:22.04 container that doesn't have npm:
codeql-action/.github/workflows/__test-proxy.yml
Lines 61 to 62 in 6deb596
But it doesn't need node_modules for the magic that it's doing...
| ( | ||
| echo '*/*-action.js'; | ||
| echo '*/*-action-post.js' | ||
| ) >> .gitignore | ||
| for action in $( | ||
| find * -mindepth 1 -maxdepth 1 -type f -name action.yml | ||
| ); do | ||
| git rm -f "$(dirname "$action")"/*-action*.js | ||
| done |
There was a problem hiding this comment.
Can you explain why this is necessary as a comment?
There was a problem hiding this comment.
I can explain the problem here. I'm not entirely certain what the code is doing/trying to do.
The problem is that because we're bundling all the pieces from node_modules that we actually use as well as any dependency information, there's a segment in each of these files...
that looks like
{ Kie.exports = { name: "codeql", version: "3.27.1", private: !0, description: "CodeQL action", scripts: { build: "tsc --build && npm run package", package: "bash .github/workflows/script/package.sh", test: "ava src/**.test.ts --serial --verbose", "test-debug": "ava src/**.test.ts --serial --verbose --timeout=20m", lint: "eslint --report-unused-disable-directives --max-warnings=0 .", "lint-fix": "eslint --report-unused-disable-directives --max-warnings=0 . --fix", "lint-ci": "SARIF_ESLINT_IGNORE_SUPPRESSED=true eslint --report-unused-disable-directives --max-warnings=0 . --format @microsoft/eslint-formatter-sarif --output-file=eslint.sarif" }, ava: { typescript: { rewritePaths: { "src/": "lib/" }, compile: !1 } }, license: "MIT", dependencies: { "@actions/artifact": "^2.1.9", "@actions/artifact-legacy": "npm:@actions/artifact@^1.1.2", "@actions/cache": "^3.2.4", "@actions/core": "^1.11.1", "@actions/exec": "^1.1.1", "@actions/github": "^5.1.1", "@actions/glob": "^0.4.0", "@actions/io": "^1.1.3", "@actions/tool-cache": "^2.0.1", "@chrisgavin/safe-which": "^1.0.2", "@octokit/plugin-retry": "^5.0.2", "@octokit/types": "^13.6.1", "@schemastore/package": "0.0.10", "@types/node-forge": "^1.3.11", "@types/uuid": "^10.0.0", "adm-zip": "^0.5.16", "check-disk-space": "^3.4.0", "console-log-level": "^1.4.1", del: "^6.1.1", "fast-deep-equal": "^3.1.3", "file-url": "^3.0.0", "follow-redirects": "^1.15.9", fs: "0.0.1-security", "get-folder-size": "^2.0.1", "js-yaml": "^4.1.0", jsonschema: "1.4.1", long: "^5.2.3", "node-forge": "^1.3.1", path: "^0.12.7", semver: "^7.6.3", uuid: "^11.0.1", zlib: "^1.0.5" }, "//": ["micromatch is an unspecified dependency of ava"], devDependencies: { "@ava/typescript": "4.1.0", "@eslint/compat": "^1.1.1", "@eslint/eslintrc": "^3.1.0", "@eslint/js": "^9.13.0", "@microsoft/eslint-formatter-sarif": "^3.1.0", "@types/adm-zip": "^0.5.5", "@types/console-log-level": "^1.4.5", "@types/follow-redirects": "^1.14.4", "@types/get-folder-size": "^2.0.0", "@types/js-yaml": "^4.0.9", "@types/node": "20.9.0", "@types/semver": "^7.5.8", "@types/sinon": "^17.0.3", "@typescript-eslint/eslint-plugin": "^8.11.0", "@typescript-eslint/parser": "^8.11.0", ava: "^5.3.1", esbuild: "^0.24.0", eslint: "^8.57.1", "eslint-import-resolver-typescript": "^3.6.3", "eslint-plugin-filenames": "^1.3.2", "eslint-plugin-github": "^5.0.2", "eslint-plugin-import": "2.29.1", "eslint-plugin-no-async-foreach": "^0.1.1", micromatch: "4.0.8", nock: "^13.5.5", sinon: "^19.0.2", typescript: "^5.6.3" }, overrides: { "@actions/tool-cache": { semver: ">=6.3.1" }, "eslint-plugin-import": { semver: ">=6.3.1" }, "eslint-plugin-jsx-a11y": { semver: ">=6.3.1" } } } })reformatted as almost json
{ name: "codeql", version: "3.27.1", private: !0, description: "CodeQL action", scripts: { build: "tsc --build && npm run package", package: "bash .github/workflows/script/package.sh", test: "ava src/**.test.ts --serial --verbose",
"test-debug": "ava src/**.test.ts --serial --verbose --timeout=20m", lint: "eslint --report-unused-disable-directives --max-warnings=0 .",
"lint-fix": "eslint --report-unused-disable-directives --max-warnings=0 . --fix",
"lint-ci": "SARIF_ESLINT_IGNORE_SUPPRESSED=true eslint --report-unused-disable-directives --max-warnings=0 . --format @microsoft/eslint-formatter-sarif --output-file=eslint.sarif"
}, ava: { typescript: { rewritePaths: {
"src/": "lib/"
}, compile: !1
}
}, license: "MIT", dependencies: {
"@actions/artifact": "^2.1.9",
"@actions/artifact-legacy": "npm:@actions/artifact@^1.1.2",
"@actions/cache": "^3.2.4",
"@actions/core": "^1.11.1",
"@actions/exec": "^1.1.1",
"@actions/github": "^5.1.1",
"@actions/glob": "^0.4.0",
"@actions/io": "^1.1.3",
"@actions/tool-cache": "^2.0.1",
"@chrisgavin/safe-which": "^1.0.2",
"@octokit/plugin-retry": "^5.0.2",
"@octokit/types": "^13.6.1",
"@schemastore/package": "0.0.10",
"@types/node-forge": "^1.3.11",
"@types/uuid": "^10.0.0",
"adm-zip": "^0.5.16",
"check-disk-space": "^3.4.0",
"console-log-level": "^1.4.1", del: "^6.1.1",
"fast-deep-equal": "^3.1.3",
"file-url": "^3.0.0",
"follow-redirects": "^1.15.9", fs: "0.0.1-security",
"get-folder-size": "^2.0.1",
"js-yaml": "^4.1.0", jsonschema: "1.4.1", long: "^5.2.3",
"node-forge": "^1.3.1", path: "^0.12.7", semver: "^7.6.3", uuid: "^11.0.1", zlib: "^1.0.5"
},
"//": [
"micromatch is an unspecified dependency of ava"
], devDependencies: {
"@ava/typescript": "4.1.0",
"@eslint/compat": "^1.1.1",
"@eslint/eslintrc": "^3.1.0",
"@eslint/js": "^9.13.0",
"@microsoft/eslint-formatter-sarif": "^3.1.0",
"@types/adm-zip": "^0.5.5",
"@types/console-log-level": "^1.4.5",
"@types/follow-redirects": "^1.14.4",
"@types/get-folder-size": "^2.0.0",
"@types/js-yaml": "^4.0.9",
"@types/node": "20.9.0",
"@types/semver": "^7.5.8",
"@types/sinon": "^17.0.3",
"@typescript-eslint/eslint-plugin": "^8.11.0",
"@typescript-eslint/parser": "^8.11.0", ava: "^5.3.1", esbuild: "^0.24.0", eslint: "^8.57.1",
"eslint-import-resolver-typescript": "^3.6.3",
"eslint-plugin-filenames": "^1.3.2",
"eslint-plugin-github": "^5.0.2",
"eslint-plugin-import": "2.29.1",
"eslint-plugin-no-async-foreach": "^0.1.1", micromatch: "4.0.8", nock: "^13.5.5", sinon: "^19.0.2", typescript: "^5.6.3"
}, overrides: {
"@actions/tool-cache": { semver: ">=6.3.1"
},
"eslint-plugin-import": { semver: ">=6.3.1"
},
"eslint-plugin-jsx-a11y": { semver: ">=6.3.1"
}
}
}Focussing just on the...
devDependencies
{
"@ava/typescript": "4.1.0",
"@eslint/compat": "^1.1.1",
"@eslint/eslintrc": "^3.1.0",
"@eslint/js": "^9.13.0",
"@microsoft/eslint-formatter-sarif": "^3.1.0",
"@types/adm-zip": "^0.5.5",
"@types/console-log-level": "^1.4.5",
"@types/follow-redirects": "^1.14.4",
"@types/get-folder-size": "^2.0.0",
"@types/js-yaml": "^4.0.9",
"@types/node": "20.9.0",
"@types/semver": "^7.5.8",
"@types/sinon": "^17.0.3",
"@typescript-eslint/eslint-plugin": "^8.11.0",
"@typescript-eslint/parser": "^8.11.0", ava: "^5.3.1", esbuild: "^0.24.0", eslint: "^8.57.1",
"eslint-import-resolver-typescript": "^3.6.3",
"eslint-plugin-filenames": "^1.3.2",
"eslint-plugin-github": "^5.0.2",
"eslint-plugin-import": "2.29.1",
"eslint-plugin-no-async-foreach": "^0.1.1", micromatch: "4.0.8", nock: "^13.5.5", sinon: "^19.0.2", typescript: "^5.6.3"
}... anyway, in there is this field:
"@types/node": "20.9.0",Which will be, by definition, different when we install a different version of @types/node.
Assuming all the code wants to know is "does this stuff compile", then the changes here are the steps required to make it happy. It basically removes the current versions of the files, adds them to ignore (because the check tool uses git porcelain to complain about any unknown or dirty files).
It's possible that this script could be simplified to do less checking, but it required a lot of thought just to understand why it was unhappy and how to make it happy, deciding whether it was still relevant was above my pay grade (you can see the same story with the removeNPMAbsolutePaths thing which I suspected wasn't necessary but wasn't confident about).
Note: the presence of the above blob makes each change annoying (as you will see when I refresh this branch with the location change to package.sh). If it's possible to remove the dependency information without breaking any code or legal requirements, I'd personally be inclined to do so. I'm not sure who's asking for it/why -- perhaps there's a contract that a library be able to inspect itself to determine which versions it thought it asked for? I don't really think that the app needs to know which build scripts were present in package.json.
There was a problem hiding this comment.
- Looking a bit further at this blob, I opened Remove
micromatchfrom package.json #2584 -- that could be included here or later.
|
After you address the merge conflicts then I can run the tests again. Also, in a separate PR, you can upgrade all of the references to |
|
I hadn't thought through It's referenced by: codeql-action/.github/update-release-branch.py Lines 100 to 101 in 3ef4c08 codeql-action/.github/dependabot.yml Lines 9 to 10 in 3ef4c08 codeql-action/.github/workflows/post-release-mergeback.yml Lines 136 to 137 in 3ef4c08 codeql-action/.github/workflows/post-release-mergeback.yml Lines 156 to 163 in 3ef4c08 Thinking about what they're doing also gave me a headache. In the old world, updating dependencies involved committing all of the files in For dependabot, that appeared as #2575 (comment) I think that the same stuff still needs to exist, in that we would want a commit (for at least dependabot) that updates the bundled files. I think what we want to do is to keep the workflow but change its work from committing the node_modules to committing the bundles see f73e1b6. |
|
I've updated |
This reduces the cost to download repository as an action. This commit is logically part of the next commit, but is split to enable the next commit to be reviewed.
jsoref
force-pushed
the
issue-2542
branch
2 times, most recently
from
eddc244
to
08d3519
Compare
Instead of using a bundled node_modules, * Run `npm install` before performing various tasks Change pr-checks to not be particularly picky about the generated content because it will differ between different versions as everything is bundled together.
libandnode_modules*/action.ymlto use local minified bundled files*/*action.js+*/*action-post.jspackage.shwhich is responsible for regenerating the minified files and fixing up the action files.Merge / deployment checklist