fix(cmake): improve shared libs and pkg config files #1842

Apteryks · 2024-05-06T18:45:31Z

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area build

/area libscap-engine-gvisor

/area libpman

/area libsinsp

What this PR does / why we need it:

It sanitizes the generated pkg-config files of libscap.pc and libsinsp.pc, and add missing includes needed to build as shared libraries.

Which issue(s) this PR fixes:

Fixes #1820

Special notes for your reviewer:

Also see #1825, which is not addressed by this PR but can be easily worked around.

Does this PR introduce a user-facing change?:

NONE

poiana · 2024-05-06T18:45:40Z

Welcome @Apteryks! It looks like this is your first PR to falcosecurity/libs 🎉

Apteryks · 2024-05-08T02:40:59Z

I've now successfully built sysdig against a shared library falcosecurity-libs distinct package with this series, on GNU Guix.

FedeDP · 2024-05-09T15:48:00Z

Hi! Thanks for this PR!
I will invoke our cmake experts for a review :)
To me, changes look good btw!

cc @federico-sysdig @geraldcombs

EDIT: i will edit the PR title and body to follow our template (as per our commit convention: https://github.com/falcosecurity/.github/blob/main/CONTRIBUTING.md#commit-convention)

federico-sysdig

It's an interesting PR and there's excellent work on the corrections for the generated pkgconfig files.
I do believe that something else is desirable for the management of the dependent libraries other than the proposed generator expressions.

I'm curious; I have tried to implement the ability to integrate falcosecurity/libs in a client project through find_package, which is a better, more CMake-oriented, way to use a library. This is not to say that pkgconfig should be left behind, just another option. What are your thoughts on that?

userspace/libscap/engine/gvisor/CMakeLists.txt

userspace/libsinsp/CMakeLists.txt

Apteryks · 2024-05-10T17:35:05Z

It's an interesting PR and there's excellent work on the corrections for the generated pkgconfig files. I do believe that something else is desirable for the management of the dependent libraries other than the proposed generator expressions.

I'll get to these in a bit.

I'm curious; I have tried to implement the ability to integrate falcosecurity/libs in a client project through find_package, which is a better, more CMake-oriented, way to use a library. This is not to say that pkgconfig should be left behind, just another option. What are your thoughts on that?

My immediate thought on this is that a FindFalcosecurityLibs.cmake or similarly named module implementing the logic forfind_package could be implemented viapkg_check_modules, with the obvious drawback that it adds a requirement on a pkg-config binary being available in the environment. Whatever the implementation detail chosen for an eventual CMake-based find_package module for this project, I think it can and should remain a distinct effort from this PR, which focuses on improving the pkg-config generated files :-).

federico-sysdig · 2024-05-10T17:45:59Z

Whatever the implementation detail chosen for an eventual CMake-based find_package module for this project, I think it can and should remain a distinct effort from this PR, which focuses on improving the pkg-config generated files :-).

Of course, I wasn't suggesting to change the scope of this PR.

geraldcombs · 2024-05-10T20:41:21Z

The "build-shared-libs-macos-amd64" job is failing because CFlags in libscap.pc no longer includes -I/opt/homebrew/include:

In file included from /tmp/libs-test/include/falcosecurity/libsinsp/sinsp.h:45:
In file included from /tmp/libs-test/include/falcosecurity/libscap/scap.h:66:
/tmp/libs-test/include/falcosecurity/libscap/uthash_ext.h:24:10: fatal error: 'uthash.h' file not found
#include "uthash.h"
         ^~~~~~~~~~

Apteryks · 2024-05-11T01:13:07Z

The "build-shared-libs-macos-amd64" job is failing because CFlags in libscap.pc no longer includes -I/opt/homebrew/include:

In file included from /tmp/libs-test/include/falcosecurity/libsinsp/sinsp.h:45:
In file included from /tmp/libs-test/include/falcosecurity/libscap/scap.h:66:
/tmp/libs-test/include/falcosecurity/libscap/uthash_ext.h:24:10: fatal error: 'uthash.h' file not found
#include "uthash.h"
         ^~~~~~~~~~

The Cflags of the libscap.pc file shouldn't have changed; I've only added two new entries to them. For example, on my machine, the old copy (master) looks like:

 $ cat ./libscap/libscap.pc 
prefix=${pcfiledir}/../..
libdir=${prefix}/lib64
includedir=${prefix}/include

Name: libscap
Description: lib for System CAPture
Version: 0.0.0

