refactor libsinsp sub-components #14
Conversation
leogr
force-pushed
the
refactor/sub-components-2
branch
from
d0b5298 to
1ff71fd
Compare
leogr
force-pushed
the
refactor/sub-components-2
branch
3 times, most recently
from
d31504c to
85800dc
Compare
leogr
changed the title
This commit assumes those sub-components will be ported back to draios/sysdig Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
Note about `CHISELS_INSTALLATION_DIR`: Only the library consumer uses it, so we can safely move it away. Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
leogr
force-pushed
the
refactor/sub-components-2
branch
from
85800dc to
e73587d
Compare
| if(WITH_CHISEL) | ||
| list(APPEND SINSP_SOURCES | ||
| ../chisel/chisel_api.cpp | ||
| ../chisel/chisel_fields_info.cpp | ||
| ../chisel/chisel_utils.cpp | ||
| ../chisel/chisel.cpp | ||
| ../chisel/lua_parser_api.cpp | ||
| ../chisel/lua_parser.cpp) | ||
| endif() |
There was a problem hiding this comment.
Could this be a separate libsinspchisel.a? We'd lose one build-time variable for libsinsp, at least
There was a problem hiding this comment.
Short answer
it could be, but it's outside the scope of this PR :)
Long answer
The main goal of this PR is to reduce the external dependencies by moving out those sub-components (which should not part of libsinsp by default). When doing that, I've realized there are a lot of further improvements that can be done, but the set of modifications required just to move them away was already too big. In particular, I have noticed they are tightly coupled with libsinsp internals, most of the implementation relies on friend classes, unfortunately. So I decided to keep things simple and reduce the scope of this PR as minimal as possible (that's XXL size anyways). Basically, you should think about this PR as a baby step.
For those reasons, I really think that we should first review the implementation and then make libsinspchisel.a a thing, eventually.
|
LGTM label has been added. Git tree hash: 57d6db1212541bed93e5c611eb054c4a735b91f3
|
|
/hold |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gnosek, leodido 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 |
|
/hold cancel |
See: - falcosecurity/libs#14 - #1747 Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
What type of PR is this?
/kind cleanup
Any specific area of the project related to this PR?
/area build
/area libsinsp
What this PR does / why we need it:
This PR aims to refactor 2 libsinp sub-components in the following way:
ncursesdependencyuserspace/chisel) and its inclusion is controlled by a cmake optionWITH_CHISEL(disabled by default)N.B.:
Those sub-components rely on several extension points that were implemented by using the
friend classmechanism. The extension points are untouched, refactoring them is outside the scope of this PR.Special notes for your reviewer:
We will need to introduce a new CI job to build the libraries with
-DWITH_CHISEL=TrueDoes this PR introduce a user-facing change?: