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

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

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

Conversation

Apteryks
Copy link

@Apteryks Apteryks commented May 6, 2024

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
Copy link
Contributor

poiana commented May 6, 2024

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

@Apteryks
Copy link
Author

Apteryks commented May 8, 2024

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

@FedeDP
Copy link
Contributor

FedeDP commented May 9, 2024

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)

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

@federico-sysdig federico-sysdig left a comment

Choose a reason for hiding this comment

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

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/libsinsp/CMakeLists.txt Outdated Show resolved Hide resolved
@Apteryks
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

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 Apteryks force-pushed the fix-shared-lib-and-pkg-config-files branch from 816a718 to 5b0dea2 Compare May 11, 2024 00:51
@Apteryks
Copy link
Author

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 Outdated Show resolved Hide resolved
userspace/libsinsp/libsinsp.pc.in Show resolved Hide resolved
@FedeDP
Copy link
Contributor

FedeDP commented May 15, 2024

/milestone 0.18.0

@poiana poiana added this to the 0.18.0 milestone May 15, 2024
@Apteryks Apteryks force-pushed the fix-shared-lib-and-pkg-config-files branch from 5b0dea2 to c8ea84a Compare May 19, 2024 00:51
@poiana
Copy link
Contributor

poiana commented Aug 17, 2024

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
Copy link
Contributor

FedeDP commented Aug 21, 2024

/remove-lifecycle stale

@FedeDP
Copy link
Contributor

FedeDP commented Aug 21, 2024

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

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 <maxim.cournoyer@gmail.com>
This is needed when linking to the static scap library, as its
pkg-config file contains an '-lpman' directive.

* userspace/libpman/CMakeLists.txt: New install target.

Signed-off-by: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Previously, an erroneous -linstall_lib_link_libraries-NOTFOUND
directive could be added to the configured libscap.pc pkg-config file.

* cmake/modules/libscap.cmake: Ensure ${install_lib_link_library} is
true.

Signed-off-by: Maxim Cournoyer <maxim.cournoyer@gmail.com>
* driver/CMakeLists.txt (DRIVER_SOURCES): Add missing headers.
* userspace/libscap/engine/gvisor/CMakeLists.txt
[BUILD_SHARED_LIBS]: Add missing include directories.
* test/libscap/CMakeLists.txt (LIBSCAP_TESTS_LIBRARIES): Add
${PROTOBUF_LIB}.

Fixes: falcosecurity#1820
Signed-off-by: Maxim Cournoyer <maxim.cournoyer@gmail.com>
The generated pkg-config file now makes use of pkg-config Requires and
Requires.static fields, which should reduce over-linking when linking
to shared libraries.

* userspace/libscap/libscap.pc.in (prefix): Set directly to
CMAKE_INSTALL_PREFIX.
(Libs): Add -L{libdir}/@LIBS_PACKAGE_NAME@/libscap.
* userspace/libsinsp/CMakeLists.txt: Separate libraries into
pkg-config Requires and Requires.private lists.  Do not infer from
installable targets.
* userspace/libsinsp/libsinsp.pc.in (Requires): Add
@LIBSINSP_REQUIRES@.
(Requires.private): New field.
(Libs): Replace @SINSP_PKG_CONFIG_LIBDIRS@ and @SINSP_PKG_CONFIG_LIBS@
with @LIBSINSP_LINK_FLAGS@.
(Cflags): Remove @SINSP_PKG_CONFIG_INCLUDES@ and
@SINSP_PKG_CONFIG_INCLUDES@/driver.  Add
-I${includedir}/@LIBS_PACKAGE_NAME@/driver.

Signed-off-by: Maxim Cournoyer <maxim.cournoyer@gmail.com>
@FedeDP FedeDP force-pushed the fix-shared-lib-and-pkg-config-files branch from c8ea84a to f74a20d Compare August 28, 2024 06:44
@poiana
Copy link
Contributor

poiana commented Aug 28, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Apteryks
Once this PR has been reviewed and has the lgtm label, please assign leogr for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

@FedeDP
Copy link
Contributor

FedeDP commented Aug 28, 2024

I just rebased on top of latest master. 🙏

Copy link

Perf diff from master - unit tests

     9.04%     +0.95%  [.] sinsp_parser::reset
     3.04%     +0.94%  [.] gzfile_read
     7.20%     -0.78%  [.] sinsp::next
     0.92%     +0.68%  [.] libsinsp::events::is_unknown_event
     1.40%     -0.66%  [.] sinsp_evt::get_param
     0.64%     +0.56%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char*>
     6.51%     -0.50%  [.] next
     1.79%     -0.48%  [.] std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release
     1.30%     -0.46%  [.] std::vector<sinsp_evt_param, std::allocator<sinsp_evt_param> >::emplace_back<sinsp_evt*, unsigned int&, char const*, unsigned long&>
     4.66%     -0.45%  [.] sinsp_parser::process_event

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.0165         -0.0163           149           146           149           146
BM_sinsp_split_median                                          -0.0189         -0.0187           149           146           149           146
BM_sinsp_split_stddev                                          +1.5610         +1.5879             0             1             0             1
BM_sinsp_split_cv                                              +1.6040         +1.6309             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_mean                  -0.0231         -0.0229            42            41            42            41
BM_sinsp_concatenate_paths_relative_path_median                -0.0204         -0.0201            42            41            42            41
BM_sinsp_concatenate_paths_relative_path_stddev                -0.5612         -0.5605             1             0             1             0
BM_sinsp_concatenate_paths_relative_path_cv                    -0.5508         -0.5502             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_mean                     +0.0302         +0.0304            17            17            17            17
BM_sinsp_concatenate_paths_empty_path_median                   +0.0254         +0.0255            17            17            17            17
BM_sinsp_concatenate_paths_empty_path_stddev                   +0.4093         +0.4127             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_cv                       +0.3680         +0.3710             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_mean                  -0.1337         -0.1335            49            43            49            43
BM_sinsp_concatenate_paths_absolute_path_median                -0.1351         -0.1349            50            43            50            43
BM_sinsp_concatenate_paths_absolute_path_stddev                -0.4129         -0.4152             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_cv                    -0.3224         -0.3251             0             0             0             0
BM_sinsp_split_container_image_mean                            +0.0124         +0.0126           354           359           354           359
BM_sinsp_split_container_image_median                          +0.0122         +0.0122           354           359           354           359
BM_sinsp_split_container_image_stddev                          -0.1567         -0.1534             2             2             2             2
BM_sinsp_split_container_image_cv                              -0.1670         -0.1639             0             0             0             0

@FedeDP
Copy link
Contributor

FedeDP commented Aug 28, 2024

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 ?

Copy link

codecov bot commented Aug 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.20%. Comparing base (89edd36) to head (f74a20d).
Report is 174 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1842   +/-   ##
=======================================
  Coverage   74.20%   74.20%           
=======================================
  Files         253      253           
  Lines       30832    30832           
  Branches     5394     5394           
=======================================
  Hits        22880    22880           
+ Misses       7934     7924   -10     
- Partials       18       28   +10     
Flag Coverage Δ
libsinsp 74.20% <ø> (ø)

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.

@FedeDP
Copy link
Contributor

FedeDP commented Aug 28, 2024

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

@poiana poiana modified the milestones: 0.18.0, 0.19.0 Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[-DBUILD_SHARED_LIBS=ON] "error: driver/syscall_compat_x86_64.h: No such file or directory" at build time
5 participants