Support Swift Package Manager #2786
Conversation
|
Thanks for this contribution! What would be your way to test these bindings? |
I will write some test cases later or use existing test code to build a test module. Once completed, you just need to run the |
Since only the C interface is exposed here (Swift can directly call C), only the C code needs to be tested. For a more complete Swift binding, I would prefer to write another |
Nice! Thanks a lot! Just having some testing is really required. So we have a chance to catch errors in case there are ABI issues. |
I've written some simple Swift test code, and you can now run |
Hi! I have recently updated capstone-swift to work with 5.0.6 as the current implementation was designed for v4 and missed some definitions added in v5. I will be adding support for v6 soon. @Mx-Iris |
|
I suggest that a new CI workflow be added as well, as this might break during our future development. It also helps us to understand how to test it. |
# Conflicts: # .github/workflows/CITest.yml
|
Hope I find time to review it next week. |
There was a problem hiding this comment.
Thanks for your contribution. I never wrote Swift so be so kind and be explicit in your explanations when you answer the comments.
Regarding the tests:
They only print something to the screen. They don't test actual values. In case you want to continue like this we can only merge it into a separated branch. But not into the next branch.
The test cases are not sufficient enough for this.
If you want to have it in the next branch it is required that you implement the equivalent of cstest_py. So it needs to run all test cases in tests/ and compare the values.
That said, the test code looks AI generated to me. Apologies if I am mistaken. But if not not, please don't contribute fully generate code.
| cmake --preset=${{ matrix.config.platform }}-x64 | ||
| cmake --build --preset build-${{ matrix.config.platform }}-release | ||
| cmake --build --preset install-${{ matrix.config.platform }}-release |
There was a problem hiding this comment.
Why those additions to the Windows build?
| cmake --build --preset build-${{ matrix.config.platform }}-release | ||
| cmake --build --preset install-${{ matrix.config.platform }}-release | ||
| macOS: | ||
| name: Execute tests on macOS |
There was a problem hiding this comment.
| name: Execute tests on macOS | |
| name: Execute tests on macOS (Swift) |
| swift test \ | ||
| -c debug |
There was a problem hiding this comment.
| swift test \ | |
| -c debug | |
| swift test -c debug |
| swift test \ | ||
| -c release |
There was a problem hiding this comment.
| swift test \ | |
| -c release | |
| swift test -c release |
| name: "Ccapstone", | ||
| targets: ["Ccapstone"] |
There was a problem hiding this comment.
Why the name Ccapstone is necessary because capstone exists already?
My naive search brought up nothing:
https://swiftpackageindex.com/search?query=capstone
Or is it to not conflict with @santalvarez bindings?
There was a problem hiding this comment.
Why are these scripts necessary?
Should the swift package manager take care of installing the headers correctly?
|
|
||
| guard count > 0 else { | ||
| print("⚠️ No instructions disassembled") | ||
| return |
There was a problem hiding this comment.
Just printing an error in case of failure is not enough. The CI must fail, please add such a mechanism.
There was a problem hiding this comment.
I guess it needs to throw an exception here?
There was a problem hiding this comment.
There are many compilation warnings in this file. Please fix them.
| /// Cross-platform compatibility and edge case tests for the Capstone Swift binding | ||
| /// These tests verify behavior across different platforms and unusual conditions |
There was a problem hiding this comment.
I don't see how this is true. Please elaborate what you wanted to achieve here.
Detailed description
This pull request adds Swift Package Manager support to capstone, allowing capstone to be imported as a dependency in Swift.