Libs: -L${libdir} -L/gnu/store/65iv1fc7z68cammywngh0dn5995ylbmk-profile/lib -L/gnu/store/65iv1fc7z68cammywngh0dn5995ylbmk-profile/lib -L/gnu/store/65iv1fc7z68cammywngh0dn5995ylbmk-profile/lib -L/gnu/store/65iv1fc7z68cammywngh0dn5995ylbmk-profile/lib -L/gnu/store/65iv1fc7z68cammywngh0dn5995ylbmk-profile/lib -lscap -lz -lprotobuf -ljsoncpp -lscap_engine_nodriver -lscap_engine_test_input -lscap_engine_source_plugin -lscap_engine_kmod -lscap_engine_bpf -lelf -lscap_engine_modern_bpf -lpman
Cflags: -I${includedir}/falcosecurity/libscap

With this change it now reads:

$ cat ../build/libscap/libscap.pc
prefix=/usr/local
libdir=${prefix}/lib64
includedir=${prefix}/include

Name: libscap
Description: lib for System CAPture
Version: 0.0.0

Requires: zlib
Libs: -L${libdir} -L{libdir}/falcosecurity/libscap  -lscap -lscap_engine_nodriver -lscap_engine_test_input -lscap_engine_source_plugin -lscap_engine_kmod -lscap_engine_bpf -lscap_engine_modern_bpf
Cflags: -I${includedir}/falcosecurity/libscap -I${includedir}/falcosecurity/driver -I${includedir}/falcosecurity

Probably this header was found via libsinsp.pc, which was capturing a lot of build-specific, non-installed directories, which I think shouldn't be baked in the generated .pc file.

Previously, it looked like:

$ cat ./libsinsp/libsinsp.pc 
prefix=${pcfiledir}/../..
libdir=${prefix}/lib64
includedir=${prefix}/include

Name: libsinsp
Description: lib for System INSPection
Version: 0.0.0

Requires: libscap
Libs: -L${libdir} -lsinsp -L/gnu/store/65iv1fc7z68cammywngh0dn5995ylbmk-profile/lib -lz -lcurl -ljsoncpp -lre2 -lcares -lgRPC::grpc++ -lgRPC::grpc -lgRPC::gpr -lprotobuf -lcares -lrt -lanl -lssl -lcrypto -ldl -lpthread
Cflags: -I${includedir}/falcosecurity/libsinsp -I/home/maxim/src/falcosecurity-libs -I/home/maxim/src/falcosecurity-libs/userspace -I/home/maxim/src/falcosecurity-libs/userspace/libscap -I/home/maxim/src/falcosecurity-libs/build_orig -I/home/maxim/src/falcosecurity-libs/build_orig/driver/src -I/gnu/store/65iv1fc7z68cammywngh0dn5995ylbmk-profile/include/tbb -I/gnu/store/65iv1fc7z68cammywngh0dn5995ylbmk-profile/include

Now, it looks like:

$ cat ../build/libsinsp/libsinsp.pc 
prefix=/usr/local
libdir=${prefix}/lib64
includedir=${prefix}/include

Name: libsinsp
Description: lib for System INSPection
Version: 0.0.0

Requires: libscap jsoncpp libcares gpr grpc grpc++ protobuf libcrypto libssl
Requires.private: libcurl re2 tbb
Libs: -L${libdir} -lsinsp -lrt -lanl -ldl -lpthread
Cflags: -I${includedir}/falcosecurity/libsinsp -I${includedir}/falcosecurity/driver -I${includedir}/falcosecurity

userspace/libscap/libscap.pc.in

userspace/libsinsp/libsinsp.pc.in

Apteryks · 2025-01-28T01:29:11Z

ugh. format code. I thought I had cmake-format'ed everything already.

Apteryks · 2025-01-28T01:37:33Z

OK, I've run make format-all and pushed; hopefully CI is green now.

FedeDP · 2025-01-28T08:23:25Z

/cc @geraldcombs PTAL thanks!

Apteryks · 2025-01-31T14:21:20Z

Hi; there was a single test error, apparently because it was cancelled: https://github.com/falcosecurity/libs/actions/runs/13001896582/job/36273360103

Could it please be retried?

FedeDP · 2025-01-31T14:36:25Z

Done!

Apteryks · 2025-02-01T03:04:48Z

The single test failure looks like:

Run cd build/test/libsinsp_e2e/
./libsinsp_e2e_tests: 1: Syntax error: ";" unexpected
Error: Process completed with exit code 2.

That only happens on ARM; any ideas what is going on here?

