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

meson: install cmake dependency files #185

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

neheb
Copy link

@neheb neheb commented Feb 10, 2025

CMake users seem to prefer these to pkgconfig files.

@eli-schwartz how would extra_args be passed as in pkgconfig?

@eli-schwartz
Copy link

@eli-schwartz how would extra_args be passed as in pkgconfig?

My understanding is that with cmake you need a "basic package version" file, which this function you added does, that effectively serves as the "Version: xxx" and "Name: yyy" from a pkg-config file.

You also need a *Targets.cmake file which contains the actual -L / -l and -I arguments, and you have to handwrite that one. So you could include extra args at the same time.

@neheb
Copy link
Author

neheb commented Feb 11, 2025

I made some modifications. I doubt these are correct. I have absolutely no idea how this is supposed to work. conan's version of this package says that

CMake package name(s): inih
CMake target name(s): inih::inih
pkg-config file name(s): INIReader.pc 

is what works but I don't see any modifications to add inih::inih anywhere.

Let's ping some hopefully experts:

ping @aengusjiang @otavio @Ryanf55

@eli-schwartz
Copy link

eli-schwartz commented Feb 11, 2025

If you did this:

pkg_check_modules(INIH IMPORTED_TARGET GLOBAL inih)
pkg_check_modules(INIREADER IMPORTED_TARGET GLOBAL INIReader)

Then pkg_check_modules will execute pkg-config, find those two dependencies, and creates an "interface" library named PkgConfig::INIH or PkgConfig::INIREADER. It has the usual INCLUDE_DIRECTORIES and LINK_LIBRARIES and COMPILE_OPTIONS etc properties.

You probably want to model that somehow, instead of just setting a freestanding CFLAGS variable. People still have to know the name of the produced interface library but at least all the info is in one place.

This is roughly speaking the same experience as a meson dependency() object type, except that you have to guess the name of the produced library instead of having it as a return value. :)

@neheb
Copy link
Author

neheb commented Feb 12, 2025

right. but the cmake interface seems to be inih as used by consumers.

@Ryanf55
Copy link

Ryanf55 commented Feb 13, 2025

This package is so simple, is there a reason not to add a CMakeLists? It wouldn't take that much code at all.

@eli-schwartz
Copy link

I don't see how that would be remotely helpful. Then there would be multiple ways to build inih and neither one would provide everything that downstream users have come to depend on.

Copy link

@Ryanf55 Ryanf55 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Syntax doesn't work as is. Try my PR, which seems to get us 95% of the way there.
neheb#1

There's instructions in the CMakeLists. I added targets for both libraries, and a version file for each.

It currently has an issue in the installed shared library, which you can reproduce on master.

inih library can be linked well, but inih_reader has runtime shared library loading issues.

$ ldd build/ini_reader 
        linux-vdso.so.1 (0x00007ffe7ddd5000)
        libINIReader.so.0 => /home/ryan/Dev/benhoyt/inih/install/lib/x86_64-linux-gnu/libINIReader.so.0 (0x00007f516aad8000)
        libstdc++.so.6 => /lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007f516a889000)
        libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f516a869000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f516a640000)
        libinih.so.0 => not found
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f516a557000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f516aaf3000)

@@ -0,0 +1,4 @@
@PACKAGE_INIT@

set(inih_CFLAGS "${inih_CFLAGS}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
set(inih_CFLAGS "${inih_CFLAGS}"
set(inih_CFLAGS "${inih_CFLAGS}")

Missing close paran.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Does this even work?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, you were missing finding the library and creating the target. See my PR.

CMake users seem to prefer these to pkgconfig files.

Signed-off-by: Rosen Penev <[email protected]>
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.

3 participants