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

Use CMAKE_INSTALL_FULL_LIBDIR, CMAKE_INSTALL_FULL_INCLUDEDIR #1124

Draft
wants to merge 2 commits into
base: c_master
Choose a base branch
from

Conversation

saper
Copy link

@saper saper commented Jun 10, 2024

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

Do not roll our own path contatenation, let's use
GNUInstallDirs macros to do the work.
@saper
Copy link
Author

saper commented Jun 10, 2024

This is a follow-up from the discussion at 4e027b7

  • I don't think that the ability to query pkg-config --variable exec_prefix msgfmt-c and the likes is needed. Anyone needs to use it for anything?
  • 4e027b7 <-- why do we need special handling for macOS here?

saper referenced this pull request Jun 10, 2024
Issue

When `CMAKE_INSTALL_LIBDIR` and `CMAKE_INSTALL_INCLUDEDIR` are set to absolute paths, the `msgpack-c.pc` file generated by CMake improperly configures `libdir` and `includedir`. This leads to incorrect paths that prevent the compiler from locating necessary header and library files.

How to reproduce

Build and install `msgpack-c`.

```console
% cmake -S . -B ../msgpack-c.build -DCMAKE_INSTALL_LIBDIR=/tmp/local/lib -DCMAKE_INSTALL_INCLUDEDIR=/tmp/local/include
% cmake --build ../msgpack-c.build
% sudo cmake --install ../msgpack-c.build
```

Compile `example/simple_c.c` using installed msgpack-c. The following error happens because the linker cannot find paths provided by pkg-config.

```console
% 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)
/usr/bin/ld: cannot find -lmsgpack-c: No such file or directory
collect2: error: ld returned 1 exit status
```

Expected

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

```console
% 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 generated `msgpack-c.pc` file does not handle absolute paths correctly. Here is the result of the incorrect configuration in `How to reproduce` section. In the following `msgpack-c.pc` file, `libdir` and `includedir` are showing unrecognized paths, leading to incorrect paths.

```console
% cat /tmp/local/lib/pkgconfig/msgpack-c.pc
prefix=/usr/local
exec_prefix=/usr/local
libdir=${prefix}//tmp/local/lib <- Here the path is wrong. We expected `/tmp/local/lib`
includedir=${prefix}//tmp/local/include <- Here the path is wrong. We expected `/tmp/local/include`

Name: MessagePack
Description: Binary-based efficient object serialization library
Version: 6.0.1
Libs: -L${libdir} -lmsgpack-c
Cflags: -I${includedir}
```

Solution

Modify the `CMakeLists.txt` file to ensure that `libdir` and `includedir` use absolute paths. This change addresses the issue by providing correct paths to the compiler and linker.
@redboltz
Copy link
Contributor

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

otegami commented Jun 11, 2024

@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 saper marked this pull request as draft June 11, 2024 09:56
@saper
Copy link
Author

saper commented Jun 11, 2024

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

otegami commented Jun 11, 2024

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:

msgpack-c/CMakeLists.txt

Lines 33 to 42 in a5c8a2c

IF (IS_ABSOLUTE ${CMAKE_INSTALL_LIBDIR})
SET (libdir ${CMAKE_INSTALL_LIBDIR})
ELSE ()
SET (libdir "\${prefix}/${CMAKE_INSTALL_LIBDIR}")
ENDIF ()
IF (IS_ABSOLUTE ${CMAKE_INSTALL_INCLUDEDIR})
SET (includedir ${CMAKE_INSTALL_INCLUDEDIR})
ELSE ()
SET (includedir "\${prefix}/${CMAKE_INSTALL_INCLUDEDIR}")
ENDIF ()

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. 🙏

msgpack-c/CMakeLists.txt

Lines 273 to 280 in a5c8a2c

FOREACH (file ${msgpack-c_common_HEADERS})
GET_FILENAME_COMPONENT (dir ${file} PATH)
INSTALL (FILES ${file} DESTINATION ${dir})
ENDFOREACH ()
FOREACH (file ${msgpack-c_configured_HEADERS})
GET_FILENAME_COMPONENT (dir ${file} PATH)
INSTALL (FILES ${CMAKE_CURRENT_BINARY_DIR}/${file} DESTINATION ${dir})
ENDFOREACH ()

@otegami
Copy link

otegami commented Jun 11, 2024

I tried to fix the above problem at this PR #1125.
Could you give me the advice if I miss something? 🙏🏾

@saper
Copy link
Author

saper commented Jun 25, 2024

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

elliejs commented Jul 2, 2024

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

elliejs commented Jul 2, 2024

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)

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.

4 participants