-
Notifications
You must be signed in to change notification settings - Fork 329
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
Remove node_modules and compile typescript into minified scripts #2578
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,19 +26,23 @@ jobs: | |
- name: Checkout | ||
uses: actions/checkout@v4 | ||
|
||
- name: Install | ||
shell: bash | ||
run: npm install | ||
|
||
- name: Lint | ||
id: lint | ||
run: npm run-script lint-ci | ||
|
||
- name: Upload sarif | ||
uses: github/codeql-action/upload-sarif@v3 | ||
# Only upload SARIF for the latest version of Node.js | ||
if: "!cancelled() && matrix.node-types-version == 'current' && !startsWith(github.head_ref, 'dependabot/')" | ||
if: ${{ !cancelled() && matrix.node-types-version == 'current' && !startsWith(github.head_ref, 'dependabot/') }} | ||
with: | ||
sarif_file: eslint.sarif | ||
category: eslint | ||
|
||
- name: Update version of @types/node | ||
- name: Override version of @types/node | ||
if: matrix.node-types-version != 'current' | ||
env: | ||
NODE_TYPES_VERSION: ${{ matrix.node-types-version }} | ||
|
@@ -52,6 +56,25 @@ jobs: | |
# `npm install` on Linux. | ||
npm install | ||
|
||
# esbuild embeds package.json version details into these files. | ||
# Since the jq step has actively changed package.json, we know that if these files | ||
# are successfully rebuilt (without the changes below), they would be dirty. | ||
# | ||
# In order to allow check-js.sh to verify that it can build them at all, we ignore them, | ||
# delete them, and commit those changes. Thus, when it runs, it will be able to try to | ||
# build them, and as long at they build, it will be happy. If it can't build them, it can | ||
# complain, although that error won't make much sense, because you shouldn't update them | ||
# using the wrong node types version information. | ||
( | ||
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 | ||
Comment on lines
+68
to
+76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain why this is necessary as a comment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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 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 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
if [ ! -z "$(git status --porcelain)" ]; then | ||
git config --global user.email "[email protected]" | ||
git config --global user.name "github-actions[bot]" | ||
|
@@ -63,17 +86,6 @@ jobs: | |
- name: Check generated JS | ||
run: .github/workflows/script/check-js.sh | ||
|
||
check-node-modules: | ||
if: github.event_name != 'push' || github.ref == 'refs/heads/main' || startsWith(github.ref, 'refs/heads/releases/v') | ||
name: Check modules up to date | ||
runs-on: macos-latest | ||
timeout-minutes: 45 | ||
|
||
steps: | ||
- uses: actions/checkout@v4 | ||
- name: Check node modules up to date | ||
run: .github/workflows/script/check-node-modules.sh | ||
|
||
check-file-contents: | ||
if: github.event_name != 'push' || github.ref == 'refs/heads/main' || startsWith(github.ref, 'refs/heads/releases/v') | ||
name: Check file contents | ||
|
@@ -102,7 +114,7 @@ jobs: | |
npm-test: | ||
if: github.event_name != 'push' || github.ref == 'refs/heads/main' || startsWith(github.ref, 'refs/heads/releases/v') | ||
name: Unit Test | ||
needs: [check-js, check-node-modules] | ||
needs: check-js | ||
strategy: | ||
fail-fast: false | ||
matrix: | ||
|
@@ -112,6 +124,9 @@ jobs: | |
|
||
steps: | ||
- uses: actions/checkout@v4 | ||
- name: Build | ||
run: | | ||
npm run build | ||
- name: npm test | ||
run: | | ||
# Run any commands referenced in package.json using Bash, otherwise | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
#!/bin/sh | ||
bundle_file() { | ||
module=$(dirname "$1") | ||
file=$(perl -ne 'next unless m<'"$2"': .(?:.*/|)(.*\.js)>;print $1' "$1") | ||
if [ -n "$file" ]; then | ||
if [ "$2" = main ]; then | ||
suffix='' | ||
else | ||
suffix="-$2" | ||
fi | ||
./node_modules/.bin/esbuild "lib/$module-action$suffix.js" --bundle --minify --platform=node --outfile="./$module/$file" | ||
perl -pi -e 's/scripts:\{.*?\}/scripts:{}/' "./$module/$file" | ||
fi | ||
}; | ||
for a in */action.yml; do | ||
bundle_file $a main; | ||
bundle_file $a post; | ||
done |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the
if
necessary? If npm isn't available, how are we going to install dependencies?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...