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

Moving From YAJL to Jansson #141

Merged
merged 2 commits into from
Nov 6, 2024

Conversation

xw19
Copy link

@xw19 xw19 commented Oct 26, 2024

Moving YAJL to Jansson

Added a temporary function to yajl_val to json_t
converted YAJL_GET_STRING to json_string_value
converted YAJL_IS_NUMBER to json_is_number
added functions from double to various int types.
YAJL_GET_NUMBER returns string. While json_number_value returns double.

Added a temporary function to yajl_val to json_t
converted YAJL_GET_STRING to json_string_value
converted YAJL_IS_NUMBER to json_is_number
added functions from double to various int types.
YAJL_GET_NUMBER returns string. While json_number_value returns double.

Signed-off-by: Sourav Moitra <[email protected]>
@xw19 xw19 force-pushed the switch-to-jansson-SM branch from db89f16 to 78ccdaa Compare October 26, 2024 20:37
@xw19 xw19 changed the title Moving YAJL to Jansson Moving From YAJL to Jansson Oct 26, 2024
@saschagrunert
Copy link
Member

@giuseppe PTAL

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

@giuseppe
Copy link
Member

@saschagrunert are you ok to have this change in the switch-to-jansson branch?

@giuseppe
Copy link
Member

tests are failing

@saschagrunert
Copy link
Member

@xw19 can you please fixup the tests? :)

@xw19
Copy link
Author

xw19 commented Oct 29, 2024

@giuseppe @saschagrunert can we retrigger these test builds?

@xw19
Copy link
Author

xw19 commented Oct 29, 2024

I think the issue is caused by arm32v7 latest ubuntu images release last week https://hub.docker.com/r/arm32v7/ubuntu/tags

There is update on the uraimo/run-on-arch-action tag v2.8.1

uraimo/run-on-arch-action@96c27aa

Which coincides with the exact line failure

https://github.com/containers/libocispec/actions/runs/11534806854/job/32210560862

@xw19 xw19 force-pushed the switch-to-jansson-SM branch from d132a05 to e2d047f Compare October 29, 2024 19:19
@giuseppe
Copy link
Member

it seems there is a missing file:

  ../../jansson/src/jansson.h:15:10: fatal error: jansson_config.h: No such file or directory
     15 | #include "jansson_config.h"

@xw19
Copy link
Author

xw19 commented Oct 30, 2024

I found the cause of the failure was probably because we were not running configure on the jansson hence the jansson_config.h is not being generated.

https://jansson.readthedocs.io/en/latest/gettingstarted.html

(edited: configure is being triggered)

@xw19
Copy link
Author

xw19 commented Oct 30, 2024

WIP: I will first run in my fork and fix then will revert back

@xw19 xw19 changed the title Moving From YAJL to Jansson WIP: Moving From YAJL to Jansson Oct 30, 2024
@xw19 xw19 force-pushed the switch-to-jansson-SM branch 2 times, most recently from 696d678 to 9397552 Compare November 5, 2024 15:55
@xw19 xw19 force-pushed the switch-to-jansson-SM branch from 9397552 to 8588c67 Compare November 5, 2024 16:39
@xw19 xw19 changed the title WIP: Moving From YAJL to Jansson Moving From YAJL to Jansson Nov 5, 2024
@xw19
Copy link
Author

xw19 commented Nov 5, 2024

@saschagrunert @giuseppe Please re-review I think I have fixed the CI issues

https://github.com/xw19/libocispec/actions/runs/11688628819/job/32549520172?pr=1

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

@giuseppe giuseppe merged commit 216f6f7 into containers:switch-to-jansson Nov 6, 2024
6 checks passed
@saschagrunert
Copy link
Member

Refers to #138

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.

3 participants