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 poiana added kind/bug Something isn't working dco-signoff: no labels May 6, 2024
@poiana
Copy link
Contributor

poiana commented May 6, 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

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