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(build): add includes to gvisor shared lib build #2079

Closed

Conversation

LucaGuerra
Copy link
Contributor

@LucaGuerra LucaGuerra commented Sep 20, 2024

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

Does this PR require a change in the driver versions?

No

What this PR does / why we need it:

It looks like when trying to build gvisor with shared libs we only got one include which is the uthash one
Example:

"/usr/bin/c++ -DBUILD_TYPE_RELEASE -DFALCO_PROBE_BPF_FILEPATH=\\\".falco/falco-bpf.o\\\" -DHAS_CAPTURE -DHAS_GVISOR -DHAS_MODERN_BPF -DHAVE_STRLCAT -DHAVE_STRLCPY -DHAVE_SYS_SYSMACROS_H -DPLATFORM_NAME=\\\"Linux\\\" -DSCAP_HOSTNAME_ENV_VAR=\\\"FALCO_HOSTNAME\\\" -DSCAP_HOST_ROOT_ENV_VAR_NAME=\\\"HOST_ROOT\\\" -DSCAP_KERNEL_MODULE_NAME=\\\"falco\\\" -D_GNU_SOURCE -I/falco/build/uthash-prefix/src/uthash/src -std=c++17  -fstack-protector-strong -D_FORTIFY_SOURCE=2 -Wall -ggdb    -O3 -fno-strict-aliasing -DNDEBUG -std=c++17 -fPIC -o CMakeFiles/scap_engine_gvisor_o.dir/fillers.cpp.o -c /falco/build/falcosecurity-libs-repo/falcosecurity-libs-prefix/src/falcosecurity-libs/userspace/libscap/engine/gvisor/fillers.cpp",

with preset

                "CMAKE_INSTALL_PREFIX": "/usr",
                "CMAKE_INSTALL_LIBDIR": "lib",
                "CMAKE_BUILD_TYPE": "MinSizeRel",
                "USE_BUNDLED_TBB": "OFF",
                "USE_BUNDLED_RE2": "OFF",
                "USE_BUNDLED_JSONCPP": "OFF",
                "BUILD_FALCO_MODERN_BPF": "ON",
                "BUILD_STATIC_LIBS": "OFF",
                "USE_BUNDLED_DEPS": "OFF",
                "BUILD_SHARED_LIBS": "ON",
                "BUILD_DRIVER": "OFF",
                "FALCOSECURITY_LIBS_SOURCE_DIR": "/falcosecurity/libs",
                "DRIVER_SOURCE_DIR": "/falcosecurity/libs/driver",
                "FALCOSECURITY_SHARED_LIBS_VERSION": "0",
                "FALCOSECURITY_SHARED_LIBS_SOVERSION": "0",
                "CMAKE_EXPORT_COMPILE_COMMANDS": "ON"

Leading, obviously, to all sorts of include not found errors.

This PR makes sure that those directories are included for scap_engine_gvisor_o. I am not 100% sure this is the right way to fix it

cc @geraldcombs @federico-sysdig

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

fix(build): add includes to gvisor shared lib build

@poiana
Copy link
Contributor

poiana commented Sep 20, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LucaGuerra

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

github-actions bot commented Sep 20, 2024

Perf diff from master - unit tests

     4.91%     +1.17%  [.] next
     4.12%     +1.02%  [.] gzfile_read
     1.90%     +0.95%  [.] sinsp_thread_manager::find_thread
     1.27%     +0.84%  [.] std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release
     1.31%     -0.77%  [.] sinsp_filter_check::parse_field_name
     2.47%     -0.63%  [.] scap_event_decode_params
     3.48%     -0.61%  [.] sinsp_thread_manager::get_thread_ref
     0.65%     +0.60%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char*>
     4.33%     -0.56%  [.] sinsp_parser::process_event
    10.69%     +0.44%  [.] sinsp_parser::reset

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.0112         -0.0112           151           149           151           149
BM_sinsp_split_median                                          -0.0093         -0.0094           151           150           151           150
BM_sinsp_split_stddev                                          +0.9133         +0.9117             1             2             1             2
BM_sinsp_split_cv                                              +0.9349         +0.9333             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_mean                  +0.1436         +0.1436            41            47            41            47
BM_sinsp_concatenate_paths_relative_path_median                +0.1450         +0.1450            41            47            41            47
BM_sinsp_concatenate_paths_relative_path_stddev                -0.0443         -0.0462             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_cv                    -0.1643         -0.1660             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_mean                     -0.0061         -0.0061            17            17            17            17
BM_sinsp_concatenate_paths_empty_path_median                   -0.0029         -0.0029            17            17            17            17
BM_sinsp_concatenate_paths_empty_path_stddev                   -0.2822         -0.2826             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_cv                       -0.2778         -0.2782             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_mean                  +0.1422         +0.1422            45            51            45            51
BM_sinsp_concatenate_paths_absolute_path_median                +0.1382         +0.1383            45            51            45            51
BM_sinsp_concatenate_paths_absolute_path_stddev                -0.7333         -0.7333             1             0             1             0
BM_sinsp_concatenate_paths_absolute_path_cv                    -0.7665         -0.7665             0             0             0             0
BM_sinsp_split_container_image_mean                            +0.0297         +0.0297           350           360           350           360
BM_sinsp_split_container_image_median                          +0.0319         +0.0319           350           361           350           361
BM_sinsp_split_container_image_stddev                          +0.5508         +0.5511             2             3             2             3
BM_sinsp_split_container_image_cv                              +0.5060         +0.5063             0             0             0             0

Copy link

codecov bot commented Sep 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.58%. Comparing base (b8c2d0d) to head (8c32de3).
Report is 57 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2079   +/-   ##
=======================================
  Coverage   73.58%   73.58%           
=======================================
  Files         253      253           
  Lines       31867    31867           
  Branches     5640     5649    +9     
=======================================
+ Hits        23449    23450    +1     
- Misses       8414     8416    +2     
+ Partials        4        1    -3     
Flag Coverage Δ
libsinsp 73.58% <ø> (+<0.01%) ⬆️

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.

@geraldcombs
Copy link
Contributor

Some of the files in userspace/libscap/engine/gvisor include paths that start with pkg/sentry, which is in gvisor/ports. Does this need $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>/ports as well? It looks fine to me otherwise.

@FedeDP
Copy link
Contributor

FedeDP commented Sep 25, 2024

/milestone TBD

@poiana poiana added this to the TBD milestone Sep 25, 2024
@FedeDP
Copy link
Contributor

FedeDP commented Oct 7, 2024

Hey @LucaGuerra is this still needed after #2005?

@LucaGuerra
Copy link
Contributor Author

I don't think we should integrate this into libs master. I can close this one.

@LucaGuerra LucaGuerra closed this Oct 7, 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.

4 participants