-
-
Notifications
You must be signed in to change notification settings - Fork 87
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 dependency check targets to Makefile #901
Add dependency check targets to Makefile #901
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The direction is good, but we just need simple functions to return a message about which dependencies are missing. Regarding your solution, there is no dependency check in Node.js. It would be nice if you could implement a general check that calls all the dependencies, so users don't need to run each command one by one.
|
||
# Compile the all gRPC files | ||
.PHONY: protogen | ||
protogen:| proto-agent proto-crux |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing new line end of the file.
Makefile
Outdated
|
||
.PHONY: check-dependencies | ||
check-dependencies: check-golang-dependencies check-nodejs-dependencies | ||
@echo "All dependencies are up to date." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not checking the dependencies, this is just a simple echo. Please remove it, implement it or add a TODO comment to implement in later.
|
||
.PHONY: check-nodejs-dependencies | ||
check-nodejs-dependencies: | ||
@echo "Checking Node.js dependencies..." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function does not check the dependencies.
@polaroi8d, I incorporated the changes you advised. Please review my PR again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still a bunch of errors and problems. Please take your time to think over the task and find the best possible solution.
...And the PR title is not valid. Please check the CONTRIBUTING.md for how we format PRs; you missed specifying the scope and type of the contribution.
|
||
.PHONY: check-nodejs-dependencies | ||
check-nodejs-dependencies: | ||
@echo "Checking Node.js dependencies..." | ||
@npm install | ||
@npm install 2>&1 | grep "npm ERR!" && (echo "Node.js dependencies are missing" && exit 1) || echo "Node.js dependencies are up to date." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I have an outdated npm version, it will still indicate that my Node.js version is up-to-date, but this is not true at that moment."
.PHONY: check-golang-dependencies | ||
check-golang-dependencies: | ||
@echo "Checking Golang dependencies..." | ||
@go list -f '{{if not .Indirect}}{{.}}{{end}}' ./... | xargs go list -f '{{if not .Standard}}{{.ImportPath}}{{end}}' | xargs go get -u | ||
@go list -f '{{if not .Indirect}}{{.}}{{end}}' ./... | xargs go list -f '{{if not .Standard}}{{.ImportPath}}{{end}}' | xargs go get -u 2>&1 | grep "go: " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got the following error message when try out this command:
Checking Golang dependencies...
template: main:1:9: executing "main" at <.Indirect>: can't evaluate field Indirect in type *load.PackagePublic
make: *** [check-golang-dependencies] Error 1
|
||
.PHONY: check-nodejs-dependencies | ||
check-nodejs-dependencies: | ||
@echo "Checking Node.js dependencies..." | ||
@npm install | ||
@npm install 2>&1 | grep "npm ERR!" && (echo "Node.js dependencies are missing" && exit 1) || echo "Node.js dependencies are up to date." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got the following error:
Checking Node.js dependencies...
npm ERR! code ENOENT
npm ERR! syscall open
npm ERR! path /xxx/.../package.json
npm ERR! errno -2
npm ERR! enoent ENOENT: no such file or directory, open '/xxx/.../package.json'
npm ERR! enoent This is related to npm not being able to find a file.
npm ERR! enoent
npm ERR! A complete log of this run can be found in: /xxx/.../.npm/_logs/2024-01-17T10_08_11_168Z-debug-0.log
Node.js dependencies are missing
Node.js dependencies are up to date.
The PR title is still invalid, eg. you can use: |
Any update on this @Parvezkhan0? We have to close due to the inactivity. |
Close the PR due the inactivity. Feel free to reopen. |
Fixes #628