FedeDP · 2025-02-03T07:57:27Z

Never seen that; @therealbobo perhaps can help us?

therealbobo · 2025-02-03T08:26:43Z

Never seen that, I'll build locally to see if the error pops up

geraldcombs · 2025-02-13T00:39:07Z

I was able to test this with Stratoshark on macOS, and everything built and ran fine with no modifications required for the .pc files.

FedeDP · 2025-02-13T08:13:06Z

Thanks Gerald!
@Apteryks can you rebase to fix the conflict? Then we are hopefully good to go :)

This is to so that includes work whether using e.g. #include <scap.h> or #include <libscap/scap.h>, and likewise for libsinp. * userspace/libsinsp/libsinsp.pc.in (Cflags): Add include directive for falcosecurity/driver. * userspace/libscap/libscap.pc.in: Likewise. Also add include directive for uthash. Signed-off-by: Maxim Cournoyer <[email protected]>

* userspace/libpman/libpman.pc.in: New file. * userspace/libpman/CMakeLists.txt: Configure and install it along the libpman header. Signed-off-by: Maxim Cournoyer <[email protected]>

The generated pkg-config files of libscap and libsinsp now makes use of pkg-config Requires and Requires.static fields, which should reduce over-linking when linking to shared libraries. * cmake/modules/BuildPkgConfigDependencies.cmake (add_pkgconfig_library): Add debug messages and fix an issue where IN_LIST had no effect. * cmake/modules/libscap.cmake: Move pkgconfig dependency computation to, pkg-config file configuration to... * userspace/libscap/CMakeLists.txt: ... here, conditionally accumulating Requires and Requires.private values. * userspace/libscap/libscap.pc.in (prefix): Set directly to CMAKE_INSTALL_PREFIX. (Requires, Requires.private): New fields. * userspace/libsinsp/CMakeLists.txt: Separate libraries into pkg-config Requires and Requires.private lists. Add the pkg-config requirements to the ignored link dependencies, since these are now recorded as Requires in the pkg-config file. * userspace/libsinsp/libsinsp.pc.in (Requires): Add @LIBSINSP_REQUIRES@. (Requires.private): New field. (Libs): Remove -lsinsp, automatically computed in SINSP_PKG_CONFIG_LIBS. Signed-off-by: Maxim Cournoyer <[email protected]>

* driver/CMakeLists.txt (DRIVER_SOURCES): Add missing headers. * userspace/libsinsp/test/CMakeLists.txt (unit-test-libsinsp): Link to libgrpc++ to avoid a missing DSO error. * userspace/libscap/CMakeLists.txt: Do not hardcode STATIC type for scap_event_schema and scap_platform libraries, so as to install them (they are referenced in the pkg-config files). Fixes: falcosecurity#1820 Signed-off-by: Maxim Cournoyer <[email protected]>

Apteryks · 2025-02-13T11:56:28Z

Thanks Gerald! @Apteryks can you rebase to fix the conflict? Then we are hopefully good to go :)

Done!

poiana · 2025-02-13T16:09:17Z

@geraldcombs: changing LGTM is restricted to collaborators

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

FedeDP

/approve

poiana · 2025-02-13T16:21:07Z

LGTM label has been added.

Git tree hash: 041b2a198f1e9774e2b4b16227a4e9654672524f

poiana · 2025-02-13T16:21:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Apteryks, FedeDP

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [FedeDP]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

poiana added kind/bug Something isn't working dco-signoff: no do-not-merge/release-note-label-needed labels May 6, 2024

poiana requested review from gnosek and hbrueckner May 6, 2024 18:45

poiana added the size/S label May 6, 2024

Apteryks force-pushed the fix-shared-lib-and-pkg-config-files branch from 192681c to 816a718 Compare May 8, 2024 02:39

poiana added dco-signoff: yes size/L and removed dco-signoff: no size/S labels May 8, 2024

Apteryks mentioned this pull request May 9, 2024

build: Add support for linking to shared falcosecurity libraries. draios/sysdig#2093

Merged

FedeDP changed the title ~~Fix shared lib and pkg config files~~ fix(cmake): fixed shared libs and pkg config files May 9, 2024

poiana added release-note-none and removed do-not-merge/release-note-label-needed labels May 9, 2024

FedeDP added the area/build label May 9, 2024

federico-sysdig reviewed May 9, 2024

View reviewed changes

userspace/libscap/engine/gvisor/CMakeLists.txt Outdated Show resolved Hide resolved

userspace/libsinsp/CMakeLists.txt Outdated Show resolved Hide resolved

