fix: include paths when installed #1647
Conversation
poiana
added
release-note-none
dco-signoff: yes
kind/bug
| #include <libsinsp/eventformatter.h> | ||
|
|
||
| #include <libsinsp/include/sinsp_external_processor.h> |
There was a problem hiding this comment.
Apart from some re-ordering of the includes to put the standard ones at the end, the real change here is the removal of this libsinsp/include/... layer.
|
Thanks for this PR! |
|
/milestone 0.15.0 |
userspace/libsinsp/CMakeLists.txt
Outdated
| $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}> | ||
| $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../plugin> | ||
| $<INSTALL_INTERFACE:${CMAKE_INSTALL_PREFIX}/include/${LIBS_PACKAGE_NAME}> | ||
| $<INSTALL_INTERFACE:${CMAKE_INSTALL_PREFIX}/include/${LIBS_PACKAGE_NAME}/libsinsp> |
There was a problem hiding this comment.
@therealbobo Could you please check this? After your recent changes does it still make sense to have an include path that enters inside the libsinsp/ directory? I suppose it does if you want both these includes to work:
#include <libsinsp/sinsp.h> // modern way
#include <sinsp.h> // legacyIt would be nice to have a single approach, though.
There was a problem hiding this comment.
I have to do some more cleanups of my leftovers! But yes, I completely agree on that! Let's go with <libsinsp/*> 😄
There was a problem hiding this comment.
@therealbobo This PR has been stalling for a while, I have just rebased it to fix a conflict.
Are you expecting me to remove the support for the headers with no path (e.g. #include <sinsp.h>) in this PR? I can do that, but I need clear indication that it is ok and expected.
There was a problem hiding this comment.
Yes, exactly! I did a deep cleanup recently. I don't think that there's any header with a relative path.
federico-sysdig
force-pushed
the
fix-include-paths-for-install
branch
from
47c632b to
8cac3da
Compare
|
The Cflags sections in the pkg-config files (userspace/libscap/libscap.pc.in and userspace/libsinsp/libsinsp.pc.in) need to be updated, but LGTM otherwise. |
Thanks for catching this! Fixed. |
|
LGTM label has been added. Git tree hash: 0d81ed30338dd283b1fc95a19c2a78a803120208
|
federico-sysdig
force-pushed
the
fix-include-paths-for-install
branch
from
130954a to
d2d62cb
Compare
Signed-off-by: Federico Aponte <[email protected]>
Signed-off-by: Federico Aponte <[email protected]>
Signed-off-by: Federico Aponte <[email protected]>
federico-sysdig
force-pushed
the
fix-include-paths-for-install
branch
from
d2d62cb to
57cdb55
Compare
|
LGTM label has been added. Git tree hash: d16b037fb2793f17213bbc0a56ca521e45d70824
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Andreagit97, FedeDP, federico-sysdig, incertum 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 |
What type of PR is this?
/kind bug
/kind cleanup
Any specific area of the project related to this PR?
/area build
Does this PR require a change in the driver versions?
No
What this PR does / why we need it:
Fix a couple of issues with the include headers.
include/falcosecurity/has extra directories of directory layers that are not necessary, such asuserspace/and another unneededinclude/. There might be other issues, but these two were the more evident ones and they are now fixed.$<BUILD_INTERFACE:...>,$<INSTALL_INTERFACE:...>) allow to do that.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: