Makefile: Ensure goyacc is installed before calling it #16202
Conve 10000 rsation
Makefile
Outdated
| @@ -120,6 +120,9 @@ endif | |||
|
|
|||
| promql/parser/generated_parser.y.go: promql/parser/generated_parser.y | |||
| @echo ">> running goyacc to generate the .go file." | |||
| ifeq (,$(shell command -v goyacc 2> /dev/null)) | |||
| $(MAKE) install-goyacc | |||
There was a problem hiding this comment.
Do you mind doing the same for 115 line while we are here? For consistency, I think it's ok to auto-install on parser command too, no?
Also I don't know why don't rely on Makefile dependency logic (e.g. promql/parser/generated_parser.y.go: promql/parser/generated_parser.y $(goyacc), but it's fine to fix later.
There was a problem hiding this comment.
Yes, I have improved.
7e3ae33 to
4e3b74c
Compare
Signed-off-by: zhengkezhou1 <madzhou1@gmail.com>
4e3b74c to
46e36a7
Compare
| parser: promql/parser/generated_parser.y.go | ||
|
|
||
| promql/parser/generated_parser.y.go: promql/parser/generated_parser.y | ||
| @$(MAKE) install-goyacc | ||
| @echo ">> running goyacc to generate the .go file." | ||
| @$(FIRST_GOPATH)/bin/goyacc -l -o promql/parser/generated_parser.y.go promql/parser/generated_parser.y | ||
|
|
||
| .PHONY: install-goyacc | ||
| install-goyacc: | ||
| @echo ">> installing goyacc $(GOYACC_VERSION)" | ||
| @go install golang.org/x/tools/cmd/goyacc@$(GOYACC_VERSION) | ||
|
|
There was a problem hiding this comment.
This is still not ideal as it always reinstall. This might work (and it's safer as we pin binary version)
| parser: promql/parser/generated_parser.y.go | |
| promql/parser/generated_parser.y.go: promql/parser/generated_parser.y | |
| @$(MAKE) install-goyacc | |
| @echo ">> running goyacc to generate the .go file." | |
| @$(FIRST_GOPATH)/bin/goyacc -l -o promql/parser/generated_parser.y.go promql/parser/generated_parser.y | |
| .PHONY: install-goyacc | |
| install-goyacc: | |
| @echo ">> installing goyacc $(GOYACC_VERSION)" | |
| @go install golang.org/x/tools/cmd/goyacc@$(GOYACC_VERSION) | |
| parser: promql/parser/generated_parser.y.go | |
| GOYACC = $(FIRST_GOPATH)/bin/goyacc-$(GOYACC_VERSION) | |
| $(GOYACC): | |
| @echo ">> installing goyacc $(GOYACC_VERSION)" | 8000|
| @go install golang.org/x/tools/cmd/goyacc@$(GOYACC_VERSION) | |
| @cp $(FIRST_GOPATH)/bin/goyacc $(GOYACC) | |
| @rm $(FIRST_GOPATH)/bin/goyacc | |
| promql/parser/generated_parser.y.go: promql/parser/generated_parser.y $(GOYACC) | |
| @echo ">> running goyacc to generate the .go file." | |
| @$(FIRST_GOPATH)/bin/goyacc -l -o promql/parser/generated_parser.y.go promql/parser/generated_parser.y |
There was a problem hiding this comment.
We shouldn't rm the goyacc binary as this is a shared directory and someone could have manually installed it for another use.
There was a problem hiding this comment.
yes, here is my idea:
-
If goyacc is already installed locally, rename the existing goyacc executable to
goyacc<old-version>, install the desired version of goyacc and rename it togoyacc<old-version>, then restore the original goyacc by renaminggoyacc<old-version>back to goyacc. -
If goyacc is not installed locally, simply install the desired version and rename the executable to
goyacc<version>.
There was a problem hiding this comment.
it would be better to check that the version match what we expect instead of modifying other binaries/directories.
There was a problem hiding this comment.
I've found a new solution: After upgrading to Go 1.24, the go get command has introduced a new flag -tool, which makes managing tools much more convenient now.
Please Take a look: 🚀✨
go get -tool golang.org/x/tools/cmd/goyacc@v0.6.0
go: added golang.org/x/tools v0.6.0
Why is this change needed?
When following the steps to contribute and running make test for the first time, we encounter:
However, there are no instructions indicating that goyacc should be installed before running the tests. Additionally, we can automate the installation of goyacc to simplify the setup process and reduce manual steps.