geraldcombs reviewed May 9, 2024

View reviewed changes

userspace/libsinsp/CMakeLists.txt Outdated Show resolved Hide resolved

geraldcombs reviewed May 9, 2024

View reviewed changes

userspace/libsinsp/CMakeLists.txt Outdated Show resolved Hide resolved

Apteryks force-pushed the fix-shared-lib-and-pkg-config-files branch from 816a718 to 5b0dea2 Compare May 11, 2024 00:51

Apteryks requested review from geraldcombs and federico-sysdig May 11, 2024 01:14

geraldcombs reviewed May 14, 2024

View reviewed changes

userspace/libscap/libscap.pc.in Outdated Show resolved Hide resolved

userspace/libsinsp/libsinsp.pc.in Outdated Show resolved Hide resolved

Apteryks force-pushed the fix-shared-lib-and-pkg-config-files branch from d7f10d4 to 507b8f5 Compare January 28, 2025 01:37

Apteryks mentioned this pull request Jan 31, 2025

new(driver/modern_bpf,userspace/libpman): support multiple programs for each event #2255

Merged

Apteryks added 4 commits February 13, 2025 20:52

build: Install the pman header and a libpman.pc file.

251302d

* userspace/libpman/libpman.pc.in: New file. * userspace/libpman/CMakeLists.txt: Configure and install it along the libpman header. Signed-off-by: Maxim Cournoyer <[email protected]>

Apteryks force-pushed the fix-shared-lib-and-pkg-config-files branch from 507b8f5 to 7096eed Compare February 13, 2025 11:56

leogr requested a review from geraldcombs February 13, 2025 15:26

geraldcombs approved these changes Feb 13, 2025

View reviewed changes

FedeDP approved these changes Feb 13, 2025

View reviewed changes

poiana assigned FedeDP Feb 13, 2025

poiana added the lgtm label Feb 13, 2025

poiana added the approved label Feb 13, 2025

leogr approved these changes Feb 13, 2025

View reviewed changes

poiana assigned leogr Feb 13, 2025

poiana merged commit d4e5c6b into falcosecurity:master Feb 13, 2025
57 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cmake): improve shared libs and pkg config files #1842

fix(cmake): improve shared libs and pkg config files #1842

Apteryks commented May 6, 2024 •

edited by FedeDP

Loading

poiana commented May 6, 2024

Apteryks commented May 8, 2024

FedeDP commented May 9, 2024 •

edited

Loading

federico-sysdig left a comment

Apteryks commented May 10, 2024

federico-sysdig commented May 10, 2024

geraldcombs commented May 10, 2024

Apteryks commented May 11, 2024

Apteryks commented Jan 28, 2025

Apteryks commented Jan 28, 2025

FedeDP commented Jan 28, 2025

Apteryks commented Jan 31, 2025

FedeDP commented Jan 31, 2025

Apteryks commented Feb 1, 2025

FedeDP commented Feb 3, 2025

therealbobo commented Feb 3, 2025

geraldcombs commented Feb 13, 2025

FedeDP commented Feb 13, 2025

Apteryks commented Feb 13, 2025

poiana commented Feb 13, 2025

FedeDP left a comment

poiana commented Feb 13, 2025

poiana commented Feb 13, 2025

fix(cmake): improve shared libs and pkg config files #1842

fix(cmake): improve shared libs and pkg config files #1842

Conversation

Apteryks commented May 6, 2024 • edited by FedeDP Loading

poiana commented May 6, 2024

Apteryks commented May 8, 2024

FedeDP commented May 9, 2024 • edited Loading

federico-sysdig left a comment

Choose a reason for hiding this comment

Apteryks commented May 10, 2024

federico-sysdig commented May 10, 2024

geraldcombs commented May 10, 2024

Apteryks commented May 11, 2024

Apteryks commented Jan 28, 2025

Apteryks commented Jan 28, 2025

FedeDP commented Jan 28, 2025

Apteryks commented Jan 31, 2025

FedeDP commented Jan 31, 2025

Apteryks commented Feb 1, 2025

FedeDP commented Feb 3, 2025

therealbobo commented Feb 3, 2025

geraldcombs commented Feb 13, 2025

FedeDP commented Feb 13, 2025

Apteryks commented Feb 13, 2025

poiana commented Feb 13, 2025

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Feb 13, 2025

poiana commented Feb 13, 2025

Apteryks commented May 6, 2024 •

edited by FedeDP

Loading

FedeDP commented May 9, 2024 •

edited

Loading