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

Enable Building Both libjbpf.a and libjbpf.so with cmake option -DJBPF_STATIC=Both #22

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

doctorlai-msrc
Copy link
Collaborator

This PR changes the cmake option JBPF_STATIC to 3 states: On, Off and Both.
When set to Both, both the libjbpf.a and libjbpf.so will be built

@doctorlai-msrc doctorlai-msrc marked this pull request as ready for review December 9, 2024 14:23
@xfoukas
Copy link
Collaborator

xfoukas commented Dec 11, 2024

@doctorlai-msrc Can you please explain why we cannot build both the static and the dynamic library by default? Why

@doctorlai-msrc
Copy link
Collaborator Author

@doctorlai-msrc Can you please explain why we cannot build both the static and the dynamic library by default? Why

We can, however, building both would double the time.

@xfoukas
Copy link
Collaborator

xfoukas commented Dec 12, 2024

Why would it double the time? Have you profiled this? Could you share some numbers?

@doctorlai-msrc
Copy link
Collaborator Author

Why would it double the time? Have you profiled this? Could you share some numbers?

Because building both will have two add_library. I did a few quick tests, however between tests, the time statistics do vary.

rm -rf * && rm -rf ../out/ && cmake ../ -DJBPF_STATIC=On
time bash -c " make -j"
real 1m16.577s
user 6m53.224s
sys 0m58.393s

rm -rf * && rm -rf ../out/ && cmake ../ -DJBPF_STATIC=Off
time bash -c " make -j"
real 1m46.668s
user 9m29.203s
sys 1m28.137s

rm -rf * && rm -rf ../out/ && cmake ../ -DJBPF_STATIC=Both
time bash -c " make -j"
real 1m58.854s
user 10m11.512s
sys 1m45.767s

@xfoukas
Copy link
Collaborator

xfoukas commented Dec 17, 2024

The default (JBPF_STATIC=off) and the case when building both take similar time according to this profiling, so in my view we should build both by default, as it simplifies the build process.

@xfoukas xfoukas self-requested a review December 17, 2024 15:54
CMakeLists.txt Outdated
add_definitions(-DJBPF_SHARED_LIB)
elseif(JBPF_STATIC_OPTION_UPPER STREQUAL "ON")
# Optional: Add definitions or logic for static builds if needed
elseif(JBPF_STATIC_OPTION_UPPER STREQUAL "BOTH")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this introduces an a bug for the static build. If this option is set to "BOTH", then jbpf will be built with -DJBPF_SHARED_LIB enabled, which would introduce a bug in the produced .a due to this preprocessor check:

#ifdef JBPF_SHARED_LIB

When this option is set to "BOTH", we need to make sure that the static library is built without this flag enabled and the shared library is built with it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants