15
8
Fork
You've already forked zig-spec
2

grammar: Handle the peg step in the Makefile properly #61

Open
sagehane wants to merge 1 commit from sagehane/zig-spec:Makefile into master
pull from: sagehane/zig-spec:Makefile
merge into: ziglang:master
ziglang:master
ziglang:sync
ziglang:disorganized-spec-chunks
First-time contributor
Copy link

Resolves the issue described in #2. (Which I also just hit)

Currently, if someone runs make and peg isn't installed, build/parser.c ends up becoming an empty file. This breaks the Makefile logic like so:

$ guix shell clang-toolchain make -- make
peg grammar.peg > build/parser.c
/gnu/store/y9wj7889n87i8pabsgqrrdsx2yip4kyn-bash-minimal-5.2.37/bin/sh: line 1: peg: command not found
make: *** [Makefile:11: build/parser.c] Error 127
$ guix shell clang-toolchain make -- make
cc -O3 -o build/parser main.c
main.c:17:14: error: unknown type name 'yycontext'
 17 | void yyerror(yycontext* ctx, char* message) {
 | ^
main.c:59:5: error: use of undeclared identifier 'yycontext'
 59 | yycontext ctx;
 | ^~~~~~~~~
main.c:60:5: error: call to undeclared library function 'memset' with type 'void *(void *, int, unsigned long)'; ISO C99 and later do
 not support implicit function declarations [-Wimplicit-function-declaration]
 60 | memset(&ctx, 0, sizeof(yycontext));
 | ^
main.c:60:5: note: include the header <string.h> or explicitly provide a declaration for 'memset'
main.c:60:13: error: use of undeclared identifier 'ctx'
 60 | memset(&ctx, 0, sizeof(yycontext));
 | ^~~
main.c:60:28: error: use of undeclared identifier 'yycontext'
 60 | memset(&ctx, 0, sizeof(yycontext));
 | ^~~~~~~~~
main.c:61:9: error: call to undeclared function 'yyparse'; ISO C99 and later do not support implicit function declarations
 [-Wimplicit-function-declaration]
 61 | if (yyparse(&ctx) == 0) {
 | ^
main.c:61:18: error: use of undeclared identifier 'ctx'
 61 | if (yyparse(&ctx) == 0) {
 | ^~~
main.c:62:18: error: use of undeclared identifier 'ctx'
 62 | yyerror(&ctx, "syntax error\n");
 | ^~~
8 errors generated.
make: *** [Makefile:14: build/parser] Error 1

By using -o instead of >, the file won't be created unless peg runs succesfully.

Resolves the issue described in https://codeberg.org/ziglang/zig-spec/issues/2. (Which I also just hit) Currently, if someone runs `make` and `peg` isn't installed, `build/parser.c` ends up becoming an empty file. This breaks the Makefile logic like so: ```sh-session $ guix shell clang-toolchain make -- make peg grammar.peg > build/parser.c /gnu/store/y9wj7889n87i8pabsgqrrdsx2yip4kyn-bash-minimal-5.2.37/bin/sh: line 1: peg: command not found make: *** [Makefile:11: build/parser.c] Error 127 $ guix shell clang-toolchain make -- make cc -O3 -o build/parser main.c main.c:17:14: error: unknown type name 'yycontext' 17 | void yyerror(yycontext* ctx, char* message) { | ^ main.c:59:5: error: use of undeclared identifier 'yycontext' 59 | yycontext ctx; | ^~~~~~~~~ main.c:60:5: error: call to undeclared library function 'memset' with type 'void *(void *, int, unsigned long)'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 60 | memset(&ctx, 0, sizeof(yycontext)); | ^ main.c:60:5: note: include the header <string.h> or explicitly provide a declaration for 'memset' main.c:60:13: error: use of undeclared identifier 'ctx' 60 | memset(&ctx, 0, sizeof(yycontext)); | ^~~ main.c:60:28: error: use of undeclared identifier 'yycontext' 60 | memset(&ctx, 0, sizeof(yycontext)); | ^~~~~~~~~ main.c:61:9: error: call to undeclared function 'yyparse'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 61 | if (yyparse(&ctx) == 0) { | ^ main.c:61:18: error: use of undeclared identifier 'ctx' 61 | if (yyparse(&ctx) == 0) { | ^~~ main.c:62:18: error: use of undeclared identifier 'ctx' 62 | yyerror(&ctx, "syntax error\n"); | ^~~ 8 errors generated. make: *** [Makefile:14: build/parser] Error 1 ``` By using `-o` instead of `>`, the file won't be created unless `peg` runs succesfully.
Member
Copy link

Doesn't work for me:

❯ make
mkdir build
peg grammar.peg -o build/parser.c
-o: No such file or directory
make: *** [Makefile:11: build/parser.c] Error 1
Doesn't work for me: ``` ❯ make mkdir build peg grammar.peg -o build/parser.c -o: No such file or directory make: *** [Makefile:11: build/parser.c] Error 1 ```
Author
First-time contributor
Copy link

Looks like the version I'm using as provided by NixOS is 0.1.20 (a dev branch).

$ peg -V
peg version 0.1.20

Judging by https://piumarta.com/software/peg/peg.1.html, it seems like -ooutput should have been supported by some time around 2013.

Could you see if the following patch works instead? (I swapped the order of $@ and $^ to match the man page and C compiler)

diff --git a/grammar/Makefile b/grammar/Makefile
index e32589c..da1717d 100644
--- a/grammar/Makefile
+++ b/grammar/Makefile
@@ -8,7 +8,7 @@ build:
 mkdir build
 
 build/parser.c: grammar.peg | build
- peg $^ > $@
+ peg -o$@ $^
 
 build/parser: main.c | build/parser.c
 $(CC) -O3 -o $@ $^
$ guix shell clang-toolchain make -- make build/parser.c
mkdir build
peg -obuild/parser.c grammar.peg

Seems to work fine for me.

Looks like the version I'm using as provided by NixOS is 0.1.20 (a dev branch). ```sh-session $ peg -V peg version 0.1.20 ``` Judging by https://piumarta.com/software/peg/peg.1.html, it seems like `-ooutput` should have been supported by some time around 2013. Could you see if the following patch works instead? (I swapped the order of `$@` and `$^` to match the man page and C compiler) ```diff diff --git a/grammar/Makefile b/grammar/Makefile index e32589c..da1717d 100644 --- a/grammar/Makefile +++ b/grammar/Makefile @@ -8,7 +8,7 @@ build: mkdir build build/parser.c: grammar.peg | build - peg $^ > $@ + peg -o$@ $^ build/parser: main.c | build/parser.c $(CC) -O3 -o $@ $^ ``` ```sh-session $ guix shell clang-toolchain make -- make build/parser.c mkdir build peg -obuild/parser.c grammar.peg ``` Seems to work fine for me.
Member
Copy link

Indeed, this works with peg 0.1.18 without the space.

Indeed, this works with peg 0.1.18 without the space.
grammar/Makefile Outdated
@ -12,3 +11,4 @@
peg $^ -o $@
build/parser: main.c | build/parser.c
$(CC) -O3 -o $@ $^
First-time contributor
Copy link

The caching here is wrong I think, editing the peg file does not trigger a rebuild of the binary.

The caching here is wrong I think, editing the peg file does not trigger a rebuild of the binary.
Member
Copy link

Indeed, though I think this patch is still a strict improvement.

Indeed, though I think this patch is still a strict improvement.
Author
First-time contributor
Copy link

I'm no GNU Make expert, but these "order-only" source files after | seem weird in general.

.PHONY: all clean
all: build/parser build/parser_debug
clean:
	rm -rf build
build:
	mkdir build
build/parser.c: grammar.peg | build
	peg -o$@ $^
build/parser: main.c build/parser.c
	$(CC) -O3 -o $@ main.c
build/parser_debug: main.c build/parser.c
	$(CC) -O3 -o $@ main.c -DYY_DEBUG

This would be one suggestion for how to fix this. Using $< instead of $^ to use the first source only is possible, but seemed easier to just specify main.c. If sources are to be split later, one could use something like $(SRC) or whatever to accommodate.

I also changed rm -r build to rm -rf build so that the command doesn't error when calling make clean before the build directory is created.

If this seems fine, I guess I'll force push to this instead.

I'm no GNU Make expert, but these "order-only" source files after `|` seem weird in general. ```Makefile .PHONY: all clean all: build/parser build/parser_debug clean: rm -rf build build: mkdir build build/parser.c: grammar.peg | build peg -o$@ $^ build/parser: main.c build/parser.c $(CC) -O3 -o $@ main.c build/parser_debug: main.c build/parser.c $(CC) -O3 -o $@ main.c -DYY_DEBUG ``` This would be one suggestion for how to fix this. Using `$<` instead of `$^` to use the first source only is possible, but seemed easier to just specify `main.c`. If sources are to be split later, one could use something like `$(SRC)` or whatever to accommodate. I also changed `rm -r build` to `rm -rf build` so that the command doesn't error when calling `make clean` before the `build` directory is created. If this seems fine, I guess I'll force push to this instead.
sagehane marked this conversation as resolved
Author
First-time contributor
Copy link

Since I didn't get a response and I saw no reason not to, I changed the Makefile to the one mentioned in the above conversation.

Since I didn't get a response and I saw no reason not to, I changed the Makefile to the one mentioned in the above conversation.
Author
First-time contributor
Copy link

Just to document some useless GNU Make trivia I just learned of: apparently, one can use $(RM) in Make and it will default to rm -f. Presumably, this is so one can pass a custom remove program instead of the default one by merely setting an envvar or editing the Makefile slightly.

Too bad removing build requires the -r flag, and I feel $(RM) -r defeats the purpose because it would then break if one passes a remove program not supporting said flag. And defining RM = rm -rf sounds very weird.

Just to document some useless GNU Make trivia I just learned of: apparently, one can use `$(RM)` in Make and it will default to `rm -f`. Presumably, this is so one can pass a custom remove program instead of the default one by merely setting an envvar or editing the Makefile slightly. Too bad removing `build` requires the `-r` flag, and I feel `$(RM) -r` defeats the purpose because it would then break if one passes a remove program not supporting said flag. And defining `RM = rm -rf` sounds very weird.
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u Makefile:sagehane-Makefile
git switch sagehane-Makefile
Sign in to join this conversation.
No reviewers
Labels
Clear labels
bounty
https://ziglang.org/news/announcing-donor-bounties
bug
Observed behavior contradicts documented or intended behavior.
contributor-friendly
This issue is limited in scope and/or knowledge of project internals.
downstream
An issue with a third-party project that uses this project.
enhancement
Solving this issue will likely involve adding new logic or components to the codebase.
infra
An issue related to project infrastructure, e.g. continuous integration.
optimization
A task to improve performance and/or resource usage.
question
No questions on the issue tracker; use a community space instead.
regression
Something that used to work in a previous version stopped working
upstream
An issue with a third-party project that this project uses.
use case
Describes a real use case that is difficult or impossible, but does not propose a solution.
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
ziglang/zig-spec!61
Reference in a new issue
ziglang/zig-spec
No description provided.
Delete branch "sagehane/zig-spec:Makefile"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?