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 header file installation to respect CMAKE_INSTALL_INCLUDEDIR #1125

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

otegami
Copy link

@otegami otegami commented Jun 11, 2024

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.

@otegami otegami marked this pull request as ready for review June 11, 2024 16:12
Previously, header files were installed uniformly under the include directory, ignoring the specified paths.
This change ensures header files are installed in the appropriate directories under `CMAKE_INSTALL_INCLUDEDIR`.
@otegami otegami force-pushed the fix-ignored-includedir branch from 0073acf to 724d1aa Compare June 12, 2024 12:26
@redboltz
Copy link
Contributor

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
Copy link
Author

otegami commented Jun 21, 2024

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: otegami/groonga#2

@redboltz
Copy link
Contributor

redboltz commented Jun 24, 2024

Let me confirm what you mean.

  1. Fix header file installation to respect CMAKE_INSTALL_INCLUDEDIR #1125 should be merged to cpp_master
  2. Fix header file installation to respect CMAKE_INSTALL_INCLUDEDIR #1125 solves Use CMAKE_INSTALL_FULL_LIBDIR, CMAKE_INSTALL_FULL_INCLUDEDIR #1124 (draft) too. So Use CMAKE_INSTALL_FULL_LIBDIR, CMAKE_INSTALL_FULL_INCLUDEDIR #1124 should be closed (without merge)
    after Fix header file installation to respect CMAKE_INSTALL_INCLUDEDIR #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
Copy link
Contributor

I think that I misunderstand about #1124.
#1128 (comment) 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.

@redboltz redboltz merged commit bf2504e into msgpack:c_master Jun 24, 2024
19 checks passed
@otegami otegami deleted the fix-ignored-includedir branch June 24, 2024 11:45
@otegami
Copy link
Author

otegami commented Jun 24, 2024

@redboltz
Thank you for handling it. My understanding is the same as this comment #1128 (comment) 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants