-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Support Swift Package Manager #2786
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
base: next
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name: Execute tests on macOS | |
name: Execute tests on macOS (Swift) |
swift test \ | ||
-c debug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
swift test \ | |
-c debug | |
swift test -c debug |
swift test \ | ||
-c release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
swift test \ | |
-c release | |
swift test -c release |
name: "Ccapstone", | ||
targets: ["Ccapstone"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Swift convention is to name libraries imported from C as C + LibraryName for example, Swift's official example, and then have a pure Swift binding library that drops the C and starts with an uppercase letter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these scripts necessary?
Should the swift package manager take care of installing the headers correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this one
|
||
guard count > 0 else { | ||
print("⚠️ No instructions disassembled") | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it needs to throw an exception here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how this is true. Please elaborate what you wanted to achieve here.
Thank you for your patient review. This PR does not add any new code. If we need to write corresponding Swift tests for every use case, the workload would be very large. I will try to see if I can reuse the cstest_py code. I think it should be fine as long as swift build succeeds. |
I am not familiar with Swift at all (and have limited time to read into it currently). So I can't judge what possibilities of bugs are on the ABI level. Is it just calling the C functions? Are the primitive types guaranteed to have the same bit width? All these things let to bugs with the Python bindings. This is why it is so important to me to have it tested.
I know adding it is not a quick task, and can take two to three days, but with v6 we raised the bar of testing quite significantly. And I really would like to keep it. I consider Capstone infrastructure because it provides a basic function and is used in many big projects. If the others like @kabeor @wargio @wtdcode or @jiegec over vote me it is fine. But I would sleep better if our test coverage grows with each addition. |
Okay, I understand. I will submit a new PR as soon as possible. |
@santalvarez Do you see cooporation potential btw? Since you folks work on the same problem. |
@Rot127 if this pr just adds a bunch of links, why not just keep the script and generate them for the release zip/tarball and keep the sources clean? |
Good point. In this case it would be just an implementation of |
Yeah of course I'd love to cooperate on this. @Mx-Iris I was reviewing this PR and I had a question. What advantage does this provide over the capstone-swift bindings specially considering that by adding capstone-swift to your swift project you can already do |
capstone-swift imports Ccapstone using brew. This PR allows to import it from source, without any other dependencies. |
Detailed description
This pull request adds Swift Package Manager support to capstone, allowing capstone to be imported as a dependency in Swift.