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

Use the correct jq package #363

Merged
merged 4 commits into from
Oct 21, 2024
Merged

Use the correct jq package #363

merged 4 commits into from
Oct 21, 2024

Conversation

rougetimelord
Copy link
Collaborator

@rougetimelord rougetimelord commented Oct 14, 2024

jq package is a jQuery wrapper, and we don't use commander at all.

node-jq allows us to use jq inside of node files, and as a side benefit it downloads the jq binary to ./node_modules/node-jq/bin/jq. So we could update the makefile to use that binary, or alternatively turn the functionality of the makefile into a node script?

jq package is a jQuery wrapper, and we don't use commander at all. node-jq allows us to use jq inside of node files, and downloads jq to `./node_modules/node-jq/bin/jq` ;)
cooljeanius
cooljeanius previously approved these changes Oct 14, 2024
@cooljeanius
Copy link
Collaborator

jq package is a jQuery wrapper, and we don't use commander at all.

node-jq allows us to use jq inside of node files, and as a side benefit it downloads the jq binary to ./node_modules/node-jq/bin/jq. So we could update the makefile to use that binary, or alternatively turn the functionality of the makefile into a node script?

Personally I know Makefiles better than I know node, so I'd prefer to keep it as a Makefile instead of turning it into a node script...

@rougetimelord
Copy link
Collaborator Author

Personally I know Makefiles better than I know node, so I'd prefer to keep it as a Makefile instead of turning it into a node script...

Would you be willing to change the makefile so that it uses the downloaded binary?

@cooljeanius
Copy link
Collaborator

Personally I know Makefiles better than I know node, so I'd prefer to keep it as a Makefile instead of turning it into a node script...

Would you be willing to change the makefile so that it uses the downloaded binary?

Well... hm. I know how I'd do this if we were generating the makefile with autotools, but since we're not... I guess PATH modification can be hardcoded directly into the makefile; let me check...

@cooljeanius
Copy link
Collaborator

cooljeanius commented Oct 15, 2024

Personally I know Makefiles better than I know node, so I'd prefer to keep it as a Makefile instead of turning it into a node script...

Would you be willing to change the makefile so that it uses the downloaded binary?

Well... hm. I know how I'd do this if we were generating the makefile with autotools, but since we're not... I guess PATH modification can be hardcoded directly into the makefile; let me check...

@rougetimelord do you want me to do this as part of this PR, or a separate one?

cooljeanius
cooljeanius previously approved these changes Oct 16, 2024
@rougetimelord
Copy link
Collaborator Author

@rougetimelord do you want me to do this as part of this PR, or a separate one?

Yeah let's do it in here

add `./node_modules/node-jq/bin` to `PATH`
(untested)
@cooljeanius
Copy link
Collaborator

cooljeanius commented Oct 17, 2024

Could you give f7c8ebb a try? I'm not quite sure if I did it the right way, or if I need to add an extra $, or put it inside $(shell ...), or add export, or something...

@cooljeanius
Copy link
Collaborator

cooljeanius commented Oct 17, 2024

Could you give f7c8ebb a try? Not quite sure if I did it the right way, or need to add an extra $, or put it inside $(shell ...), or add export, or something...

Ah wait nvm, CI caught it...

try tweaking setting of `PATH` a bit
@cooljeanius
Copy link
Collaborator

OK try now after 2427a42?

@rougetimelord
Copy link
Collaborator Author

I don't have a lot of knowledge around make, does it work locally for you?

@cooljeanius
Copy link
Collaborator

I don't have a lot of knowledge around make, does it work locally for you?

I mean, I have jq installed globally, so that would just get picked up first anyways either way...

@rougetimelord rougetimelord marked this pull request as ready for review October 21, 2024 20:34
@rougetimelord rougetimelord merged commit c7cec32 into main Oct 21, 2024
6 checks passed
@rougetimelord rougetimelord deleted the uninstall-jq branch October 21, 2024 20:42
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.

2 participants