Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

Mx-Iris
Copy link

@Mx-Iris Mx-Iris commented Sep 15, 2025

Detailed description

This pull request adds Swift Package Manager support to capstone, allowing capstone to be imported as a dependency in Swift.

@Rot127
Copy link
Collaborator

Rot127 commented Sep 15, 2025

Thanks for this contribution!
I hope I find some time later this week to read into Swift. I never used it.

What would be your way to test these bindings?

@Mx-Iris
Copy link
Author

Mx-Iris commented Sep 15, 2025

Thanks for this contribution! I hope I find some time later this week to read into Swift. I never used it.

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 swift test command.

@Mx-Iris
Copy link
Author

Mx-Iris commented Sep 15, 2025

Thanks for this contribution! I hope I find some time later this week to read into Swift. I never used it.

What would be your way to test these bindings?

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 swift-capstone package (in fact, there is one already, but it only adapts to v5).

@Rot127
Copy link
Collaborator

Rot127 commented Sep 15, 2025

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 swift test command.

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.

@Mx-Iris
Copy link
Author

Mx-Iris commented Sep 15, 2025

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 swift test command.

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 swift test in the project root directory to check it.

@Rot127 Rot127 added the Swift Bindings label Sep 15, 2025
@santalvarez
Copy link

(in fact, there is one already, but it only adapts to v5).

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

@wtdcode
Copy link
Contributor

wtdcode commented Sep 17, 2025

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.

@github-actions github-actions bot added the Github-files Github related files label Sep 21, 2025
@Rot127
Copy link
Collaborator

Rot127 commented Sep 22, 2025

Hope I find time to review it next week.
@santalvarez If you want to, feel free to leave your review here as well.

Copy link
Collaborator

@Rot127 Rot127 left a 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.

Comment on lines +230 to +232
cmake --preset=${{ matrix.config.platform }}-x64
cmake --build --preset build-${{ matrix.config.platform }}-release
cmake --build --preset install-${{ matrix.config.platform }}-release
Copy link
Collaborator

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
name: Execute tests on macOS
name: Execute tests on macOS (Swift)

Comment on lines +251 to +252
swift test \
-c debug
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
swift test \
-c debug
swift test -c debug

Comment on lines +255 to +256
swift test \
-c release
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
swift test \
-c release
swift test -c release

Comment on lines +10 to +11
name: "Ccapstone",
targets: ["Ccapstone"]
Copy link
Collaborator

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?

Copy link
Author

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.

Copy link
Collaborator

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?

Copy link
Collaborator

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
Copy link
Collaborator

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.

Copy link
Collaborator

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?

Copy link
Collaborator

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.

Comment on lines +5 to +6
/// Cross-platform compatibility and edge case tests for the Capstone Swift binding
/// These tests verify behavior across different platforms and unusual conditions
Copy link
Collaborator

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.

@Mx-Iris
Copy link
Author

Mx-Iris commented Oct 7, 2025

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.

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.

@Rot127
Copy link
Collaborator

Rot127 commented Oct 7, 2025

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.

cstest_py doesn't do much. It just parses the Yaml files in tests/ and disassembles the input data with its Python disasm() function. Then compares the output. If the output mismatches it fails.

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.
This is why I personally push for more test coverage where ever possible.

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.

@Mx-Iris
Copy link
Author

Mx-Iris commented Oct 8, 2025

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.

cstest_py doesn't do much. It just parses the Yaml files in tests/ and disassembles the input data with its Python disasm() function. Then compares the output. If the output mismatches it fails.

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. This is why I personally push for more test coverage where ever possible.

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.

@Rot127
Copy link
Collaborator

Rot127 commented Oct 8, 2025

@santalvarez Do you see cooporation potential btw? Since you folks work on the same problem.

@wargio
Copy link
Collaborator

wargio commented Oct 9, 2025

@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?

@Rot127
Copy link
Collaborator

Rot127 commented Oct 9, 2025

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 cstest_swift and a script setting the links.

@santalvarez
Copy link

@santalvarez Do you see cooporation potential btw? Since you folks work on the same problem.

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 import Ccapstone and access everything the C capstone library exposes.

@Mx-Iris
Copy link
Author

Mx-Iris commented Oct 13, 2025

@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 import Ccapstone and access everything the C capstone library exposes.

capstone-swift imports Ccapstone using brew. This PR allows to import it from source, without any other dependencies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Github-files Github related files Swift Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.