build(install): make install to common libs and include directory paths #247

terylt · 2022-03-09T23:58:29Z

Signed-off-by: TPT teryl.taylor@gmail.com

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area build

What this PR does / why we need it:

This PR installs libs, headers and dependencies into a common standard location when doing a make install. For example, the libs could be installed to:
/usr/libs/falcosecurity, /usr/include/falcosecurity, /usr/bin/falcosecurity. The install prefix can be changed. This feature is useful for consumers of the libs, as it makes it easier to find all libs and headers that need to be included to build an executable. It also puts the libs in a common standard location.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:
It installs libs, dependencies and headers to a common standard location.

`make install` installs libs to a common, standard location. Defaults too: `/usr/libs/falcosecurity`, `/usr/include/falcosecurity`, `/usr/bin/falcosecurity`

Signed-off-by: TPT <teryl.taylor@gmail.com>

poiana · 2022-03-09T23:58:37Z

Welcome @terylt! It looks like this is your first PR to falcosecurity/libs 🎉

poiana · 2022-03-09T23:58:37Z

Hi @terylt. Thanks for your PR.

I'm waiting for a falcosecurity member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

geraldcombs · 2022-03-10T20:47:36Z

Thanks for working on this! Have you considered using CMake's GnuInstallDirs module? It defines a bunch of variables that help ensure that you're installing to standard locations on Linux and other systems.

leogr · 2022-03-10T21:27:03Z

/ok-to-test

terylt · 2022-03-11T15:30:07Z

@geraldcombs Hi Gerald, thank you for your feedback! I did not know about this module. Similar functionality is built directly into the install() function in CMAKE but it is only available in new versions of cmake so I didn't use it. I mocked up a version with the GnuInstallDirs, and preliminarily, it looks like things get installed too:
/usr/libs/x86_64-linux-gnu/falcosecurity/
/usr/include/falcosecurity/
/usr/bin/falcosecurity/

If people prefer, I can update the PR with the GnuInstallDirs module.

Last question is on the bin/falcosecurity. Currently, the PR installs two things there.. protoc (protobuf compiler) and grpc_cpp_plugin. Not sure that these would be needed for an installation, but I kept them in for now. Open to comments on that.

geraldcombs · 2022-03-11T18:30:53Z

If people prefer, I can update the PR with the GnuInstallDirs module.

I think it would be beneficial in the long run. From a quick read of the module source it looks like it takes care of a lot of special cases that would otherwise have to be handled locally, e.g. Debian Multiarch and /usr/lib64 on other systems.

leogr · 2022-03-11T21:39:19Z

Hey @terylt

First, thank you for this PR! 🤩

If people prefer, I can update the PR with the GnuInstallDirs module.

I tend to agree with @geraldcombs , using a GnuInstallDirs would be preferable. If it's compatible with cmake 2.8.2, then I would go for it with no doubt.

Last question is on the bin/falcosecurity. Currently, the PR installs two things there.. protoc (protobuf compiler) and grpc_cpp_plugin. Not sure that these would be needed for an installation, but I kept them in for now. Open to comments on that.

Why do we need to install those components? If I remember correctly prooc and the ccp plugin are only required at build time. So there should be no need to install them. Am I missing something?

terylt · 2022-03-11T22:54:01Z

@leogr

Hi Leo, Thank you for the comments. I will remove the installed bins. I checked on GnuInstallDirs and according to cmake's change logs, it appears that the module was added in CMake 2.8.5 (https://cmake.org/files/v2.8/CMakeChangeLog-2.8.5). Are there system out there still using CMake 2.8.2, or can we bump the minimum requirement to 2.8.5?

leogr · 2022-03-14T14:19:10Z

@leogr

Hi Leo, Thank you for the comments. I will remove the installed bins. I checked on GnuInstallDirs and according to cmake's change logs, it appears that the module was added in CMake 2.8.5 (https://cmake.org/files/v2.8/CMakeChangeLog-2.8.5). Are there system out there still using CMake 2.8.2, or can we bump the minimum requirement to 2.8.5?

@terylt
Yes, I'm confident we can bump to any 2.8.x without issues 👍

…es. removed installation of protobuf bins. Signed-off-by: TPT <teryl.taylor@gmail.com>

terylt · 2022-03-14T21:53:12Z

Okay, pushed the changes to support GnuInstallDirs, pulled out the proto bins, and upgraded the min requirement for cmake to 2.8.5...

FedeDP

Apart from minor comments, it LGTM.
I will test this right now!

FedeDP · 2022-03-18T10:53:10Z

