msgpack / msgpack-c

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

Fix header file installation to respect `CMAKE_INSTALL_INCLUDEDIR` #1125

Closed otegami closed 4 months ago

otegami commented 5 months ago

When CMAKE_INSTALL_INCLUDEDIR are set to absolute path or relative path, the header files is installed in unspecifed places. This leads to incorrect paths that prevent the compiler from locating necessary header files.

How to reproduce

Configure msgpack-c with -DCMAKE_INSTALL_PREFIX=/tmp/local and -DCMAKE_INSTALL_INCLUDEDIR=specified-include And then build and install msgpack-c.

$ cmake -S . -B ../msgpack-c.build -DCMAKE_INSTALL_PREFIX=/tmp/local -DCMAKE_INSTALL_INCLUDEDIR=specified-include
$ cmake --build ../msgpack-c.build
$ cmake --install ../msgpack-c.build
-- Install configuration: ""
-- Installing: /tmp/local/lib/libmsgpack-c.so.2.0.0
-- Installing: /tmp/local/lib/libmsgpack-c.so.2
-- Installing: /tmp/local/lib/libmsgpack-c.so
-- Installing: /tmp/local/lib/libmsgpack-c.a
-- Installing: /tmp/local/include/msgpack.h
-- Installing: /tmp/local/include/msgpack/fbuffer.h
-- Installing: /tmp/local/include/msgpack/gcc_atomic.h
-- Installing: /tmp/local/include/msgpack/object.h
-- Installing: /tmp/local/include/msgpack/pack.h
-- Installing: /tmp/local/include/msgpack/pack_define.h
-- Installing: /tmp/local/include/msgpack/sbuffer.h
-- Installing: /tmp/local/include/msgpack/timestamp.h
-- Installing: /tmp/local/include/msgpack/unpack.h
-- Installing: /tmp/local/include/msgpack/unpack_define.h
-- Installing: /tmp/local/include/msgpack/unpack_template.h
-- Installing: /tmp/local/include/msgpack/util.h
-- Installing: /tmp/local/include/msgpack/version.h
-- Installing: /tmp/local/include/msgpack/version_master.h
-- Installing: /tmp/local/include/msgpack/vrefbuffer.h
-- Installing: /tmp/local/include/msgpack/zbuffer.h
-- Installing: /tmp/local/include/msgpack/zone.h
-- Installing: /tmp/local/include/msgpack/pack_template.h
-- Installing: /tmp/local/include/msgpack/sysdep.h
-- Installing: /tmp/local/lib/pkgconfig/msgpack-c.pc
-- Installing: /tmp/local/lib/cmake/msgpack-c/msgpack-c-targets.cmake
-- Installing: /tmp/local/lib/cmake/msgpack-c/msgpack-c-targets-noconfig.cmake
-- Installing: /tmp/local/lib/cmake/msgpack-c/msgpack-c-config.cmake
-- Installing: /tmp/local/lib/cmake/msgpack-c/msgpack-c-config-version.cmake

Compile example/simple_c.c using installed msgpack-c. The following error happens because the linker cannot find header files.

$ export PKG_CONFIG_PATH=/tmp/local/lib/pkgconfig:$PKG_CONFIG_PATH
$ gcc -o simple_c example/simple_c.c $(pkg-config --cflags --libs msgpack-c)
example/simple_c.c:1:10: fatal error: msgpack.h: No such file or directory
    1 | #include <msgpack.h>
      |          ^~~~~~~~~~~
compilation terminated.

Expected

Successfully compile example/simple_c.c using installed msgpack-c. We can execute simple_c like the following.

$ gcc -o simple_c example/simple_c.c $(pkg-config --cflags --libs msgpack-c)
$ ./simple_c
93 01 c3 a7 65 78 61 6d 70 6c 65
[1, true, "example"]

Explain the problem in detail

The issue was caused by header files being installed under a uniform include directory, ignoring the specified CMAKE_INSTALL_INCLUDEDIR path. This resulted in incorrect paths during the installation process, causing the compiler to be unable to locate the necessary header files.

