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 jaq #513

Merged
merged 3 commits into from
Jun 8, 2024
Merged

Add jaq #513

merged 3 commits into from
Jun 8, 2024

Conversation

jayvdb
Copy link
Contributor

@jayvdb jayvdb commented Jun 8, 2024

@jayvdb jayvdb marked this pull request as ready for review June 8, 2024 02:02
Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need a jq clone support in this action?

I get that it is an alternative in Rust and I support it, but given that GHA already have jq and it is enough for most use cases, I'm not sure if this is needed since this action fallbacks to cargo-binstall for rust crates.

Sure it is a bit slower than supporting it here directly but most of the time it is fine and good enough, I don't think we should support everything here.

@NobodyXu NobodyXu mentioned this pull request Jun 8, 2024
Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read through jaq description, it seems that they aim to be more correct (regarding floating point, etc), plus it's written in rust and use serde_json to parse json which provides better memory safety than C parser.

It makes sense for medical software and other area which pay great attention to safety and correctness.

@jayvdb
Copy link
Contributor Author

jayvdb commented Jun 8, 2024

In addition to the general "install-action vs cargo-binstall" aspect addressed with at #514 (comment) , the reason we want to use jaq and xh instead of jq and curl in our workflows is that our build process explicitly use jaq and xh, which are not identical to jq and curl, so that our product builds and V&V processes have as few non-Rust dependencies as possible. Especially with jaq, we've had to adjust the queries so they are supported by jaq.

Each and every non-Rust binary on our build workers needs extra paperwork, including "system" provided tools. And while https://github.com/rustsec/advisory-db tends to be very conservative, that is a good thing in my industry, there isnt an equivalent for C/C++ , so C/C++ binaries are not just extra paperwork - they require active monitoring of system library dependencies, instead of relying on the rustsec community.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Jun 8, 2024

Thanks for explanation, that makes sense.

It seems that the CI has hit rate limit of GitHub, so it's quite slow.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Jun 8, 2024

I have rebased the PR against main to fix the CI due to outdated cargo-binstall

@NobodyXu NobodyXu enabled auto-merge (squash) June 8, 2024 08:06
@NobodyXu NobodyXu merged commit a6bc18a into taiki-e:main Jun 8, 2024
29 checks passed
@jayvdb jayvdb deleted the jaq branch June 9, 2024 23:15
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