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.

Sign up for GitHub

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
Open

Fix some issues with building the project on macos #103

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
97c5457
Fix issue with makefile on macos
ddickstein Sep 26, 2024
95ab02b
Depend on the grammar spec instead of on generated json file to keep …
ddickstein Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions common/common.mak
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ override CFLAGS += -I$(SRC_DIR) -std=c11 -fPIC

# ABI versioning
SONAME_MAJOR := $(word 1,$(subst ., ,$(VERSION)))
SONAME_MINOR := $(shell sed -n 's/#define LANGUAGE_VERSION //p' $(PARSER))
SONAME_MINOR := $(shell sed -n 's/\#define LANGUAGE_VERSION //p' $(PARSER))

# OS-specific bits
ifeq ($(shell uname),Darwin)
Expand Down Expand Up @@ -83,8 +83,8 @@ $(LANGUAGE_NAME).pc: ../../bindings/c/$(LANGUAGE_NAME).pc.in
-e 's|=$(PREFIX)|=$${prefix}|' \
-e 's|@PREFIX@|$(PREFIX)|' $< > $@

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

@ObserverOfTime ObserverOfTime Sep 27, 2024 •
edited
Loading

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.


install: all
install -d '$(DESTDIR)$(INCLUDEDIR)'/tree_sitter '$(DESTDIR)$(PCLIBDIR)' '$(DESTDIR)$(LIBDIR)'
Expand Down