Skip to content
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

Fix pkgconfig file to make #include <capstone/capstone.h> work #2307

Merged
merged 1 commit into from
May 2, 2024

Conversation

ret2libc
Copy link
Contributor

@ret2libc ret2libc commented Apr 4, 2024

Your checklist for this pull request

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

Detailed description

The simple example in https://www.capstone-engine.org/lang_c.html does not work for me because the it can't find capstone/capstone.h .

This is because the pkgconfig file only adds -I{includedir}/capstone and not -I{includedir}.

Test plan

Just try to compile the code in the documentation page.

Closing issues

...

@XVilka
Copy link
Contributor

XVilka commented Apr 5, 2024

Note, that this is controversial change that was done back and forth mutliple times in the past. So it should be checked carefully first why it was done. cc @kabeor @Rot127

@Rot127 Rot127 added the build & packaging Build system and packaging related label Apr 5, 2024
@Rot127
Copy link
Collaborator

Rot127 commented Apr 10, 2024

Please someone correct me if I am wrong. But don't we, without this change, get conflicts with other package's headers of the same name? Basically like #2316?

@aquynh @kabeor Any comments?

@ret2libc
Copy link
Contributor Author

Yes indeed the thing is that "recently" it was made "mandatory" to use #include <capstone/xyz.h> but then the pkgconfig file does not actually produce the right cflags to make that work.

@Rot127
Copy link
Collaborator

Rot127 commented Apr 10, 2024

Any reference to "recently" and the "mandatory" points? Because we should add the explanation to the commit message. So the discussion is decided once and for all :)

@Rot127 Rot127 added this to the v6 milestone Apr 10, 2024
@ret2libc
Copy link
Contributor Author

Any reference to "recently" and the "mandatory" points? Because we should add the explanation to the commit message. So the discussion is decided once and for all :)

I don't have any, sorry :D Just memory, so I could very well be wrong

@Rot127
Copy link
Collaborator

Rot127 commented Apr 10, 2024

I would like to test the collision theory before merging this. So we can say with absolute authority that this is the only way to do it, from now on.
@ret2libc Would you mind doing this?

@grahamwoodward Can you test this one as well for you please?

@ret2libc
Copy link
Contributor Author

I think it's slightly different from the collision you referenced. The pkg-config cflags are used when another package wants to depend on capstone (e.g. Rizin). AFAIR rizin fails to compile now with system capstone.

@grahamwoodward

This comment was marked as off-topic.

@ret2libc

This comment was marked as off-topic.

@Rot127

This comment was marked as off-topic.

@grahamwoodward

This comment was marked as off-topic.

@grahamwoodward

This comment was marked as off-topic.

@aquynh
Copy link
Collaborator

aquynh commented Apr 11, 2024

ok so we just need to update the docs on the website, right?

@ret2libc
Copy link
Contributor Author

ok so we just need to update the docs on the website, right?

No, the doc on the website uses capstone/capstone.h which is correct, but it does not use pkgconfig, so this bug is not hit. However people using libraries usually rely on pkg-config or cmake files to get the right cflags/libs.

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.

I am in favor for this one. Because the header filenames are pretty generic being more explicit from which package they are, is better IMHO.

@XVilka
Copy link
Contributor

XVilka commented Apr 30, 2024

@kabeor @aquynh, what is your opinion on this one? It's better to merge this early to allow a few months of testing on various systems/distributions so it wouldn't make a surprise before the 6.0 release

@kabeor
Copy link
Member

kabeor commented May 2, 2024

Thank you!

@kabeor kabeor merged commit f81eb3a into capstone-engine:next May 2, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build & packaging Build system and packaging related
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants