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

add a pkg-config file #128

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

add a pkg-config file #128

wants to merge 2 commits into from

Conversation

selfisekai
Copy link

makes reuse as shared library easier. example generated file:

prefix="/usr/local"
exec_prefix="${prefix}"
libdir="${prefix}/lib"
includedir="${prefix}/include"

Name: base64
Description: Fast Base64 stream encoder/decoder in C99, with SIMD acceleration
Version: 0.5.1
Cflags: -I${includedir}
Libs: -L${libdir} -lbase64

Copy link
Contributor

@BurningEnlightenment BurningEnlightenment left a comment

Choose a reason for hiding this comment

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

Can't your buildsystem consume the CMake configuration directly?

Comment on lines 3 to 4
libdir="${prefix}/lib"
includedir="${prefix}/include"
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't account for user configured CMAKE_INSTALL_INCLUDEDIR, CMAKE_INSTALL_LIBDIR (or CMAKE_INSTALL_BINDIR on Windows) variables.

Description: @CMAKE_PROJECT_DESCRIPTION@
Version: @PROJECT_VERSION@
Cflags: -I${includedir}
Libs: -L${libdir} -l@PROJECT_NAME@
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, this doesn't account for CMAKE_(STATIC|SHARED|IMPORT)_LIBRARY_(PRE|SUF)FIX(_C)?. Please also note that the PROJECT_NAME <=> target name equivalence is rather coincidental.

@selfisekai
Copy link
Author

to my knowledge, it does not (it's gn). electron creates their buildscripts manually https://github.com/electron/electron/blob/95d094d75bddb99c83d2902fbc9a4335632a41cf/patches/node/build_add_gn_build_files.patch#L450

@selfisekai
Copy link
Author

is this correct?

@aklomp
Copy link
Owner

aklomp commented Dec 20, 2023

@BurningEnlightenment Does this look good to you after the fixes? I don't really have enough knowledge about pkg-config configs to come to a conclusion.

In cases like this I'm usually in favour of merging it so it gets traction. Then we can see how the cards shake out (i.e. who starts complaining).

Copy link
Contributor

@BurningEnlightenment BurningEnlightenment left a comment

Choose a reason for hiding this comment

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

Alas, this isn't quite right. CMAKE_IMPORT_LIBRARY_PREFIX_C is only relevant when building shared libraries on Windows (and is not automatically set if the user only supplies CMAKE_IMPORT_LIBRARY_PREFIX). My previous comment meant that you need to respect CMAKE_STATIC_LIBRARY_PREFIX, CMAKE_SHARED_LIBRARY_PREFIX or CMAKE_IMPORT_LIBRARY_PREFIX and their corresponding _C suffix depending on the target platform and the target configuriation, i.e. whether we are building static or shared libraries. So the easiest and least brittle way to do that is by letting CMake do its thing and pluck the "effective" values out of the target properties. I recommend restricting this feature to CMake 3.19+ and use file(GENERATE with target generator expressions. You'll need to either inline the pkg-config template or do a double configuration dance due to the fact that the file(GENERATE INPUT <input-file> form only expands generator expressions but no CMake variables.

Generating correct pkg-config files is rather complicated due to its oversimplified and badly abstracted structure… I'm not really convinced that this will lead to better results than the custom gn buildfiles included in electron/chromium. Wasn't it recommended practice for gn to provide custom buildfiles for 3rdparty dependencies?

@selfisekai
Copy link
Author

not sure how would that work since the same pkg-config files are used whether you add --static or not. i don't see a reason why building with both BUILD_SHARED_LIBS=ON and BUILD_STATIC_LIBS=ON wouldn't be allowed if you want both, and some of our packages do just that (e.g. https://gitlab.alpinelinux.org/alpine/aports/-/blob/f3273b0dc7964024f698aa376736975db0b4ef3c/main/snappy/APKBUILD#L36-37)

Wasn't it recommended practice for gn to provide custom buildfiles for 3rdparty dependencies?

ime that's what gn projects typically do for themselves, as they typically compile everything statically, but when they sometimes don't, they typically go with pkg-config, e.g. https://github.com/flutter/engine/blob/1db92c0ac09b1010535cd0c60f3b661ef203b1cf/shell/platform/linux/config/BUILD.gn, https://source.chromium.org/chromium/chromium/src/+/main:build/linux/unbundle/brotli.gn

@BurningEnlightenment
Copy link
Contributor

i don't see a reason why building with both BUILD_SHARED_LIBS=ON and BUILD_STATIC_LIBS=ON wouldn't be allowed

BUILD_STATIC_LIBS isn't an option natively supported by CMake (see docs and CMake issue). Instead BUILD_SHARED_LIBS switches all libraries whithout an explicit [STATIC | SHARED | MODULE] specification from STATIC to SHARED. CMake doesn't natively support building shared and static variants with a single target and configure. And indeed the alpine package you mentioned patches the snappy buildsystem to introduce BUILD_STATIC_LIBS [1] and a custom snappy_static target [2].

not sure how would that work since the same pkg-config files are used whether you add --static or not.

If you configure the project both times (once for static and once for shared) in a way which generates identical pkg-configs that shouldn't be an issue. And otherwise you've just reached the point where the impedance mismatch between CMake and pkg-config worlds starts to break things.

@selfisekai
Copy link
Author

file(GENERATE does not allow to put $<INSTALL_PREFIX> in the contents, as it would be used before the install phase

@BurningEnlightenment
Copy link
Contributor

file(GENERATE does not allow to put $<INSTALL_PREFIX> in the contents, as it would be used before the install phase

Correct, I see how the wording from my previous comment is a bit unclear with regards to this (double configuration dance).

You'll need to either inline the pkg-config template or do a double configuration dance due to the fact that the file(GENERATE INPUT <input-file> form only expands generator expressions but no CMake variables.

What I meant to say: You will need to do a configure_file() in order to expand ${CMAKE_INSTALL_PREFIX} (and incdir, libdir, version, etc.) whose output file you feed into file(GENERATE INPUT to expand the target specific generator expressions.
Alternatively you can inline the template as a CMake string with file(GENERATE CONTENT where CMake variables are expanded as usual.

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.

None yet

3 participants