cmake/modules/curl.cmake

@@ -77,6 +77,9 @@ else()
 			BUILD_IN_SOURCE 1
 			BUILD_BYPRODUCTS ${CURL_LIBRARIES}
 			INSTALL_COMMAND "")
+		install(FILES "${CURL_LIBRARIES}" DESTINATION "${CMAKE_INSTALL_LIBDIR}/${LIBS_PACKAGE_NAME}/")
+		install(DIRECTORY "${CURL_INCLUDE_DIR}/curl" DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/${LIBS_PACKAGE_NAME}/"


I am just curious: in other cmake modules you added the trailing "/" to the INCLUDE variables; here instead it ends without the "/": ${CURL_INCLUDE_DIR}/curl.
Shouldn't this be: ${CURL_INCLUDE_DIR}/curl/?

FedeDP · 2022-03-18T10:53:39Z

cmake/modules/grpc.cmake

+		"${GRPC_LIB}" 
+		"${GRPCPP_LIB}"
+		"${GRPC_SRC}/libgrpc++_alts.a"
+	        "${GRPC_SRC}/libgrpc++_error_details.a"


Indentation :)

FedeDP · 2022-03-18T10:56:32Z

cmake/modules/jsoncpp.cmake

@@ -18,6 +18,7 @@ else()
 	set(JSONCPP_INCLUDE "${JSONCPP_SRC}")
 	set(JSONCPP_LIB_SRC "${JSONCPP_SRC}/jsoncpp.cpp")
 	message(STATUS "Using bundled jsoncpp in '${JSONCPP_SRC}'")
