-
-
Notifications
You must be signed in to change notification settings - Fork 143
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
[meta] Create Workflow to validate WASM Grammar Changes #740
Conversation
Alright, I've been able to fully test and confirm it's functionality over in #741 So this should be totally good to go |
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.
Seems legit.
I actually read through the validate-wasm-grammar-prs.js
enough that I would be up to speed enough to try and troubleshoot it if anything did go wrong with it in the future.
I think the logging (per the test PR) is a little sparse, maybe be more explicit in the logs about what is going on, but IDK, it didn't take me long to figure out what the logs were about by reading through the code of the script.
So, this is probably an overly thorough set of review comments, but giving the approve since the motivation sounds neat and I can't find anything obviously wrong in the implementation. Not that I spot checked every single bit of it, but looks legit.
Co-authored-by: DeeDeeG <[email protected]>
@DeeDeeG Thanks a ton for taking a look at this one as well! (You were certainly on a roll tonight lol). But I've taken your suggestions and added better comments, as well as added some additional logging. All of which is what you suggested, except I also added some logging for when the WASM file is not being used in the grammar we are checking. So feel free to let me know what you think! |
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.
Approving again, thank you for addressing feedback to make the logging a bit clearer!
(Sorry I took a while to get back to re-reviewing this PR!)
(This approve definitely still stands if the maybe-a-typo (?) code suggestion is applied, or if the typo (?) is otherwise fixed.)
Co-authored-by: DeeDeeG <[email protected]>
@DeeDeeG Thanks as always for approving this one, I'll go ahead and merge |
This was discussed over on #736.
Essentially, we thought it'd be a good idea to start adding
parserSource
as a key to WASM grammars, so that we could always determine the source repository used to generate a WASM binary. Even further, @savetheclocktower and I thought it'd be a good idea to enforce this new standard, and ensure this value is always being updated every time the WASM binary changes. Since it'd be pointless to keep around if we can't 100% rely on it's value.So to that end, we discussed two possibilities:
parserSource
listed, and make sure this new one matches what was put in by maintainers.parserSource
has changed, if the WASM binary has changed.We opted for the second choice, as option 1 would mean having to setup the entire WASM generation toolkit, which can be quite complex.
So that's exactly what this PR does, ensures that if the WASM binary has changed at all, it'll make sure the
parserSource
has also changed.A quick summary of the logic included:
git merge-base master HEAD
to find the common ancestor commit SHA from wherever this branch or fork originated. This is the SHA that will be used as the "before" picture, whereas the current changes will be seen as the "after".git diff --name-only -r HEAD ${SHA}
to check what files have been changed..wasm
files in this list, exiting cleanly if none are found.treeSitter.grammar
key of a checked grammar file uses the same file that has changedgit show ${SHA}:./${file}
to get the contents of the file before the branch or fork was created, saving this content to disk.treeSitter.parserSource
key between these two files to know if they have changed.