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

CMake project renaming, build improvements, and cleanups #41

Merged
merged 11 commits into from
Nov 17, 2021

Conversation

leogr
Copy link
Member

@leogr leogr commented May 11, 2021

What type of PR is this?

/kind cleanup
/kind feature
/kind documentation

Any specific area of the project related to this PR?

/area build
/area libscap
/area libsinsp

What this PR does / why we need it:

This PR follows up the OSS Libraries Contribution Plan by introducing the following changes:

  • rename the cmake project name to falcosecurity-libs
  • make the driver name (and some other parameters) configurable via cmake options
  • make libscap.patch no longer needed (the patch was required by Falco due to a different naming convention)
  • cleanup some old references in the comments

The changes above mainly focus on the build system. Other naming adjustments will be addressed by following PRs.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Here you can find the testing branch in Falco: https://github.com/falcosecurity/falco/tree/build/apply-libs-pr-41
N.B.: that testing branch is rebased on top of falcosecurity/falco#1671 to address an incompatible change introduced by f7029e2#diff-633539375239ac63e4a9e3358f0852d437b22e9c9f2c96de5e04e346b0b4784fL2074

TODOs:

  • rebase
  • use scap as default driver name
  • use falcosecurity-libs as cmake project name

Does this PR introduce a user-facing change?:

build(userspace/libscap): make probe name configurable, default to `scap`
build: rename CMake project name to `falcosecurity-libs`

@leogr leogr force-pushed the refactor/naming branch from c75586a to 591bfe2 Compare May 11, 2021 10:32
@leogr
Copy link
Member Author

leogr commented May 11, 2021

/retest

@leogr leogr force-pushed the refactor/naming branch from 591bfe2 to b48b03e Compare July 1, 2021 08:58
@poiana
Copy link
Contributor

poiana commented Jul 1, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leogr

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

@leogr leogr changed the title wip: refactor: correct naming and small improvements correct naming and small build improvements Jul 1, 2021
@leogr leogr changed the title correct naming and small build improvements wip: correct naming and small build improvements Jul 1, 2021
Copy link
Contributor

@gnosek gnosek left a comment

Choose a reason for hiding this comment

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

Honestly, I'd prefer a top-level PRODUCT_NAME (or something) variable that all the variants get generated from. It feels weird to tie fs paths and env var names to the name of a kernel module (that happens to be the string we want)

Do you have a companion sysdig PR?

@leogr
Copy link
Member Author

leogr commented Jul 1, 2021

Honestly, I'd prefer a top-level PRODUCT_NAME (or something) variable that all the variants get generated from. It feels weird to tie fs paths and env var names to the name of a kernel module (that happens to be the string we want)

I intended to make each name configurable (without imposing any prefix/suffix) and using the PROBE_NAME as a fallback to generate defaults when values are not provided.

The env var name is already configurable (except for my mistake, but I will fix it soon).
I can do a similar thing for the fs paths too.

@gnosek
Copy link
Contributor

gnosek commented Jul 1, 2021

Any chance you can add a cmake var for the module name (in /sys/modules)? Something like:

string(REPLACE "-" "_" PROBE_MODULE_NAME "${PROBE_NAME}")

(I'm pretty sure I got something wrong, ref: https://cmake.org/cmake/help/latest/command/string.html).

I know it doesn't change anything for falco (which doesn't use the -probe suffix) but it would let OSS Sysdig (and the commercial agent) keep working without risky changes to the prebuilt probes tooling

@leogr leogr requested a review from gnosek July 5, 2021 12:41
@leogr leogr force-pushed the refactor/naming branch from 7b46f30 to c4e86b4 Compare July 14, 2021 14:02
@leogr leogr changed the title wip: correct naming and small build improvements Cmake project renaming, build improvements, and cleanups Jul 22, 2021
@leogr leogr changed the title Cmake project renaming, build improvements, and cleanups CMake project renaming, build improvements, and cleanups Jul 22, 2021
@leogr
Copy link
Member Author

leogr commented Nov 15, 2021

/retest

@leogr leogr changed the title wip: CMake project renaming, build improvements, and cleanups CMake project renaming, build improvements, and cleanups Nov 15, 2021
FedeDP
FedeDP previously approved these changes Nov 15, 2021
leogr and others added 11 commits November 17, 2021 12:46
This patch was originally used in Falco to adapt probe naming.
Reference: https://github.com/falcosecurity/falco/blob/1653898f4f8f64f94c0729415190349c8305a40b/cmake/modules/falcosecurity-libs-repo/patch/libscap.patch

Co-authored-by: Lorenzo Fontana <[email protected]>
Co-Authored-By: Leonardo Di Donato <[email protected]>
Signed-off-by: Leonardo Grasso <[email protected]>
Co-authored-by: Grzegorz Nosek <[email protected]>
Signed-off-by: Leonardo Grasso <[email protected]>
Signed-off-by: Leonardo Grasso <[email protected]>
Signed-off-by: Leonardo Grasso <[email protected]>
@ldegio ldegio self-requested a review November 17, 2021 22:05
@poiana poiana merged commit a743aa4 into master Nov 17, 2021
@poiana poiana deleted the refactor/naming branch November 17, 2021 22:06
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.

7 participants