sysdig icon indicating copy to clipboard operation
sysdig copied to clipboard

Wrong things in build / install system

Open abucodonosor opened this issue 7 years ago • 3 comments

Hi,

there are some things I found wrong while testing / packaging sysdig for Frugalware Linux:

  • 1 - sysdig/csysdig/sysdig-probe-loader are installed in $prefix/bin while these being 'root' bins only. So they need go to $prefix/sbin Or let in /bin/ ( even when wrong ) , add a udev rule for the kernel modules with a special group so users in that special group can work with sysdig.

  • 2 - bash completions does install in /etc/bash_completion.d which these days is not the location anymore but $sharedir/bash-completion/completions. So maybe a -DWITH_BASH_COMPLETIONS=path should be added.

  • 3 - BUILD_DRIVER set to OFF should mean to not build the driver but the driver should be still configured. Distributions may build it using Distribution build system for some_kernel(s) theys provide.

  • 4 - BUILD_DRIVER and ENABLE_DKMS both set to ON does not make any sense , does it ? I also think DKMS should be set to OFF anyway.

  • 5 - SYSDIG_VERSION should be set by you not from the users. Since one only find this by reading the cmake files most user builds have a 0.1.1dev version .. That's wrong

Regards

abucodonosor avatar Apr 04 '18 18:04 abucodonosor

Some more things:

In case of !Debug builds KBUILD_FLAGS is set to not-in-this-source existing SYSDIG_FEATURE_FLAGS so KBUILD_* is always empty. In case on Debug only debug flags gets set ofc bc same issue.

Your default CFLAGS eg: -O3 with ggdb does not make any sense. Here 3 issues ..

  1. -ggdb is Debug target flag should not be set in generic target
  2. -O3 hardcoded is not nice .. if you don't want Distributions to compile with own FLAGS then just don't allow that and define full set of flags
  3. if you really want make use of ggdbX then use with max -O2

This is wrong:

if(NOT DEFINED DIR_ETC)
        set(DIR_ETC "${CMAKE_INSTALL_PREFIX}/etc")
endif()

when not set should set to /etc , I think falco has that right.

abucodonosor avatar Apr 04 '18 22:04 abucodonosor

You may also consider to check the gcc version and add -fno-delete-null-pointer-checks for gcc >=6

abucodonosor avatar Apr 05 '18 00:04 abucodonosor

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions[bot] avatar Feb 23 '23 02:02 github-actions[bot]