-
Notifications
You must be signed in to change notification settings - Fork 164
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
cleanup(build): cut down the number of shared libraries #1217
Conversation
Otherwise we can't link libbpf.a into a shared library (libscap.so) Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
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? |
Sigh. Right. We need ENABLE_PIC whenever we build shared libs. |
It passes. Debug removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM just one minor comment for @FedeDP
/approve
cmake/modules/libelf.cmake
Outdated
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it! Thanks
LGTM label has been added. Git tree hash: 8a31c34a22cc9b430ebfcba9bd8985367ff53c91
|
/hold |
/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 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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
LGTM label has been added. Git tree hash: f8d33a3034eff7b9128a831063d705c3203fecea
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
[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:
Approvers can indicate their approval by writing |
/unhold |
It does, thanks! I had to update the way we build our pkgconfig linker flags (#1222) but everything works here otherwise. |
What type of PR is this?
/kind cleanup
Any specific area of the project related to this PR?
/area build
Does this PR require a change in the driver versions?
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:
libbpf
target nowadays)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?: