Skip to content
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

Add notes on contributing, especially w.r.t the parser verification check #92

Open
fwcd opened this issue Jul 15, 2023 · 6 comments
Open
Labels
documentation Improvements or additions to documentation

Comments

@fwcd
Copy link
Owner

fwcd commented Jul 15, 2023

New PRs regularly fail the CI check that verifies that the generated parser matches the committed parser.c, presumably because their development machine didn't generate a byte-for-byte-exact copy of the parser as the GitHub Actions runner.

Perhaps we could add some notes to clarify the motivation behind this check and how to successfully generate a conforming parser.c. Something along the lines of:

  • The check exists since it is infeasible to review a ~20 MB C source file. Suggestions for something less brittle than a byte-for-byte check are of course welcome, however.
  • Unfortunately, the output of tree-sitter generate only seems to be deterministic when run on the same platform. Since the GitHub-hosted runners use x86_64 Linux, contributors have to generate the parser from that platform too (even aarch64 Linux/macOS will produce a different output).
  • One option besides running Linux natively or e.g. through a Docker container would be to use the regenerate-parser.yml workflow in their own fork. This would potentially require customizing the branch it runs on.
@fwcd fwcd added the documentation Improvements or additions to documentation label Jul 15, 2023
@fwcd fwcd pinned this issue Jul 15, 2023
@ketkarameya
Copy link
Contributor

Do we really need parser.c in the repo. The tree-sitter-swift or tree-sitter-java don't have it in the repo either.

https://github.com/alex-pinkus/tree-sitter-swift#frequently-asked-questions
https://github.com/tree-sitter/tree-sitter-java

@fwcd
Copy link
Owner Author

fwcd commented Jul 21, 2023

tree-sitter-java does in fact check it in (see here), but I do like the approach of tree-sitter-swift, that is, omitting the parser and publishing it as a CI artifact. That would probably also help with repo growth, which may become problematic due to the frequent changes in parser.c.

The only reason I am a bit hesitant is that it seems to deviate from the dominant convention of including the parser, even if it's comparatively large. See e.g.

@fwcd
Copy link
Owner Author

fwcd commented Jul 29, 2024

@clason
Copy link

clason commented Jul 29, 2024

...but also include it as a release artefact for semantic versioned(!) releases. (This parser takes a comparatively long time to generate, so just dropping it would not be preferred albeit workable. A separate "release branch", though, is highly discouraged -- some parsers chose to do that in the early days, but it's more headache for everyone than it's worth.)

In either case, please keep committing the generated json, as these are enough to generate the parser without node. (Not that your parser requires npm, as far as I can tell, but it's the principle.)

@fwcd
Copy link
Owner Author

fwcd commented Aug 2, 2024

Yes, to be clear, I don't propose to remove anything for now, just to monitor what upstream does. When the official recommendation changes, we can revisit this.

@clason
Copy link

clason commented Aug 2, 2024

Sure; I was just explaining current upstream policy ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

3 participants