capstone icon indicating copy to clipboard operation
capstone copied to clipboard

Port OCaml bindings to use Dune and Opam

Open XVilka opened this issue 1 year ago • 9 comments

Your checklist for this pull request

  • [x] I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • [x] I've added tests that prove my fix is effective or that my feature works (if possible)

Detailed description

  • Switch to use what most modern OCaml projects use - Dune build system: https://dune.build/

  • Provide an opam package

  • Based on https://github.com/capstone-engine/capstone/pull/2318

Test plan

  • CI is green
  • opam install .
  • dune test

Closing issues

Closes https://github.com/capstone-engine/capstone/issues/985

XVilka avatar Apr 09 '24 07:04 XVilka

There is still missing proper testing, I think I will just write a proper tests, that will compare the output with the expected, and will add the testing step to the CI.

XVilka avatar Apr 09 '24 07:04 XVilka

There is still missing proper testing, I think I will just write a proper tests, that will compare the output with the expected, and will add the testing step to the CI.

Yes, for now this is enough. But at minimum the build should added in the CI.

Rot127 avatar Apr 10 '24 04:04 Rot127

Hi. Against which version of capstone this binding should be compiled? The OCaml stub does not seem in sync with the Capstone API as found in the branch the OCaml stub belongs to.

strub avatar May 07 '24 12:05 strub

The bindings build correctly with Capstone 5.0.1.

strub avatar May 07 '24 12:05 strub

Hi, it's still in progress. I am targeting 6.0. I still need to implement missing architectures support that were added since the last time OCaml bindings were touched. I also plan to add proper testing with alcotest. Moreover, the auto-sync scripts should be extended to auto-regenerate constants so that they are in sync with the C version. Once everything is done, I also plan to package it for opam and send PR to the opam-repository.

XVilka avatar May 07 '24 12:05 XVilka

if this is working for 5.0.1, please make PR for branch v5, then we can merge it there.

thanks for taking care of this binding!

aquynh avatar Jun 13 '24 00:06 aquynh

if this is working for 5.0.1, please make PR for branch v5, then we can merge it there.

thanks for taking care of this binding!

I don't really want to spend time on 5.0 for these, since I am working on changing the Python code generators to regenerate constants for newly updated architectures - currently there is a mismatch between those.

XVilka avatar Jun 13 '24 01:06 XVilka

Ok, I thought you wanted that since someone mentioned v5.

On Thu, Jun 13, 2024, 09:52 Anton Kochkov @.***> wrote:

if this is working for 5.0.1, please make PR for branch v5, then we can merge it there.

thanks for taking care of this binding!

I don't really want to spend time on 5.0 for these, since I am working on changing the Python code generators to regenerate constants for newly updated architectures - currently there is a mismatch between those.

— Reply to this email directly, view it on GitHub https://github.com/capstone-engine/capstone/pull/2319#issuecomment-2164203817, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABNQNYGQQ57RZNNVMHYB2XDZHD3MPAVCNFSM6AAAAABF6A5U42VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRUGIYDGOBRG4 . You are receiving this because you commented.Message ID: @.***>

aquynh avatar Jun 13 '24 02:06 aquynh

@XVilka When you continue to work on this one, please note the minimal integration_tests every binding must pass. See the suite/cstest/test/integration_tests.py in https://github.com/capstone-engine/capstone/pull/2384.

You just have to make sure the logging messages are the same as the tests expect and they are passed to stdout and stderr.

The tests should be run like the for Python:

cd suite/cstest/test/
python3 ./integration_tests.py "python3 ../../../bindings/python/py_cstest/cstest.py"

Rot127 avatar Aug 16 '24 10:08 Rot127