capstone-engine / capstone

Capstone disassembly/disassembler framework for ARM, ARM64 (ARMv8), Alpha, BPF, Ethereum VM, HPPA, LoongArch, M68K, M680X, Mips, MOS65XX, PPC, RISC-V(rv32G/rv64G), SH, Sparc, SystemZ, TMS320C64X, TriCore, Webassembly, XCore and X86.
http://www.capstone-engine.org
7.6k stars 1.56k forks source link

Include path pollution regression #1807

Open oleavr opened 2 years ago

oleavr commented 2 years ago

I noticed that the .pc include path was changed in 0a39b785d3a65ba183aa15e6611f5ebdceb2c794. This seems like a regression in the sense that it disagrees with the website example here: https://www.capstone-engine.org/lang_c.html

Internally in Capstone though on next, both schemes are used, but capstone/capstone.h dominates:

$ git grep '#include' | grep 'capstone\.h' | grep -v capstone/ | wc -l
3
$ git grep '#include' | grep 'capstone\.h' | grep capstone/ | wc -l 
105

The drawback to adding <install_prefix>/include/capstone to the public include path is that it contains files with very generic names, such as x86.h.

Would it be worth changing this back before releasing v5?

Alternatively, one could also move the architecture headers to a subdirectory, and e.g. #include "arch/x86.h", which would avoid breaking existing code that does #include <capstone.h> instead of #include <capstone/capstone.h> (like in the website example).

aquynh commented 2 years ago

I can revert that commit, but unsure if that would break anything.

aquynh commented 2 years ago

@ael-code i am thinking about reverting that commit #1276 for the next branch. what do you think?

aquynh commented 2 years ago

i reverted that with https://github.com/capstone-engine/capstone/commit/6656bcb63ab4e87dc6079bd6b6b12cc8dd9b2ad8

hamarituc commented 2 years ago

i reverted that with 6656bcb

This change breaks a couple of packages relying on #include <capstone.h> to work, for example

The current behavior seems to be better, because it doesn't pollute the include path. But users of capstone should be informed via a changelog entry to become aware of this incompatibility and to fix their code upstream. Otherwise it would increase the burden of the distro maintainers to fix this issues (see https://bugs.gentoo.org/841716)