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

Add dependency check targets to Makefile #901

Closed
wants to merge 3 commits into from
Closed

Add dependency check targets to Makefile #901

wants to merge 3 commits into from

Conversation

Parvezkhan0
Copy link

Fixes #628

@Parvezkhan0 Parvezkhan0 requested a review from a team as a code owner January 16, 2024 06:02
Copy link
Collaborator

@polaroi8d polaroi8d left a 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
Copy link
Collaborator

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."
Copy link
Collaborator

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..."
Copy link
Collaborator

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.

@Parvezkhan0
Copy link
Author

@polaroi8d, I incorporated the changes you advised. Please review my PR again.

Copy link
Collaborator

@polaroi8d polaroi8d left a 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."
Copy link
Collaborator

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: "
Copy link
Collaborator

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."
Copy link
Collaborator

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.

@polaroi8d
Copy link
Collaborator

The PR title is still invalid, eg. you can use: feat: add dependency check to Makefile

@polaroi8d
Copy link
Collaborator

Any update on this @Parvezkhan0? We have to close due to the inactivity.

@polaroi8d
Copy link
Collaborator

Close the PR due the inactivity. Feel free to reopen.

@polaroi8d polaroi8d closed this Feb 1, 2024
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.

Add Makefile: check deps
2 participants