For example, in the How to reproduce case, we expected that header files would be installed at /tmp/local/specified-include because we passed -DCMAKE_INSTALL_PREFIX=/tmp/local and -DCMAKE_INSTALL_INCLUDEDIR=specified-include. And also, pkg-config expected this path too.

$ pkg-config --cflags --libs msgpack-c
-I/tmp/local/specified-include -L/tmp/local/lib -lmsgpack-c

However, these header files are installed under /tmp/local/include/, which is why the compiler cannot find the header files.

-- Installing: /tmp/local/include/msgpack.h
-- Installing: /tmp/local/include/msgpack/fbuffer.h
-- Installing: /tmp/local/include/msgpack/gcc_atomic.h
-- Installing: /tmp/local/include/msgpack/object.h
-- Installing: /tmp/local/include/msgpack/pack.h
-- Installing: /tmp/local/include/msgpack/pack_define.h
-- Installing: /tmp/local/include/msgpack/sbuffer.h
-- Installing: /tmp/local/include/msgpack/timestamp.h
-- Installing: /tmp/local/include/msgpack/unpack.h
-- Installing: /tmp/local/include/msgpack/unpack_define.h
-- Installing: /tmp/local/include/msgpack/unpack_template.h
-- Installing: /tmp/local/include/msgpack/util.h
-- Installing: /tmp/local/include/msgpack/version.h
-- Installing: /tmp/local/include/msgpack/version_master.h
-- Installing: /tmp/local/include/msgpack/vrefbuffer.h
-- Installing: /tmp/local/include/msgpack/zbuffer.h
-- Installing: /tmp/local/include/msgpack/zone.h
-- Installing: /tmp/local/include/msgpack/pack_template.h
-- Installing: /tmp/local/include/msgpack/sysdep.h

Solution

The solution involves modifying the CMakeLists.txt to ensure that the header files are installed in the directories specified by CMAKE_INSTALL_INCLUDEDIR.

redboltz commented 5 months ago

I haven't been able to check msgpack-c for a while due to a family bereavement.

Is this PR solve #1124 ? I mean if I merge this PR, both @saper and @otegami issue would be solved?

otegami commented 4 months ago

Thank you for addressing it every time. I tried to use this change in my project. It solves my issue although it's a little hard for you to check it. I'm so sorry. ref: https://github.com/otegami/groonga/pull/2

redboltz commented 4 months ago

Let me confirm what you mean.

  1. 1125 should be merged to cpp_master

  2. 1125 solves #1124 (draft) too. So #1124 should be closed (without merge)

    after #1125 is merged. (As far as you've checked)

Is that right?

It seems that github automatically edit the link on this comment.

Here is non linked the same text:

1.  #1125 should be merged to cpp_master
2.  #1125 solves #1124 (draft) too. So #1124 should be closed (without merge) 
    after #1125 is merged. (As far as you've checked)
redboltz commented 4 months ago

I think that I misunderstand about #1124. https://github.com/msgpack/msgpack-c/issues/1128#issuecomment-2185686067 helps my understanding.

1124 is more fundamental approach.

1125 solves different issue. (Perhaps #1124 solves #1125 too)

My decision

I will merge #1125 I keep #1124 as draft (not close)

Then, I will release the merged c_master as c-6.0.2.

1128 requests the new release.

If more fundamental solution is implemented (based on #1124), I will check it and merge it if it is better solution in the future. I would request help @otegami and @fumoboy007 at that time.

otegami commented 4 months ago

@redboltz Thank you for handling it. My understanding is the same as this comment https://github.com/msgpack/msgpack-c/issues/1128#issuecomment-2185686067 too. I'm so sorry my explanation wasn't clear enough mm

I would request help @otegami and @fumoboy007 at that time.

Sure. I think the current change in #1124 is a little bit. So I think we can handle it step by step to improve maintainability.