search

msgpack / msgpack-c

MessagePack implementation for C and C++ / msgpack.org[C/C++]
Other
3.01k stars 875 forks source link

Use CMAKE_INSTALL_FULL_LIBDIR, CMAKE_INSTALL_FULL_INCLUDEDIR #1124

Open saper opened 3 months ago

saper commented 3 months ago

Do not roll our own path contatenation, let's use GNUInstallDirs macros to do the work.

saper commented 3 months ago

This is a follow-up from the discussion at https://github.com/msgpack/msgpack-c/commit/4e027b72de6e5a7f216482bdeea1ddabe53474cf

redboltz commented 3 months ago

Thank you for creating the PR. If the PR resolves the issues raised by both @otegami and @saper, I will merge it.

@otegami, could you please check it?

otegami commented 3 months ago

@redboltz Of course, I will check it as much as I can. I couldn't try it but I think we cannot use relative paths for CMAKE_INSTALL_LIBDIR and CMAKE_INSTALL_INCLUDEDIR using pkg-config. May I try it after my working. I will want to check whether it works or not on the following situation.

saper commented 3 months ago

I agree with @otegami concerns.

This patch seems to work fine. but does not account in the possibility to let the users override ${prefix} with pkg-config when integrating with another application.

Can we integrate https://github.com/jtojnar/cmake-snips/blob/master/CMakeScripts/JoinPaths.cmake into this build? (It is MIT-licensed)?

otegami commented 3 months ago

Can we integrate https://github.com/jtojnar/cmake-snips/blob/master/CMakeScripts/JoinPaths.cmake into this build? (It is MIT-licensed)?

I believe I have already done so in the following section:

https://github.com/msgpack/msgpack-c/blob/a5c8a2c845ba43100b7cad7f8a8db0c2ce361d1e/CMakeLists.txt#L33-L42

However, I think the issue lies in the following section. These header files are being installed without respecting our specified CMAKE_INSTALL_INCLUDEDIR. We should fix this. I will check it now as well. 🙏

https://github.com/msgpack/msgpack-c/blob/a5c8a2c845ba43100b7cad7f8a8db0c2ce361d1e/CMakeLists.txt#L273-L280

otegami commented 3 months ago

I tried to fix the above problem at this PR https://github.com/msgpack/msgpack-c/pull/1125. Could you give me the advice if I miss something? 🙏🏾

saper commented 3 months ago

I'll check if the problem is fixed, unless we need to simplify the code (which might be difficult if we want to keep both relative/absolute names and prefix setting in the pkg-config file), we might not need/want this change anymore.

elliejs commented 3 months ago

~I am still encountering https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=279615 while updating my ports tree today (1 July). A package requires msgpack-c be installed, but also requires binutils, meaning the parent package fails to build because it builds msgpack-c and then binutils, which causes binutils to fail with the attached bug.~ ~So, still hitting this bug as recently as today~

I was tracking a branch before this commit

elliejs commented 2 months ago

New knowledge. This bug is not fixed in msgpack_c-6.0.2

When I patch msgpack-c using this patch, i no longer get the bug tracked here: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=279615

I do however still encounter knock on issues. Because of how msgpack-c resolves these paths, compiling binutils leads to the compile command:

cc -DHAVE_CONFIG_H -I.  -I. -I. -I../bfd -I./../bfd -I./../include -DLOCALEDIR="\"/usr/local/share/locale\"" -Dbin_dummy_emulation=bin_vanilla_emulation -isystem /usr/local/include -I/usr/local/include -W -Wall -Wstrict-prototypes -Wmissing-prototypes -Wshadow -I./../zlib -I/usr/local/include -O2 -pipe  -fstack-protector-strong -isystem /usr/local/include -fno-strict-aliasing      -MT readelf.o -MD -MP -MF .deps/readelf.Tpo -I -c -o readelf.o ./readelf.c

The relevant part of this command line is:

-I -c

This empty include path argument consumes the following -c which makes the GNU binutils package fail to build. We are currently investigating this further. My hope is to return with a full understanding of the Makefile template to understand what Include path argument is getting turned into the empty string by msgpack-c (patched with this commit)