cleanup(build): cut down the number of shared libraries #1217

gnosek · 2023-07-20T10:53:29Z

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-engine-udig

/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:

We're shipping many tiny shared libraries, which isn't really helpful (they're too small to be valuable on their own). Convert them all to static libraries which are then linked directly into libscap.so (for a shared library build).

While I was here:

clean up the scap_engine_savefile build (it's now simply a static library)
clean up modern_bpf libbpf handling (we always have the libbpf target nowadays)
fix libbpf and libelf so they can be linked properly (libbpf is statically linked anyway but needs -fPIC, libelf can be dynamically linked)

Which issue(s) this PR fixes:

Fixes #1201
Fixes #1202

Special notes for your reviewer:

With this PR, I can successfully build a shared library with all the engines: -DUSE_BUNDLED_DEPS=Off -DBUILD_SHARED_LIBS=On -DENABLE_PIC=On -DUSE_BUNDLED_VALIJSON=On -DUSE_BUNDLED_LIBBPF=On -DBUILD_LIBSCAP_MODERN_BPF=On -DBUILD_BPF=On -DBUILD_DRIVER=On -DCREATE_TEST_TARGETS=On

Note 1: looks like Ubuntu 22.04 has a vastly different libbpf API so I need to use the bundled one in all cases
Note 2: why do we have a separate option for ENABLE_PIC? (do we want to be extra clean and use it to conditionally enable -fPIC for libelf?)

Does this PR introduce a user-facing change?:

cleanup(build): scap_platform_util, scap_event_schema, driver_event_schema, scap_engine_util, scap_engine_noop and scap_platform are no longer separate shared libraries

Otherwise we can't link libbpf.a into a shared library (libscap.so) Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

gnosek · 2023-07-20T10:54:36Z

cc @dkogan maybe this helps with the Debian build? It will probably need some massaging to backport to 0.11.x but I can help if needed

cc @geraldcombs does this help with the shared macOS build?

gnosek · 2023-07-20T11:18:57Z

Sigh. Right. We need ENABLE_PIC whenever we build shared libs.

gnosek · 2023-07-20T11:26:39Z

~~Oh. I left a debug msg in. Will remove it once the CI passes (I want to see if -fPIC works now first)~~

It passes. Debug removed.

Andreagit97

LGTM just one minor comment for @FedeDP
/approve

Andreagit97 · 2023-07-20T11:56:20Z

cmake/modules/libelf.cmake

@@ -7,7 +7,7 @@ if(LIBELF_INCLUDE)
    # we already have LIBELF
 elseif(NOT USE_BUNDLED_LIBELF)
    find_path(LIBELF_INCLUDE elf.h PATH_SUFFIXES elf)
-    find_library(LIBELF_LIB NAMES libelf.a libelf.so)
+    find_library(LIBELF_LIB NAMES elf)


uhm @FedeDP do you remember why we put both of them explicitly? I remember that at a certain point, we ended up with a bundled Falco build using a dynamic libelf instead of a static one but i don't remember the cause :/

Yep, this commit: 4566941

@gnosek can you revert this part?

Not really. A dynamically linked build will try to pull in libelf.a, which isn't compiled with -fPIC and so won't link. We'll need a more involved fix.

@Andreagit97 @FedeDP 403b81d

BTW I'm pretty sure this can be solved at cmake level with some obscure variable or other but an explicit check like this seems enough for now.

I like it! Thanks

poiana · 2023-07-20T12:01:36Z

LGTM label has been added.

Git tree hash: 8a31c34a22cc9b430ebfcba9bd8985367ff53c91

FedeDP · 2023-07-20T12:04:22Z

/hold

Andreagit97 · 2023-07-20T12:04:46Z

/hold

A system libelf.a won't probably be compiled with -fPIC, meaning we can't link it into a .so. Use libelf.so in that case. (the above "probably" is true at least on Ubuntu 22.04) Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

We always have a `libbpf` target now, even if not using bundled libbpf Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

It's too small to be worth shipping as a separate .so Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

It's too small to be worth shipping as a separate .so. Also note that the set_scap_target_properties was wrong Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

It's too small to be worth shipping as a separate .so. Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

We can't (yet) easily build it as a shared library due to circular dependencies but we should be okay with building it as a static library linked directly to libscap.so. Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

It's too small to be worth shipping as a separate .so and it's not useful on its own (only as a part of another engine). Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

It's too small to be worth shipping as a separate .so. Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

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

FedeDP

/approve

poiana · 2023-07-20T12:32:48Z

LGTM label has been added.

Git tree hash: f8d33a3034eff7b9128a831063d705c3203fecea

Andreagit97

/approve

poiana · 2023-07-20T13:26:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, 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~~ [Andreagit97,FedeDP,gnosek]

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

FedeDP · 2023-07-20T13:28:31Z

/unhold

geraldcombs · 2023-07-21T23:27:33Z

cc @geraldcombs does this help with the shared macOS build?

It does, thanks! I had to update the way we build our pkgconfig linker flags (#1222) but everything works here otherwise.

fix(build): build libbpf with -fPIC

d41a644

Otherwise we can't link libbpf.a into a shared library (libscap.so) Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

poiana added kind/cleanup release-note dco-signoff: yes area/build size/M approved labels Jul 20, 2023

poiana requested review from hbrueckner and incertum July 20, 2023 10:53

gnosek mentioned this pull request Jul 20, 2023

fix(userspace/libscap): properly apply driver_event_schema target properties #1215

Closed

gnosek force-pushed the single-so branch from aa09938 to 17bb670 Compare July 20, 2023 11:29

Andreagit97 previously approved these changes Jul 20, 2023

View reviewed changes

poiana assigned Andreagit97 Jul 20, 2023

poiana added the lgtm label Jul 20, 2023

poiana added the do-not-merge/hold label Jul 20, 2023

gnosek added 9 commits July 20, 2023 14:11

fix(build): link shared libraries with libelf.so

403b81d

A system libelf.a won't probably be compiled with -fPIC, meaning we can't link it into a .so. Use libelf.so in that case. (the above "probably" is true at least on Ubuntu 22.04) Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(build): remove duplicated code

a15550f

We always have a `libbpf` target now, even if not using bundled libbpf Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(build): make scap_platform_util a static library

4eced3f

It's too small to be worth shipping as a separate .so Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(build): make scap_error a static library

c544298

It's too small to be worth shipping as a separate .so Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(build): make scap_event_schema a static library

25ca6d1

It's too small to be worth shipping as a separate .so Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(build): make driver_event_schema a static library

ec328f7

It's too small to be worth shipping as a separate .so. Also note that the set_scap_target_properties was wrong Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(build): make scap_engine_util a static library

bfba241

It's too small to be worth shipping as a separate .so. Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(build): make scap_savefile_engine a static library

2d63ab9

We can't (yet) easily build it as a shared library due to circular dependencies but we should be okay with building it as a static library linked directly to libscap.so. Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(build): make scap_engine_noop a static library

e75afa9

It's too small to be worth shipping as a separate .so and it's not useful on its own (only as a part of another engine). Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

gnosek added 2 commits July 20, 2023 14:12

cleanup(build): make scap_platform a static library

cb0d0a4

It's too small to be worth shipping as a separate .so. Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

fix(build): default to -fPIC when building shared libraries

fb1ecb7

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

gnosek dismissed Andreagit97’s stale review via fb1ecb7 July 20, 2023 12:12

gnosek force-pushed the single-so branch from 17bb670 to fb1ecb7 Compare July 20, 2023 12:12

poiana removed the lgtm label Jul 20, 2023

poiana requested a review from Andreagit97 July 20, 2023 12:12

FedeDP approved these changes Jul 20, 2023

View reviewed changes

poiana assigned FedeDP Jul 20, 2023

poiana added the lgtm label Jul 20, 2023

Andreagit97 approved these changes Jul 20, 2023

View reviewed changes

Andreagit97 added this to the 0.12.0 milestone Jul 20, 2023

poiana removed the do-not-merge/hold label Jul 20, 2023

poiana merged commit 3aa48a4 into falcosecurity:master Jul 20, 2023
25 checks passed

gnosek deleted the single-so branch July 20, 2023 13:29

geraldcombs mentioned this pull request Jul 21, 2023

update(scap): Move scap_fds.c to scap_platform_util #1199

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup(build): cut down the number of shared libraries #1217

cleanup(build): cut down the number of shared libraries #1217

gnosek commented Jul 20, 2023 •

edited

Loading

gnosek commented Jul 20, 2023

gnosek commented Jul 20, 2023

gnosek commented Jul 20, 2023 •

edited

Loading

Andreagit97 left a comment

Andreagit97 Jul 20, 2023

FedeDP Jul 20, 2023

gnosek Jul 20, 2023

gnosek Jul 20, 2023

gnosek Jul 20, 2023

FedeDP Jul 20, 2023

poiana commented Jul 20, 2023

FedeDP commented Jul 20, 2023

Andreagit97 commented Jul 20, 2023

FedeDP left a comment

poiana commented Jul 20, 2023

Andreagit97 left a comment

poiana commented Jul 20, 2023

FedeDP commented Jul 20, 2023

geraldcombs commented Jul 21, 2023

cleanup(build): cut down the number of shared libraries #1217

cleanup(build): cut down the number of shared libraries #1217

Conversation

gnosek commented Jul 20, 2023 • edited Loading

gnosek commented Jul 20, 2023

gnosek commented Jul 20, 2023

gnosek commented Jul 20, 2023 • edited Loading

Andreagit97 left a comment

Choose a reason for hiding this comment

Andreagit97 Jul 20, 2023

Choose a reason for hiding this comment

FedeDP Jul 20, 2023

Choose a reason for hiding this comment

gnosek Jul 20, 2023

Choose a reason for hiding this comment

gnosek Jul 20, 2023

Choose a reason for hiding this comment

gnosek Jul 20, 2023

Choose a reason for hiding this comment

FedeDP Jul 20, 2023

Choose a reason for hiding this comment

poiana commented Jul 20, 2023

FedeDP commented Jul 20, 2023

Andreagit97 commented Jul 20, 2023

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Jul 20, 2023

Andreagit97 left a comment

Choose a reason for hiding this comment

poiana commented Jul 20, 2023

FedeDP commented Jul 20, 2023

geraldcombs commented Jul 21, 2023

gnosek commented Jul 20, 2023 •

edited

Loading

gnosek commented Jul 20, 2023 •

edited

Loading