From 220bbffc759e7974a257f09dd3947e49ab1c31ee Mon Sep 17 00:00:00 2001 From: confused-Techie Date: Sat, 23 Sep 2023 17:19:40 -0700 Subject: [PATCH 01/11] Create Workflow --- .../workflows/validate-wasm-grammar-prs.yml | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 .github/workflows/validate-wasm-grammar-prs.yml diff --git a/.github/workflows/validate-wasm-grammar-prs.yml b/.github/workflows/validate-wasm-grammar-prs.yml new file mode 100644 index 0000000000..91d4ad583c --- /dev/null +++ b/.github/workflows/validate-wasm-grammar-prs.yml @@ -0,0 +1,21 @@ +name: Validate WASM Grammar PR Changes +# Since we now want to enforce the rule that any changes to a WASM grammar binary +# file, is accompanied by a change to the `parserSource` key within the +# `grammar.cson` file. This GHA will preform this check for us. + +on: + pull_request: + paths: + - '**.wasm' + +jobs: + validate: + runs-on: ubuntu-latest + steps: + - name: Checkout the Latest Code + uses: actions/checkout@v3 + with: + fetch-depth: 2 + + - name: Run Validation Script + run: node ./script/validate-wasm-grammar-prs.js From ad37d2e0aa30106f3bedae28b0059f986457b691 Mon Sep 17 00:00:00 2001 From: confused-Techie Date: Sat, 23 Sep 2023 17:23:41 -0700 Subject: [PATCH 02/11] Create script to preform checks --- script/validate-wasm-grammar-prs.js | 136 ++++++++++++++++++++++++++++ 1 file changed, 136 insertions(+) create mode 100644 script/validate-wasm-grammar-prs.js diff --git a/script/validate-wasm-grammar-prs.js b/script/validate-wasm-grammar-prs.js new file mode 100644 index 0000000000..ace7ece87c --- /dev/null +++ b/script/validate-wasm-grammar-prs.js @@ -0,0 +1,136 @@ +/* + * This script is called via `validate-wasm-grammar-prs.yml` + * It's purpose is to ensure that everytime a `.wasm` file is changed in a PR + * That the `parserSource` key of the grammar that uses that specific `.wasm` + * file is also updated. + * This way we can ensure that the `parserSource` is always accurate, and is + * never forgotten about. + */ + +const cp = require("node:child_process"); +const path = require("node:path"); +const fs = require("node:fs"); +const CSON = require("season"); + +// Change this if you want more logs +let verbose = false; + +// Lets first find our common ancestor commit +// This lets us determine the commit where the branch or fork departed from +const commonAncestorCmd = cp.spawnSync("git", [ "merge-base", "master", "HEAD" ]); + +if (commonAncestorCmd.status !== 0 || commonAncestorCmd.stderr.toString().length > 0) { + console.error("Git Command has failed!"); + console.error("'git merge-base master HEAD'"); + console.error(commonAncestorCmd.stderr.toString()); + process.exit(1); +} + +const commit = commonAncestorCmd.stdout.toString().trim(); + +if (verbose) { + console.log(`Common Ancestor Commit: '${commit}'`); +} + +const cmd = cp.spawnSync("git", [ "diff", "--name-only", "-r", "HEAD", commit]) + +if (cmd.status !== 0 || cmd.stderr.toString().length > 0) { + console.error("Git Command has failed!"); + console.error(`'git diff --name-only -r HEAD ${commit}'`); + console.error(cmd.stderr.toString()); + process.exit(1); +} + +const changedFiles = cmd.stdout.toString().split("\n"); +// This gives us an array of the name and path of every single changed file from the last two commits +// Now to check if there's any changes we care about. + +const wasmFilesChanged = changedFiles.filter(element => element.endsWith(".wasm")); + +if (wasmFilesChanged.length === 0) { + // No WASM files have been modified. Return success + console.log("No WASM files have been changed."); + process.exit(0); +} + +// Now for every single wasm file that's been changed, we must validate those changes +// are also accompanied by a change in the `parserSource` key + +for (const wasmFile of wasmFilesChanged) { + const wasmPath = path.dirname(wasmFile); + + const files = fs.readdirSync(path.join(wasmPath, "..")); + console.log(`Detected changes to: ${wasmFile}`); + + if (verbose) { + console.log("Verbose file check details:"); + console.log(wasmFile); + console.log(wasmPath); + console.log(files); + console.log("\n"); + } + + for (const file of files) { + const filePath = path.join(wasmPath, "..", file); + console.log(`Checking: ${filePath}`); + + if (fs.lstatSync(filePath).isFile()) { + const contents = CSON.readFileSync(filePath); + + // We now have the contents of one of the grammar files for this specific grammar. + // Since each grammar may contain multiple grammar files, we need to ensure + // that this particular one is using the tree-sitter wasm file that was + // actually changed. + const grammarFile = contents.treeSitter?.grammar ?? ""; + + if (path.basename(grammarFile) === path.basename(wasmFile)) { + // This grammar uses the WASM file that's changed. So we must ensure our key has also changed + // Sidenote we use `basename` here, since the `wasmFile` will be + // a path relative from the root of the repo, meanwhile `grammarFile` + // will be relative from the file itself + + // In order to check the previous state of what the key is, we first must retreive the file prior to this PR + const getPrevFile = cp.spawnSync("git", [ "show", `${commit}:./${filePath}` ]); + + if (getPrevFile.status !== 0 || getPrevFile.stderr.toString().length > 0) { + // This can fail for two major reasons + // 1. It actually failed + // 2. This is a new file, and it failed to find an earlier one that didn't exist + // So that we don't faile brand new TreeSitter grammars, we manually check for number 2 + + if (getPrevFile.stderr.toString().includes("exists on disk, but not in")) { + // Looks like this file is new. Skip this check + continue; + } + + console.error("Git command failed!"); + console.error(`'git show ${commit}:./${filePath}'`); + console.error(getPrevFile.stderr.toString()); + process.exit(1); + } + + fs.writeFileSync(path.join(wasmPath, "..", `OLD-${file}`), getPrevFile.stdout.toString()); + + const oldContents = CSON.readFileSync(path.join(wasmPath, "..", `OLD-${file}`)); + const oldParserSource = oldContents.treeSitter?.parserSource ?? ""; + const newParserSource = contents.treeSitter?.parserSource ?? ""; + + if (newParserSource.length === 0) { + console.error(`Failed to find the new \`parserSource\` within: '${filePath}'`); + console.error(contents.treeSitter); + process.exit(0); + } + + if (oldParserSource == newParserSource) { + // The repo and commit is identical! This means it hasn't been updated + console.error(`The \`parserSource\` key of '${filePath}' has not been updated!`); + console.error(`Current key: ${newParserSource} - Old key: ${oldParserSource}`); + process.exit(0); + } + + // Else it looks like it has been updated properly + console.log(`Validated \`parserSource\` has been updated within '${filePath}' properly.`); + } + } + } +} From bf8075edc582aced0053aed0f1bf279cde07c110 Mon Sep 17 00:00:00 2001 From: confused-Techie Date: Sat, 23 Sep 2023 17:35:45 -0700 Subject: [PATCH 03/11] Setup node, and checkout the entire repo --- .github/workflows/validate-wasm-grammar-prs.yml | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/.github/workflows/validate-wasm-grammar-prs.yml b/.github/workflows/validate-wasm-grammar-prs.yml index 91d4ad583c..ba284df084 100644 --- a/.github/workflows/validate-wasm-grammar-prs.yml +++ b/.github/workflows/validate-wasm-grammar-prs.yml @@ -15,7 +15,16 @@ jobs: - name: Checkout the Latest Code uses: actions/checkout@v3 with: - fetch-depth: 2 + fetch-depth: 0 + # Make sure we get all commits, so that we can compare to early commits + + - name: Setup Node + uses: actions/setup-node@v3 + with: + node-version: 16 + + - name: Install dependencies + run: yarn install - name: Run Validation Script run: node ./script/validate-wasm-grammar-prs.js From b6f056fa5892b0c7f5dd58dfec2b05a9ee57f863 Mon Sep 17 00:00:00 2001 From: confused-Techie Date: Sat, 23 Sep 2023 19:45:48 -0700 Subject: [PATCH 04/11] Ensure we check the right commit --- script/validate-wasm-grammar-prs.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/script/validate-wasm-grammar-prs.js b/script/validate-wasm-grammar-prs.js index ace7ece87c..f487582507 100644 --- a/script/validate-wasm-grammar-prs.js +++ b/script/validate-wasm-grammar-prs.js @@ -17,11 +17,11 @@ let verbose = false; // Lets first find our common ancestor commit // This lets us determine the commit where the branch or fork departed from -const commonAncestorCmd = cp.spawnSync("git", [ "merge-base", "master", "HEAD" ]); +const commonAncestorCmd = cp.spawnSync("git", [ "merge-base", "master", "HEAD^" ]); if (commonAncestorCmd.status !== 0 || commonAncestorCmd.stderr.toString().length > 0) { console.error("Git Command has failed!"); - console.error("'git merge-base master HEAD'"); + console.error("'git merge-base master HEAD^'"); console.error(commonAncestorCmd.stderr.toString()); process.exit(1); } @@ -32,11 +32,11 @@ if (verbose) { console.log(`Common Ancestor Commit: '${commit}'`); } -const cmd = cp.spawnSync("git", [ "diff", "--name-only", "-r", "HEAD", commit]) +const cmd = cp.spawnSync("git", [ "diff", "--name-only", "-r", "HEAD^", commit]) if (cmd.status !== 0 || cmd.stderr.toString().length > 0) { console.error("Git Command has failed!"); - console.error(`'git diff --name-only -r HEAD ${commit}'`); + console.error(`'git diff --name-only -r HEAD^ ${commit}'`); console.error(cmd.stderr.toString()); process.exit(1); } From f57ceb3dd1629a66ef3da1abda41e08257cadda3 Mon Sep 17 00:00:00 2001 From: confused-Techie Date: Sun, 24 Sep 2023 12:00:17 -0700 Subject: [PATCH 05/11] Use `origin/master` instead of `master` --- script/validate-wasm-grammar-prs.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/script/validate-wasm-grammar-prs.js b/script/validate-wasm-grammar-prs.js index f487582507..39589459aa 100644 --- a/script/validate-wasm-grammar-prs.js +++ b/script/validate-wasm-grammar-prs.js @@ -17,7 +17,7 @@ let verbose = false; // Lets first find our common ancestor commit // This lets us determine the commit where the branch or fork departed from -const commonAncestorCmd = cp.spawnSync("git", [ "merge-base", "master", "HEAD^" ]); +const commonAncestorCmd = cp.spawnSync("git", [ "merge-base", "origin/master", "HEAD^" ]); if (commonAncestorCmd.status !== 0 || commonAncestorCmd.stderr.toString().length > 0) { console.error("Git Command has failed!"); From d913e7fa9cd82f57a8b87d3809db333137bc1dd7 Mon Sep 17 00:00:00 2001 From: confused-Techie Date: Sun, 24 Sep 2023 12:01:02 -0700 Subject: [PATCH 06/11] Add new verbose logging --- script/validate-wasm-grammar-prs.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/script/validate-wasm-grammar-prs.js b/script/validate-wasm-grammar-prs.js index 39589459aa..967c806690 100644 --- a/script/validate-wasm-grammar-prs.js +++ b/script/validate-wasm-grammar-prs.js @@ -45,6 +45,11 @@ const changedFiles = cmd.stdout.toString().split("\n"); // This gives us an array of the name and path of every single changed file from the last two commits // Now to check if there's any changes we care about. +if (verbose) { + console.log("Array of changed files between commits:"); + console.log(changedFiles); +} + const wasmFilesChanged = changedFiles.filter(element => element.endsWith(".wasm")); if (wasmFilesChanged.length === 0) { From 1ffda20c43213440430d2c6bdefaf5d94d4e26f8 Mon Sep 17 00:00:00 2001 From: confused-Techie Date: Sun, 24 Sep 2023 12:01:46 -0700 Subject: [PATCH 07/11] Check current head when fidning commits made since common ancestor --- script/validate-wasm-grammar-prs.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/script/validate-wasm-grammar-prs.js b/script/validate-wasm-grammar-prs.js index 967c806690..985c917312 100644 --- a/script/validate-wasm-grammar-prs.js +++ b/script/validate-wasm-grammar-prs.js @@ -21,7 +21,7 @@ const commonAncestorCmd = cp.spawnSync("git", [ "merge-base", "origin/master", " if (commonAncestorCmd.status !== 0 || commonAncestorCmd.stderr.toString().length > 0) { console.error("Git Command has failed!"); - console.error("'git merge-base master HEAD^'"); + console.error("'git merge-base origin/master HEAD^'"); console.error(commonAncestorCmd.stderr.toString()); process.exit(1); } @@ -32,11 +32,11 @@ if (verbose) { console.log(`Common Ancestor Commit: '${commit}'`); } -const cmd = cp.spawnSync("git", [ "diff", "--name-only", "-r", "HEAD^", commit]) +const cmd = cp.spawnSync("git", [ "diff", "--name-only", "-r", "HEAD", commit]) if (cmd.status !== 0 || cmd.stderr.toString().length > 0) { console.error("Git Command has failed!"); - console.error(`'git diff --name-only -r HEAD^ ${commit}'`); + console.error(`'git diff --name-only -r HEAD ${commit}'`); console.error(cmd.stderr.toString()); process.exit(1); } From 5b0093f22916c3de56a917f55f0f9781864b5e78 Mon Sep 17 00:00:00 2001 From: confused-Techie Date: Sun, 24 Sep 2023 12:02:11 -0700 Subject: [PATCH 08/11] Ensure to exit with `1` on fail conditions --- script/validate-wasm-grammar-prs.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/script/validate-wasm-grammar-prs.js b/script/validate-wasm-grammar-prs.js index 985c917312..c56b85c75c 100644 --- a/script/validate-wasm-grammar-prs.js +++ b/script/validate-wasm-grammar-prs.js @@ -123,14 +123,14 @@ for (const wasmFile of wasmFilesChanged) { if (newParserSource.length === 0) { console.error(`Failed to find the new \`parserSource\` within: '${filePath}'`); console.error(contents.treeSitter); - process.exit(0); + process.exit(1); } if (oldParserSource == newParserSource) { // The repo and commit is identical! This means it hasn't been updated console.error(`The \`parserSource\` key of '${filePath}' has not been updated!`); console.error(`Current key: ${newParserSource} - Old key: ${oldParserSource}`); - process.exit(0); + process.exit(1); } // Else it looks like it has been updated properly From 88aaaca0db94d7efc295a94e562d0da3e21cc768 Mon Sep 17 00:00:00 2001 From: confused_techie Date: Tue, 17 Oct 2023 00:37:57 -0700 Subject: [PATCH 09/11] Update script/validate-wasm-grammar-prs.js Co-authored-by: DeeDeeG --- script/validate-wasm-grammar-prs.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/script/validate-wasm-grammar-prs.js b/script/validate-wasm-grammar-prs.js index c56b85c75c..a67302f0bc 100644 --- a/script/validate-wasm-grammar-prs.js +++ b/script/validate-wasm-grammar-prs.js @@ -101,7 +101,7 @@ for (const wasmFile of wasmFilesChanged) { // This can fail for two major reasons // 1. It actually failed // 2. This is a new file, and it failed to find an earlier one that didn't exist - // So that we don't faile brand new TreeSitter grammars, we manually check for number 2 + // So that we don't fail brand new TreeSitter grammars, we manually check for number 2 if (getPrevFile.stderr.toString().includes("exists on disk, but not in")) { // Looks like this file is new. Skip this check From 764d80c6488b1ea74b119eecda4860ebe082fcf2 Mon Sep 17 00:00:00 2001 From: confused-Techie Date: Tue, 17 Oct 2023 00:45:23 -0700 Subject: [PATCH 10/11] Added additional logging, and better comments --- script/validate-wasm-grammar-prs.js | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/script/validate-wasm-grammar-prs.js b/script/validate-wasm-grammar-prs.js index a67302f0bc..6a72103d68 100644 --- a/script/validate-wasm-grammar-prs.js +++ b/script/validate-wasm-grammar-prs.js @@ -13,7 +13,7 @@ const fs = require("node:fs"); const CSON = require("season"); // Change this if you want more logs -let verbose = false; +let verbose = true; // Lets first find our common ancestor commit // This lets us determine the commit where the branch or fork departed from @@ -99,12 +99,15 @@ for (const wasmFile of wasmFilesChanged) { if (getPrevFile.status !== 0 || getPrevFile.stderr.toString().length > 0) { // This can fail for two major reasons - // 1. It actually failed - // 2. This is a new file, and it failed to find an earlier one that didn't exist + // 1. The `git show` command has returned an error code other than `0`, failing. + // 2. This is a new file, and it failed to find an earlier copy (which didn't exist) // So that we don't fail brand new TreeSitter grammars, we manually check for number 2 if (getPrevFile.stderr.toString().includes("exists on disk, but not in")) { // Looks like this file is new. Skip this check + if (verbose) { + console.log("Looks like this file is new. Skipping..."); + } continue; } @@ -135,6 +138,10 @@ for (const wasmFile of wasmFilesChanged) { // Else it looks like it has been updated properly console.log(`Validated \`parserSource\` has been updated within '${filePath}' properly.`); + } else { + if (verbose) { + console.log("This grammar file doesn't use a WASM file that's changed (On the current interation)"); + } } } } From 26707b1f7455516b31c4f00bf7ef86ee2b58ba70 Mon Sep 17 00:00:00 2001 From: confused_techie Date: Sun, 22 Oct 2023 22:01:28 -0700 Subject: [PATCH 11/11] Update script/validate-wasm-grammar-prs.js Co-authored-by: DeeDeeG --- script/validate-wasm-grammar-prs.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/script/validate-wasm-grammar-prs.js b/script/validate-wasm-grammar-prs.js index 6a72103d68..5035475302 100644 --- a/script/validate-wasm-grammar-prs.js +++ b/script/validate-wasm-grammar-prs.js @@ -140,7 +140,7 @@ for (const wasmFile of wasmFilesChanged) { console.log(`Validated \`parserSource\` has been updated within '${filePath}' properly.`); } else { if (verbose) { - console.log("This grammar file doesn't use a WASM file that's changed (On the current interation)"); + console.log("This grammar file doesn't use a WASM file that's changed (On the current iteration)"); } } }