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

Fix some issues with building the project on macos #103

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ddickstein
Copy link

Fixes a couple issues I ran into when building the project on macos:

  1. Without the \ in the SONAME_MINOR line, make failed with:
/Library/Developer/CommandLineTools/usr/bin/make -C grammars/ocaml all
../../common/common.mak:39: *** unterminated call to function `shell': missing `)'.  Stop.
make: *** [all] Error 2

with the \, the replacement still seems to work (at least on macos).
2. Depend directly on grammar.js instead of grammar.json, which is itself generated from grammar.js. This ensures that modifications to grammar.js get reflected in the parser without you needing to remember to separately call make generate.

@314eter
Copy link
Collaborator

314eter commented Sep 26, 2024

Since this file is based on the tree-sitter template, it's best to open a PR there first.

Comment on lines +86 to +87
$(PARSER): grammar.js
$(TS) generate --no-bindings
Copy link
Member

@ObserverOfTime ObserverOfTime Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert this. Running make should not require Node.js, which generating from grammar.js does.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ObserverOfTime Isn't it worse to allow grammar.js to fall out of sync than to require node be installed? It's a real dependency of the build process. For what it's worth, I spent a non-trivial amount of time trying to figure out why my changes weren't being taking effect - I generally expect make to build the product from the latest source.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to how cargo build, go build and npm install are compiling/building language bindings, make is compiling the C bindings. They're doing that under the assumption that the parser.c files are already generated, so they don't force npm and tree-sitter dependencies on everyone.

I think this whole rule should be removed, since it introduces a dependency on the tree-sitter executable.

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

Successfully merging this pull request may close these issues.

3 participants