WIP: Pointing libocispec to Jansson branch

[xw19]

Trying out new libocispec with jansson

[kwilczynski]

@xw19, are you aware that we have decided that we are not going to be moving to a different JSON parser?

At least, to the best of my knowledge, we did.

[xw19]

@xw19, are you aware that we have decided that we are not going to be moving to a different JSON parser?

At least, to the best of my knowledge, we did.

No, I am not aware of it. I have been working on the libocispec along with @saschagrunert for last few weeks.

[kwilczynski]

@xw19, are you aware that we have decided that we are not going to be moving to a different JSON parser?
At least, to the best of my knowledge, we did.

No, I am not aware of it. I have been working on the libocispec along with @saschagrunert for last few weeks.

@xw19, see about clarifying the following:

• Replace yajl libocispec#138 (comment)

Perhaps something has changed since then.

[saschagrunert]

@kwilczynski we decided to replace it if the effort is overseeable. @xw19 did a great job over the past weeks working on this topic and I think we can benefit of switching over.

[kwilczynski]

@kwilczynski we decided to replace it if the effort is overseeable. @xw19 did a great job over the past weeks working on this topic and I think we can benefit of switching over.

Do you mean "foreseeable" or "feasible", perhaps? Alas, I wasn't aware, as there wasn't any update on the linked issue about some work being done somewhere. I assumed that the old stance towards the work still stands. Any specific place where this is tracked now?

[saschagrunert]

there wasn't any update on the linked issue about some work being done somewhere.

I was assuming this is enough: containers/libocispec#138 (comment)

Any specific place where this is tracked now?

Yes, still in containers/libocispec#138

[saschagrunert]

@giuseppe do you mind approving this PR be be able to run in CI?

[kwilczynski]

there wasn't any update on the linked issue about some work being done somewhere.

I was assuming this is enough: containers/libocispec#138 (comment)

Right. I have seen no more updates afterwards there, so I assume that nothing has happened since then.

@xw19, thank you for persevering and working on this.

[saschagrunert]

We probably need a similar development branch for this repo like for libocispec.

[giuseppe]

approved!

@xw19 thanks for your hard and important work

@kwilczynski having to statically link to yajl works but it is not ideal. If moving to a different library, that is supported, has no disadvantages then we must consider it.

The few things to consider for comparison that are important for crun:

• no performance lost, we don't really parse much json, but it is important to make sure we don't regress here.
• no memory leaks, even on the error paths, the fuzzing tests can help here.
• no memory usage peaks.

[kwilczynski]

[...]

@kwilczynski having to statically link to yajl works but it is not ideal. If moving to a different library, that is supported, has no disadvantages then we must consider it.

Sure, I simply wasn't sure where we were with it, and given that, I assumed we had given up.

The few things to consider for comparison that are important for crun:

* no performance lost, we don't really parse much json, but it is important to make sure we don't regress here.

* no memory leaks, even on the error paths, the fuzzing tests can help here.

* no memory usage peaks.

@giuseppe, anything from a security point of view to check here? Anything you can think of?

[giuseppe]

@giuseppe, anything from a security point of view to check here? Anything you can think of?

I think the fuzzing tests should cover this part too

[giuseppe]

@xw19 pointing to the jansson branch is enough or do we need to use any configure flag to enable it?

There are also some parts in crun that use directly yajl and we'd need to change them too

[xw19]

pointing to the jansson branch is enough or do we need to use any configure flag to enable it?

I have just started to explore crun. So, will do whatever necessary.

There are also some parts in crun that use directly yajl and we'd need to change them too

Sure, will do thanks @giuseppe

[packit-as-a-service]

Ephemeral COPR build failed. @containers/packit-build please check.

[xw19]

@giuseppe When I update any code the test suite doesn't run without giving explicit approvals. Can you help me by making the test cases run everytime I push the code?

[flouthoc]

@giuseppe When I update any code the test suite doesn't run without giving explicit approvals. Can you help me by making the test cases run everytime I push the code?

I approved it, I think since this is your first PR here that's why github UI is doing this, but I'm sure there must be an option to approve by default for new contributors.

[xw19]

I guess we need to modify pipelines to include jansson. I will do it after I am reasonably sure of my code.

[xw19]

updates: WIP made changes to c files for yajl to jansson. Will continue to work on the same.

[xw19]

Updates: wip able to compile, link started to fix unit tests

[xw19]

Can anyone please approve this so that we can run the tests.

[saschagrunert]

cc @giuseppe ☝️

[xw19]

Updates: Fixed the dependencies and configurations. Resulting in CodeQL and other tests now able to run. Now realizing the breadth and scope of this task its huggggggggggggggge!

[xw19]

Podman e2e test failures are not observed in local.

[xw19]

@mrniranjan Thanks for pointing to right places and helping me to fix the podman test cases.

[xw19]

@giuseppe @saschagrunert . I still need to fix the artificate and few fedora rpm tests. But can you start looking at the code?

[packit-as-a-service]

podman system tests failed. @containers/packit-build please check.

[packit-as-a-service]

Ephemeral COPR build failed. @containers/packit-build please check.

[xw19]

/retest

[packit-as-a-service]

podman system tests failed. @containers/packit-build please check.

[xw19]

@giuseppe @saschagrunert Please help on the artifact job. Apart from that I think I have all the things covered. I would like to work little bit more on the parsing numbers in the next PR.