5.0-rc2 pkg-config includedir regression

[orlitzky]

Hello, in the latest release candidate, I see

$ cat capstone-5.0-rc2/capstone.pc.in
prefix=@CMAKE_INSTALL_PREFIX@
exec_prefix=${prefix}
libdir=${prefix}/@CMAKE_INSTALL_LIBDIR@
includedir=${prefix}/include
...

In 4.x and git head, that includedir contains a /capstone suffix (see #1339 and #1340). I believe it should still have the suffix. Without it, for example, the master branch of the PHP programming language fails to build because it can't find capstone.h.

[XVilka]

As capstone prepares to the 5.0 release, could you please send a PR to the next branch, which is now default?

[arrowd]

I'm confused by this change. The .pc file installed by the capstone-5.0.1 package on FreeBSD has

includedir=/usr/local/include/capstone
Cflags: -I${includedir}

This makes it impossible to use <capstone/capstone.h> form, which seems to be the proper way to include capstone now? I can't build Rizin with system capstone because of this problem.

[orlitzky]

This makes it impossible to use <capstone/capstone.h> form

How so? Is /usr/local/include not in the default include path?

which seems to be the proper way to include capstone now

AFAICS (see my original comment) the absence of /capstone in the pkg-config file was a brief regression that only appeared in the 5.0 release candidates. The suffix was present in 4.x, and also present in git HEAD at that time. And now it's present in 5.0. The suffix had been dropped a few times in the past (see the commit message), but it was always quickly replaced after bug reports started to come in. As a result I think there's a strong argument that #include <capstone.h> is and always was the right way to include the header.

But, #include <capstone/capstone.h> should work too. Because if you install capstone/capstone.h into the system's includedir, then the system will already be looking there, so if you ask it to find capstone/capstone.h then it should. Passing e.g. -I/usr/local/include/capstone only gives it an additional place to look.

I can't build Rizin with system capstone because of this problem.

Which package is this? Can you post the build failure?

The only way I can imagine this happening requires two parts,

1. Rizin updated their #include lines to work (only) with the 5.0 release candidates
2. /usr/local/include isn't being searched for headers

If (2) is normal on FreeBSD, then the best fix would be to revert (1) because otherwise every other project would have to update the path.

[arrowd]

How so? Is /usr/local/include not in the default include path?

That's right. But this also has nothing to do with the problem.

Correct me if I have wrong expectations, but pkg-config --cflags should give me all necessary -I directories to be able to compile the code with a correct #include line. From what I can tell, the project's stance is that <capstone/capstone.h> form should be used by consumers. This form cannot be used with the current .pc file.

Adding -I/usr/local/include is possible, but it is an extra step required to fix the .pc brokenness.

[orlitzky]

How so? Is /usr/local/include not in the default include path?

That's right. But this also has nothing to do with the problem.

Sure, it just helps to understand what's happening.

Correct me if I have wrong expectations, but pkg-config --cflags should give me all necessary -I directories to be able to compile the code with a correct #include line. From what I can tell, the project's stance is that <capstone/capstone.h> form should be used by consumers. This form cannot be used with the current .pc file.

If the project's stance is that <capstone/capstone.h> should be used, then I agree, although that would be a breaking API change. I don't speak for the project, but I investigated this issue when PHP stopped building against capstone, and found evidence that the plain capstone.h form was correct. Namely,

• The /capstone suffix was dropped at least once before and caused similar breakage (capstone.pc file contains broken includedir #1339). In Fix include path in pkg-config for Makefile too (#1339). #1340 the suffix was re-added to address it.
• <capstone.h> was correct in capstone-4.x and in the git repository for capstone-5.0. It was only the release candidates that needed <capstone/capstone.h>.

The goal of this commit was to avoid changing the include path for consumers. Regardless of the project's preference, <capstone.h> was correct in 4.x (and in 5.0-HEAD). My commit made sure that it stayed the same in the 5.0 release candidates to avoid breaking consumers unintentionally.

If the project's stance is now that <capstone/capstone.h> should be used, then you have the moral high ground here, but making that change will break consumers. It can be announced in the changelog etc. and would not be the end of the world but it should be an intentional decision and not something that slips into an RC due to an oversight.