fix(cmake): fixed shared libs and pkg config files

[apteryks]

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 https://github.com/falcosecurity/libs/issues/1825, which is not addressed by this PR but can be easily worked around.

Does this PR introduce a user-facing change?:

NONE

[poiana]

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

[apteryks]

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

[FedeDP]

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)

[apteryks]

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]

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]

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]

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

[FedeDP]

/milestone 0.18.0

[poiana]

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

[FedeDP]

/remove-lifecycle stale

[FedeDP]

Hi! Can you rebase on latest master? Let's see if CI issues will disappear :D

[FedeDP]

I just rebased on top of latest master. :pray:

[github-actions[bot]]

Perf diff from master - unit tests

     8.54%     +0.49%  [.] std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release
     7.28%     +0.27%  [.] sinsp::next
     1.82%     +0.26%  [.] sinsp_thread_manager::find_thread
     1.54%     -0.25%  [.] std::_Hashtable<long, std::pair<long const, std::shared_ptr<sinsp_threadinfo> >, std::allocator<std::pair<long const, std::shared_ptr<sinsp_threadinfo> > >, std::__detail::_Select1st, std::equal_to<long>, std::hash<long>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true> >::_M_find_before_node
     2.23%     -0.23%  [.] sinsp_evt::load_params
     0.96%     -0.23%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*>
     6.56%     +0.22%  [.] sinsp_parser::reset
     0.77%     +0.21%  [.] sinsp::fetch_next_event
     3.00%     -0.17%  [.] gzfile_read
     1.47%     +0.15%  [.] is_conversion_needed

Heap diff from master - unit tests

peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Benchmarks diff from master

Comparing gbench_data.json to /root/actions-runner/_work/libs/libs/build/gbench_data.json
Benchmark                                                         Time             CPU      Time Old      Time New       CPU Old       CPU New
----------------------------------------------------------------------------------------------------------------------------------------------
BM_sinsp_split_mean                                            +0.0443         +0.0443           144           151           144           151
BM_sinsp_split_median                                          +0.0498         +0.0497           144           151           144           151
BM_sinsp_split_stddev                                          -0.3926         -0.3916             2             1             2             1
BM_sinsp_split_cv                                              -0.4183         -0.4174             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_mean                  -0.0749         -0.0749            61            56            61            56
BM_sinsp_concatenate_paths_relative_path_median                -0.0984         -0.0984            62            56            62            56
BM_sinsp_concatenate_paths_relative_path_stddev                -0.9184         -0.9183             2             0             2             0
BM_sinsp_concatenate_paths_relative_path_cv                    -0.9117         -0.9117             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_mean                     -0.0485         -0.0486            25            24            25            24
BM_sinsp_concatenate_paths_empty_path_median                   -0.0467         -0.0467            25            24            25            24
BM_sinsp_concatenate_paths_empty_path_stddev                   -0.8423         -0.8417             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_cv                       -0.8343         -0.8336             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_mean                  -0.0347         -0.0347            58            56            58            56
BM_sinsp_concatenate_paths_absolute_path_median                -0.0378         -0.0378            58            56            58            56
BM_sinsp_concatenate_paths_absolute_path_stddev                +0.3346         +0.3309             0             1             0             1
BM_sinsp_concatenate_paths_absolute_path_cv                    +0.3825         +0.3787             0             0             0             0
BM_sinsp_split_container_image_mean                            +0.0111         +0.0111           387           391           387           391
BM_sinsp_split_container_image_median                          +0.0092         +0.0092           387           390           387           390
BM_sinsp_split_container_image_stddev                          +0.9320         +0.9284             2             4             2             4
BM_sinsp_split_container_image_cv                              +0.9107         +0.9072             0             0             0             0

[FedeDP]

test-scap-{amd64,arm64} tests are failing with:

make[3]: *** No rule to make target 'protobuf-prefix/src/protobuf/target/lib/libprotobuf.a', needed by 'test/libscap/libscap_test'. Stop.

Care to give it a look @Apteryks ?

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 75.33%. Comparing base (51410de) to head (7096eed). Report is 4 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1842   +/-   ##
=======================================
  Coverage   75.33%   75.33%           
=======================================
  Files         280      280           
  Lines       34554    34554           
  Branches     5901     5900    -1     
=======================================
+ Hits        26031    26032    +1     
+ Misses       8523     8522    -1     

Flag	Coverage Δ
libsinsp	75.33% <ø> (+<0.01%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[FedeDP]

Moving to next milestone since we are nearing a release and this is not ready. /milestone 0.19.0

[FedeDP]

/milestone 0.20.0

Any news on this one?

[leogr]

Any updates on this?

Since 0.20 is too close, moving this to /milestone 0.21.0

[apteryks]

test-scap-{amd64,arm64} tests are failing with:

make[3]: *** No rule to make target 'protobuf-prefix/src/protobuf/target/lib/libprotobuf.a', needed by 'test/libscap/libscap_test'. Stop.

Care to give it a look @Apteryks ?

Sorry for the long wait, I'll take a look at it now.

[FedeDP]

No problem :)