+	install(DIRECTORY "${JSONCPP_INCLUDE}/" DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/${LIBS_PACKAGE_NAME}/")


Why not using same fix as other cmake modules here, ie: adding the trailing slash to set(JSONCPP_INCLUDE ... line, for coherence?

FedeDP · 2022-03-18T10:58:07Z

cmake/modules/luajit.cmake

@@ -77,6 +77,9 @@ else()
 					UPDATE_COMMAND ""
 					INSTALL_COMMAND "")
 			endif()
+			install(FILES "${LUAJIT_LIB}" DESTINATION "${CMAKE_INSTALL_LIBDIR}/${LIBS_PACKAGE_NAME}/")
+			install(DIRECTORY "${LUAJIT_INCLUDE}/" DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/${LIBS_PACKAGE_NAME}/"


Why not using same fix as other cmake modules here, ie: adding the trailing slash to set(LUAJIT_INCLUDE ... line, for coherence?

FedeDP · 2022-03-18T10:59:11Z

cmake/modules/tbb.cmake

@@ -29,6 +29,9 @@ else()
 			BUILD_IN_SOURCE 1
 			BUILD_BYPRODUCTS ${TBB_LIB}
 			INSTALL_COMMAND "")
+		install(FILES "${TBB_LIB}" DESTINATION "${CMAKE_INSTALL_LIBDIR}/${LIBS_PACKAGE_NAME}/")
+		install(DIRECTORY "${TBB_INCLUDE_DIR}/tbb" DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/${LIBS_PACKAGE_NAME}/")


Same as curl cmake module comment applies. I don't understand whether is it a rule that DIRECTORY install shall end with slash or not!

FedeDP · 2022-03-18T10:59:40Z

cmake/modules/zlib.cmake

+	set(ZLIB_HEADERS "")
+	list(APPEND ZLIB_HEADERS
+		"${ZLIB_INCLUDE}/crc32.h"
+	        "${ZLIB_INCLUDE}/deflate.h"


Indentation :D

…s more consistent Signed-off-by: TPT <teryl.taylor@gmail.com>

terylt · 2022-03-18T20:37:48Z

Made some updates as per @FedeDP comments! Here is the comment from the CMAKE install() documentation that talks about the differences in trailing slashes. The trailing slashes were definitely the most difficult aspect of this PR so thanks for looking at them.

The last component of each directory name is appended to the destination directory but a trailing slash may be used to avoid this because it leaves the last component empty.

https://cmake.org/cmake/help/latest/command/install.html

Have a look and don't hesitate to ask for more changes!

FedeDP · 2022-03-21T09:31:02Z

Thanks! +1 from me!

leogr

/approve

Thank you!

poiana · 2022-03-21T09:31:07Z

LGTM label has been added.

Git tree hash: 4d27729d243f841711c5452ef2d1c32512c61f35

FedeDP

/approve

poiana · 2022-03-21T09:31:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, leogr, terylt

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:

~~OWNERS~~ [FedeDP,leogr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Some users of libs use its cmake files directly via an include() directive. By doing that, those users inherit any install() directives in the libs CMakeFiles. Prior to the changes in #247, libs did not actually have any install() directives, but now that it does, the users of libs end up getting libsinsp/libscap as well as all of its dependencies installed as well, which may not be what they want. To allow for both use cases, have install()s controlled by an option INSTALL_LIBS, which defaults to ON. Any users of libs that want to use the cmake files and that don't want anything installed can set this option to OFF. Signed-off-by: Mark Stemm <mark.stemm@gmail.com>

build(install): make install to common libs and include directory paths

bd91d3b

Signed-off-by: TPT <teryl.taylor@gmail.com>

poiana added release-note kind/feature New feature or request dco-signoff: yes area/build labels Mar 9, 2022

poiana added the needs-ok-to-test label Mar 9, 2022

poiana requested review from leodido and leogr March 9, 2022 23:58

poiana added the size/L label Mar 9, 2022

poiana added ok-to-test and removed needs-ok-to-test labels Mar 10, 2022

build(install): updated install scripts to use GNUInstallDirs variabl…

ded8df7

…es. removed installation of protobuf bins. Signed-off-by: TPT <teryl.taylor@gmail.com>

FedeDP reviewed Mar 18, 2022

View reviewed changes

build(install): fixed some indenting issues, and made trailing slashe…

48037a4

…s more consistent Signed-off-by: TPT <teryl.taylor@gmail.com>

leogr approved these changes Mar 21, 2022

View reviewed changes

poiana assigned leogr Mar 21, 2022

poiana added the lgtm label Mar 21, 2022

poiana added the approved label Mar 21, 2022

FedeDP approved these changes Mar 21, 2022

View reviewed changes

poiana assigned FedeDP Mar 21, 2022

poiana merged commit a281fb1 into falcosecurity:master Mar 21, 2022

araujof deleted the libs_build branch March 30, 2022 02:07

leogr mentioned this pull request Apr 21, 2022

build: components opt-in mechanism for packages falcosecurity/falco#1979

Merged

mstemm mentioned this pull request May 16, 2022

Make installing files during install configurable #338

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build(install): make install to common libs and include directory paths #247

build(install): make install to common libs and include directory paths #247

terylt commented Mar 9, 2022

poiana commented Mar 9, 2022

poiana commented Mar 9, 2022

geraldcombs commented Mar 10, 2022

leogr commented Mar 10, 2022

terylt commented Mar 11, 2022

geraldcombs commented Mar 11, 2022

leogr commented Mar 11, 2022

terylt commented Mar 11, 2022

leogr commented Mar 14, 2022 •

edited

Loading

terylt commented Mar 14, 2022

FedeDP left a comment

FedeDP Mar 18, 2022

FedeDP Mar 18, 2022

FedeDP Mar 18, 2022

FedeDP Mar 18, 2022

FedeDP Mar 18, 2022

FedeDP Mar 18, 2022

terylt commented Mar 18, 2022

FedeDP commented Mar 21, 2022

leogr left a comment

poiana commented Mar 21, 2022

FedeDP left a comment

poiana commented Mar 21, 2022

build(install): make install to common libs and include directory paths #247

build(install): make install to common libs and include directory paths #247

Conversation

terylt commented Mar 9, 2022

poiana commented Mar 9, 2022

poiana commented Mar 9, 2022

geraldcombs commented Mar 10, 2022

leogr commented Mar 10, 2022

terylt commented Mar 11, 2022

geraldcombs commented Mar 11, 2022

leogr commented Mar 11, 2022

terylt commented Mar 11, 2022

leogr commented Mar 14, 2022 • edited Loading

terylt commented Mar 14, 2022

FedeDP left a comment

Choose a reason for hiding this comment

FedeDP Mar 18, 2022

Choose a reason for hiding this comment

FedeDP Mar 18, 2022

Choose a reason for hiding this comment

FedeDP Mar 18, 2022

Choose a reason for hiding this comment

FedeDP Mar 18, 2022

Choose a reason for hiding this comment

FedeDP Mar 18, 2022

Choose a reason for hiding this comment

FedeDP Mar 18, 2022

Choose a reason for hiding this comment

terylt commented Mar 18, 2022

FedeDP commented Mar 21, 2022

leogr left a comment

Choose a reason for hiding this comment

poiana commented Mar 21, 2022

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Mar 21, 2022

leogr commented Mar 14, 2022 •

edited

Loading