Fix pkgconfig files (mostly for static linking) #2005

gnosek · 2024-08-14T06:37:07Z

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area API-version

/area build

/area CI

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

/area libscap-engine-bpf

/area libscap-engine-gvisor

/area libscap-engine-kmod

/area libscap-engine-modern-bpf

/area libscap-engine-nodriver

/area libscap-engine-noop

/area libscap-engine-source-plugin

/area libscap-engine-savefile

/area libscap

/area libpman

/area libsinsp

/area tests

/area proposals

Does this PR require a change in the driver versions?

/version driver-API-version-major

/version driver-API-version-minor

/version driver-API-version-patch

/version driver-SCHEMA-version-major

/version driver-SCHEMA-version-minor

/version driver-SCHEMA-version-patch

What this PR does / why we need it:

This PR cleans up a bunch of things related to pkg-config files, so that:

generated pkg-config files can be used to build a working binary with static/dynamic linking and bundled/system deps
build directory paths do not leak into the generated pkg-config files

Also, it turns out we no longer have a circular dependency between scap and scap_engine_gvisor, so just make scap_engine_gvisor into a proper shared library.

Which issue(s) this PR fixes:

Fixes #2004

Special notes for your reviewer:

Is there a CI wizard in the house? We could use a similar test (build sinsp-example using pkgconfig) for static builds but I feel like I'd break more than fix if I tried that myself :)

Also, fingers crossed for all the build variants I was unable to test locally :)

Does this PR introduce a user-facing change?:

fix(build): pkgconfig files should be now generated properly even in static library builds
fix(build): scap_engine_gvisor is now a separate shared library

github-actions · 2024-08-14T06:41:36Z

Perf diff from master - unit tests

     8.93%     -1.87%  [.] sinsp::next
     9.14%     +1.06%  [.] sinsp_parser::reset
     4.18%     +0.96%  [.] sinsp_evt::load_params
     1.19%     +0.66%  [.] std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release
     1.27%     -0.66%  [.] sinsp_evt::get_direction
     1.99%     -0.66%  [.] libsinsp::sinsp_suppress::process_event
     2.43%     +0.65%  [.] sinsp_thread_manager::find_thread
     1.92%     -0.56%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*>
     1.38%     +0.50%  [.] 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
     0.71%     +0.50%  [.] libsinsp::state::stl_container_table_adapter<std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, libsinsp::state::value_table_entry_adapter<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, libsinsp::state::value_table_entry_adapter<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >::dynamic_fields_t>::stl_container_table_adapter

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.0329         -0.0329           147           142           147           142
BM_sinsp_split_median                                          -0.0355         -0.0355           147           142           147           142
BM_sinsp_split_stddev                                          +3.0737         +3.0737             0             2             0             2
BM_sinsp_split_cv                                              +3.2124         +3.2124             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_mean                  -0.0235         -0.0235            57            55            57            55
BM_sinsp_concatenate_paths_relative_path_median                -0.0218         -0.0218            57            55            57            55
BM_sinsp_concatenate_paths_relative_path_stddev                +1.2576         +1.2605             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_cv                    +1.3119         +1.3148             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_mean                     -0.0406         -0.0406            25            24            25            24
BM_sinsp_concatenate_paths_empty_path_median                   -0.0345         -0.0345            25            24            25            24
BM_sinsp_concatenate_paths_empty_path_stddev                   -0.8749         -0.8772             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_cv                       -0.8696         -0.8720             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_mean                  -0.0112         -0.0112            57            56            57            56
BM_sinsp_concatenate_paths_absolute_path_median                -0.0099         -0.0099            56            56            56            56
BM_sinsp_concatenate_paths_absolute_path_stddev                -0.3382         -0.3384             1             0             1             0
BM_sinsp_concatenate_paths_absolute_path_cv                    -0.3307         -0.3310             0             0             0             0
BM_sinsp_split_container_image_mean                            -0.0083         -0.0083           402           399           402           399
BM_sinsp_split_container_image_median                          -0.0093         -0.0093           403           399           403           399
BM_sinsp_split_container_image_stddev                          -0.3526         -0.3514             3             2             3             2
BM_sinsp_split_container_image_cv                              -0.3472         -0.3460             0             0             0             0

codecov · 2024-08-14T06:55:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.58%. Comparing base (aeb8793) to head (53cf2d7).
Report is 14 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2005      +/-   ##
==========================================
- Coverage   73.58%   73.58%   -0.01%     
==========================================
  Files         253      253              
  Lines       31869    31869              
  Branches     5649     5630      -19     
==========================================
- Hits        23452    23451       -1     
+ Misses       8416     8401      -15     
- Partials        1       17      +16

Flag	Coverage Δ
libsinsp	`73.58% <ø> (-0.01%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-08-14T06:57:09Z

Perf diff from master - unit tests

     5.00%     +1.35%  [.] sinsp_evt::get_type
    11.45%     -0.85%  [.] sinsp_parser::reset
     0.11%     +0.55%  [.] scap_file_test_helpers::capture_search_evt_by_num
     2.12%     +0.47%  [.] 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
     1.06%     -0.42%  [.] sinsp_parser::event_cleanup
     2.01%     -0.39%  [.] std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release
     0.47%     +0.38%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char*>
     0.50%     -0.36%  [.] sinsp_evt::is_filtered_out
     3.34%     -0.36%  [.] sinsp_thread_manager::get_thread_ref
     0.44%     -0.36%  [.] std::_Hashtable<long, std::pair<long const, std::shared_ptr<sinsp_fdinfo> >, std::allocator<std::pair<long const, std::shared_ptr<sinsp_fdinfo> > >, 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> >::clear

Perf diff from master - scap file

    16.17%     -7.33%  [.] sinsp_filter_check::tostring
     9.70%     +3.22%  [.] sinsp_filter_check::rawval_to_string
    12.96%     -2.11%  [.] sinsp_filter_check_event::extract_single
     9.69%     -2.04%  [.] sinsp_evt::get_type
     6.36%     -1.98%  [.] libsinsp::runc::match_container_id
     6.49%     -1.90%  [.] sinsp_filter_check_thread::extract_single
     3.22%     +1.61%  [.] sinsp_filter_check_container::extract_single
     3.24%     +1.59%  [.] 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
     3.18%     -0.25%  [.] std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::emplace_back<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >
     6.43%     -0.17%  [.] sinsp_evt_formatter::tostring_withformat

Heap diff from master - unit tests

peak heap memory consumption: -5.18K
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

geraldcombs · 2024-08-15T21:16:13Z

LGTM. I tested a Logray build on macOS using the following CMake preset. The generated pkgconfig files look correct and after updating the Logray code to match the current libs API, everything built and ran as expected.

    {
        "name": "libs-tmp",
        "displayName": "Falco libs",
        "description": "libs for bundling with Logray, /tmp/libs-test",
        "generator": "Ninja",
        "binaryDir": "${sourceDir}/build",
        "cacheVariables": {
            "BUILD_SHARED_LIBS": "ON",
            "MINIMAL_BUILD": "ON",
            "USE_BUNDLED_DEPS": "OFF",
            "USE_BUNDLED_VALIJSON": "ON",
            "USE_BUNDLED_TINYDIR": "ON",
            "CREATE_TEST_TARGETS": "OFF",
            "CMAKE_BUILD_TYPE": "RelWithDebInfo",
            "CMAKE_INSTALL_PREFIX": "/tmp/libs-test"
        }
    },

leogr · 2024-08-20T08:20:55Z

userspace/libscap/CMakeLists.txt

@@ -214,21 +214,7 @@ endif()

 if(HAS_ENGINE_GVISOR)
 	add_subdirectory(engine/gvisor)
-	# The static and shared build differs here because a shared scap_engine_gvisor
-	# will result in circular dependencies.
-	if(BUILD_SHARED_LIBS)


Just a question after a very first look: will BUILD_SHARED_LIBS behave like before? 🤔

WDYM? libscap_engine_gvisor.so becomes a separate shared library, so that's a change, but other than that, there should be no observable changes (at least, Works For Me ™️)

TBH I'm not entirely sure when the circular dependency disappeared, probably when extracting some scap_platform_util or other

I meant regarding BUILD_SHARED_LIBS in general, but I noticed right now that it's only related to gvisor (and I believe it's acceptable).

cc @LucaGuerra

FWIW, I consider splitting out scap_engine_gvisor as a separate .so a fix

sounds fine!

leogr · 2024-08-21T13:44:43Z

userspace/libscap/engine/gvisor/CMakeLists.txt


-    add_dependencies(scap_engine_gvisor uthash)
-    target_link_libraries(scap_engine_gvisor
+target_include_directories(scap_engine_gvisor
    PRIVATE


Should this be PUBLIC too? 🤔

Hmm we'll find out when somebody tries to include scap_gvisor's headers :)

Overall, including anything except libsinsp/sinsp.h and its dependencies is not really supported right now (IIRC you can't include libsinsp/cri.h for example) and there's a decision to make. We either:

support that and install a bunch of extra headers (IIRC we need absl and it's a pain to extract from grpc sources)

explicitly don't support that and (ideally) remove the headers from the installed set

I'd greatly prefer option 2 (drop most headers from the installed set) but that would require us to have a well specified API :) I guess one option is to remove them and see who complains. Another would be to decree that sinsp.h is the public API and anything it does not include is off limits (if you want to include anything else, you're not using sinsp, you're developing it).

Either way, I think it's best to have a clear way with the private(ish) headers shipped with libs and IMO figuring that out is out of scope for this PR (which AFAIK at least doesn't make things worse).

we'll find out when somebody tries to include scap_gvisor's headers :)
😶‍🌫️

gnosek · 2024-09-11T05:41:14Z

I updated the branch to handle bs_threadpool, mostly by ignoring it (it does not appear in any headers it seems). I haven't got around to using the thread pool yet, but if it turns out we do need to install the headers, we'll know eventually ;)

FedeDP · 2024-09-11T10:26:16Z

/milestone 0.19.0

FedeDP · 2024-09-24T09:37:28Z

Hey @gnosek can you rebase this PR on top of latest master (so that we test zig build? :D )

After that, this LGTM and can be merged IMHO.

LucaGuerra · 2024-09-25T09:20:07Z

Agreed! Let's rebase and merge this

Unify the implementation between libscap and libsinsp, recursively descend into dependencies to build the whole tree (while avoiding the scap->scap_engine_gvisor->scap cyclic dependency) and skip static libraries linked by shared libraries (they do not need to be linked again when building the final binary). Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

I must be the only one for whom building gvisor in a shared library fails (or maybe the only one actually trying to do this), but I can't see how it could compile. We need to: * privately link the object library with scap_platform_util to pick up its includes * publicly link with gRPC (and other dependencies) to ensure correct linking order Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

When using bundled dependencies, we install their headers into .../include/falcosecurity, so we do not need to generate a fancy include path. This way we don't leak the build-time include paths to dependencies in the generated pkgconfig files. When we're not using bundled dependencies, we still need to add their include paths to ours though, so keep doing that (for the three dependencies: tbb, curl, jsoncpp, that are actually used in any headers). The elephant in the room is gRPC (with its dependency absl), which we don't install at all but also have never put in LIBSINSP_INCLUDE_DIRS, so this patch doesn't make things worse: including pkgconfig-installed sinsp headers that depend on gRPC didn't work before and doesn't work now either. The real fix would be to stop installing them (they're not really public), but reviewing all headers for public/private status is out of scope for this PR. Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

In fact, do not install the headers either, since they're not used in the public API. Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

gnosek · 2024-10-01T10:20:21Z

@FedeDP @LucaGuerra just rebased and pushed

FedeDP · 2024-10-01T10:25:08Z

Thank you Grzeg!

FedeDP

/approve

poiana · 2024-10-01T10:44:37Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,gnosek]

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

poiana · 2024-10-01T10:44:37Z

LGTM label has been added.

Git tree hash: ce095dd5ce42502b3f115984198c4b13cb82f02e

poiana added release-note dco-signoff: yes kind/bug Something isn't working kind/cleanup area/build approved labels Aug 14, 2024

poiana requested review from hbrueckner and mstemm August 14, 2024 06:37

poiana added the size/XL label Aug 14, 2024

gnosek force-pushed the static-pkgconfig-fix branch from e4de4bf to 76021dc Compare August 14, 2024 06:44

leogr reviewed Aug 20, 2024

View reviewed changes

Andreagit97 added this to the TBD milestone Aug 27, 2024

leogr reviewed Aug 28, 2024

View reviewed changes

gnosek force-pushed the static-pkgconfig-fix branch from 76021dc to 5996563 Compare September 11, 2024 05:37

gnosek force-pushed the static-pkgconfig-fix branch from 5996563 to 68a5edd Compare September 11, 2024 09:08

poiana modified the milestones: TBD, 0.19.0 Sep 11, 2024

gnosek added 5 commits October 1, 2024 12:16

fix(build): install libpman

af7b76b

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

fix(build): install cri_* helper libraries and generated headers

a9399a2

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

gnosek added 7 commits October 1, 2024 12:16

fix(build): link cri_* with grpc

00c76a8

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

fix(build): make sinsp->cri_* linkage public

2bf1a1b

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(build): remove now-unused link

f6f1119

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

fix(build): make engines' links public

f08b1ef

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

fix(build): do not install entire libelf as "headers"

0535c22

In fact, do not install the headers either, since they're not used in the public API. Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(build): do not install libbpf headers

53ee284

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(build): gvisor no longer has a circular dependency on scap

53cf2d7

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

gnosek force-pushed the static-pkgconfig-fix branch from 68a5edd to 53cf2d7 Compare October 1, 2024 10:16

FedeDP approved these changes Oct 1, 2024

View reviewed changes

poiana assigned FedeDP Oct 1, 2024

poiana added the lgtm label Oct 1, 2024

LucaGuerra approved these changes Oct 2, 2024

View reviewed changes

poiana assigned LucaGuerra Oct 2, 2024

leogr approved these changes Oct 2, 2024

View reviewed changes

poiana assigned leogr Oct 2, 2024

poiana merged commit 85713d3 into falcosecurity:master Oct 2, 2024
49 checks passed

gnosek deleted the static-pkgconfig-fix branch October 2, 2024 08:46

FedeDP mentioned this pull request Oct 7, 2024

fix(build): add includes to gvisor shared lib build #2079

Closed

FedeDP mentioned this pull request Oct 15, 2024

fix(userspace/libscap): scap-gvisor does need to depend upon jsoncpp. #2112

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pkgconfig files (mostly for static linking) #2005

Fix pkgconfig files (mostly for static linking) #2005

gnosek commented Aug 14, 2024

github-actions bot commented Aug 14, 2024 •

edited

Loading

codecov bot commented Aug 14, 2024 •

edited

Loading

github-actions bot commented Aug 14, 2024

geraldcombs commented Aug 15, 2024

leogr Aug 20, 2024

gnosek Aug 20, 2024

leogr Aug 20, 2024

gnosek Aug 20, 2024

LucaGuerra Aug 20, 2024

leogr Aug 21, 2024

gnosek Aug 28, 2024

leogr Aug 28, 2024

gnosek commented Sep 11, 2024

FedeDP commented Sep 11, 2024

FedeDP commented Sep 24, 2024

LucaGuerra commented Sep 25, 2024

gnosek commented Oct 1, 2024

FedeDP commented Oct 1, 2024

FedeDP left a comment

poiana commented Oct 1, 2024

poiana commented Oct 1, 2024

Fix pkgconfig files (mostly for static linking) #2005

Fix pkgconfig files (mostly for static linking) #2005

Conversation

gnosek commented Aug 14, 2024

github-actions bot commented Aug 14, 2024 • edited Loading

Perf diff from master - unit tests

Heap diff from master - unit tests

Heap diff from master - scap file

Benchmarks diff from master

codecov bot commented Aug 14, 2024 • edited Loading

Codecov Report

github-actions bot commented Aug 14, 2024

Perf diff from master - unit tests

Perf diff from master - scap file

Heap diff from master - unit tests

Heap diff from master - scap file

geraldcombs commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnosek commented Sep 11, 2024

FedeDP commented Sep 11, 2024

FedeDP commented Sep 24, 2024

LucaGuerra commented Sep 25, 2024

gnosek commented Oct 1, 2024

FedeDP commented Oct 1, 2024

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Oct 1, 2024

poiana commented Oct 1, 2024

github-actions bot commented Aug 14, 2024 •

edited

Loading

codecov bot commented Aug 14, 2024 •

edited

Loading