[apteryks]

Lot's have changed (for the better it seems); there's less in this PR following a rebase. I've been conservative in my edits in the hope that even the most exotic platforms shouldn't be impacted in the CI; let's see.

[apteryks]

Lot's have changed (for the better it seems); there's less in this PR following a rebase. I've been conservative in my edits in the hope that even the most exotic platforms shouldn't be impacted in the CI; let's see.

Actually, the pkg-config files Libs: entries are back to contain a bit too much, which is correct for the static build case but leads to overlinking on GNU/Linux, which is not fatal. I'm not sure how to fix this cleanly for both builds to pass, so I guess I'll leave it at that.

[apteryks]

Actually, the pkg-config files Libs: entries are back to contain a bit too much, which is correct for the static build case but leads to overlinking on GNU/Linux, which is not fatal. I'm not sure how to fix this cleanly for both builds to pass, so I guess I'll leave it at that.

The latest revision resolves that. It's a bit hacky in the add_pkgconfig_library macro, but to be honest, it already was :-). The result looks great and I could build sysdig (which a few edits there to support the latest falcosecurity-libs).

I'm sharing the content of the freshly built pkg-config files, for a build configured with the flags: cmake -DBUILD_SHARED_LIBS=ON -DUSE_BUNDLED_DEPS=OFF -DCMAKE_INSTALL_PREFIX=$PWD/install "-DBUILD_DRIVER=OFF" "-DENABLE_DKMS=OFF" "-DBUILD_LIBSCAP_MODERN_BPF=ON":

$ cat install/lib64/pkgconfig/libscap.pc
prefix=/home/maxim/src/falcosecurity-libs/buildc/install
libdir=${prefix}/lib64
includedir=${prefix}/include/falcosecurity

Name: libscap
Description: lib for System CAPture
Version: 0.21.0-19+d07beb8

# Note: jsoncpp and protobuf are required by scap_engine_gvisor, which
# currently lacks its own pkg-config file.
Requires: jsoncpp libpman protobuf
Requires.private: zlib
Libs: -L${libdir}  -lscap -lscap_event_schema -lscap_platform -lscap_engine_nodriver -lscap_engine_test_input -lscap_engine_source_plugin -lscap_engine_kmod -lscap_event_schema -lscap_platform -lscap_engine_bpf -lscap_event_schema -lscap_platform -lelf -lscap_engine_modern_bpf -lscap_engine_gvisor -lscap_event_schema
Cflags: -I${includedir} -I${includedir}/libscap -I${includedir}/driver -I/gnu/store/4b30xdqjj81dcyl5p518rp33vlm2iag8-profile/include

$ cat install/lib64/pkgconfig/libsinsp.pc
prefix=/home/maxim/src/falcosecurity-libs/buildc/install
libdir=${prefix}/lib64
includedir=${prefix}/include/falcosecurity

Name: libsinsp
Description: lib for System INSPection
Version: 0.21.0-19+d07beb8

Requires: libscap jsoncpp libcares gpr grpc grpc++ protobuf libcares libcrypto libssl
Requires.private: libcurl re2 tbb
Libs: -L${libdir}  -lsinsp
Cflags: -I${includedir} -I${includedir}/libsinsp -I${includedir}/driver -I/gnu/store/4b30xdqjj81dcyl5p518rp33vlm2iag8-profile/include/tbb -I/gnu/store/4b30xdqjj81dcyl5p518rp33vlm2iag8-profile/include

$ cat install/lib64/pkgconfig/libpman.pc
prefix=/home/maxim/src/falcosecurity-libs/buildc/install
libdir=${prefix}/lib64
includedir=${prefix}/include

Name: libpman
Description: Utility library for BPF probes
Version: 0.21.0-19+d07beb8

Requires: libbpf zlib
Libs: -L${libdir} -lpman -lscap_event_schema -lscap_platform
Cflags: -I${includedir}

[apteryks]

I think that's ready to be reviewed again.

[apteryks]

Only the sinsp example with pkg-config test appears to fail when building as a shared library on Linux and MacOS. I'll look into it; perhaps the install directory needs to be added to PKG_CONFIG_PATH.

[apteryks]

These tests pass locally, on my Guix System. I needed to use lib64 instead of lib in the PKG_CONFIG_PATH (and LIBRARY_PATH) though, so perhaps the paths in the ci.yml file need a similar adjustment?

[apteryks]

Phew. I think my last push hopefully addresses the remaining test failures. I've made the Requires and Requires.private for libscap conditionally computed like for libsinsp, and dropped what seemed like an extraneous protobuf link dependency I had added to the scap unit test.

[apteryks]

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

[apteryks]

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

[FedeDP]

/cc @geraldcombs PTAL thanks!