Remove node_modules and compile typescript into minified scripts - #2578
jsoref wants to merge 2 commits into
Conversation
Sorry, something went wrong.
Before github/codeql-action@v3 6624720 (6s)Fri, 01 Nov 2024 13:42:39 GMT Prepare all required actions Fri, 01 Nov 2024 13:42:39 GMT Getting action download info Fri, 01 Nov 2024 13:42:39 GMT Download action repository 'github/codeql-action@v3' (SHA:662472033e021d55d94146f66f6058822b0b39fd) Fri, 01 Nov 2024 13:42:44 GMT Run ./.git/check-spelling-actions/upload-sarif Fri, 01 Nov 2024 13:42:44 GMT Run github/codeql-action/upload-sarif@v3 Fri, 01 Nov 2024 13:42:44 GMT Uploading results Fri, 01 Nov 2024 13:42:44 GMT Processing sarif files: ["/tmp/tmp.yvZ4B9jZAF"] Fri, 01 Nov 2024 13:42:44 GMT Validating /tmp/tmp.yvZ4B9jZAF Fri, 01 Nov 2024 13:42:44 GMT Combining SARIF files using the CodeQL CLI Fri, 01 Nov 2024 13:42:44 GMT Adding fingerprints to SARIF file. See https://docs.github.com/en/enterprise-cloud@latest/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning#providing-data-to-track-code-scanning-alerts-across-runs for more information. Fri, 01 Nov 2024 13:42:44 GMT Uploading results Fri, 01 Nov 2024 13:42:45 GMT Successfully uploaded results After check-spelling-sandbox/github-codeql-action@thin 3dc118c (2s)Tue, 05 Nov 2024 13:22:18 GMT Prepare all required actions Tue, 05 Nov 2024 13:22:18 GMT Getting action download info Tue, 05 Nov 2024 13:22:18 GMT Download action repository 'check-spelling-sandbox/github-codeql-action@thin' (SHA:3dc118c13c28b2804932fce08b479e77e5007079) Tue, 05 Nov 2024 13:22:19 GMT Run ./.git/check-spelling-actions/upload-sarif Tue, 05 Nov 2024 13:22:19 GMT Run check-spelling-sandbox/github-codeql-action/upload-sarif@thin Tue, 05 Nov 2024 13:22:20 GMT Uploading results Tue, 05 Nov 2024 13:22:20 GMT Processing sarif files: ["/tmp/tmp.CoFUHRgCYx"] Tue, 05 Nov 2024 13:22:20 GMT Validating /tmp/tmp.CoFUHRgCYx Tue, 05 Nov 2024 13:22:20 GMT Combining SARIF files using the CodeQL CLI Tue, 05 Nov 2024 13:22:20 GMT Adding fingerprints to SARIF file. See https://docs.github.com/en/enterprise-cloud@latest/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning#providing-data-to-track-code-scanning-alerts-across-runs for more information. Tue, 05 Nov 2024 13:22:20 GMT Uploading results Tue, 05 Nov 2024 13:22:20 GMT Successfully uploaded results |
Sorry, something went wrong.
|
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:
|
Sorry, something went wrong.
|
Sorry, something went wrong.
|
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.) |
Sorry, something went wrong.
aeisenberg
left a comment
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 .gitattributes file to include the new bundled files as linguist-generated=true?
- The .github/workflows/update-dependencies.yml workflow can be removed.
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
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...
Sorry, something went wrong.
| ( | ||
| 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?
Sorry, something went wrong.
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" } } } }){ 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.
Sorry, something went wrong.
There was a problem hiding this comment.
- Looking a bit further at this blob, I opened Remove micromatch from package.json #2584 -- that could be included here or later.
Sorry, something went wrong.
|
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 *-artifact@v3 to @v4 in our workflows. These will need to change before @v3 is removed. |
Sorry, something went wrong.
|
I hadn't thought through .github/workflows/update-dependencies.yml 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 node_modules (and maybe committing lib??). In the new world, if you update package.json/package-lock.json then you need to update all of the actions (as managed by package.sh) which means someone somewhere still needs to do that. 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. |
Sorry, something went wrong.
|
I've updated .gitattributes. |
Sorry, something went wrong.
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.
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.
|
Thanks for your work on this. We've now fixed this issue in a different way. See #3054. Closing this PR now. |
Sorry, something went wrong.
Merge / deployment checklist