fix(build): update grpc cmake module to use find_package() when building using system library #144

FedeDP · 2021-11-23T16:19:49Z

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

Co-authored-by: Leonardo Grasso <me@leonardograsso.com>

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

Any specific area of the project related to this PR?

/area build

What this PR does / why we need it:
It allows to build falco linking system grpc.

Which issue(s) this PR fixes:

Fixes #99

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

fix(build): fix build without bundled deps on system with newer grpc versions

…ing using system library. This fixes the linking of libraries at the end of Falco build. Signed-off-by: Federico Di Pierro <nierro92@gmail.com> Co-authored-by: Leonardo Grasso <me@leonardograsso.com>

poiana · 2021-11-23T16:19:58Z

Hi @FedeDP. 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.

…cosecurity/libs#144. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

leogr · 2021-11-23T17:20:43Z

/ok-to-test

…make module is not found on system. Ubuntu focal is not shipping the module. Avoid breaking CI and builds on it. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

leogr

/approve

👍

poiana · 2021-11-25T13:15:53Z

LGTM label has been added.

Git tree hash: b8f2c6f38667c50e502cc7f22ebf1f9b380aab0d

gnosek

If we need the fallback anyway, does the find_package approach actually help with anything? i.e. are there environments where find_package works while find_library doesn't?

cmake/modules/grpc.cmake

gnosek · 2021-11-25T17:42:51Z

cmake/modules/grpc.cmake

+		find_path(GRPCXX_INCLUDE NAMES grpc++/grpc++.h)
+		if(NOT GRPCXX_INCLUDE)
+			find_path(GRPCPP_INCLUDE NAMES grpcpp/grpcpp.h)
+			add_definitions(-DGRPC_INCLUDE_IS_GRPCPP=1)
+		endif()


Do we want to pass PATHS ${GRPC_INCLUDE} (or something that uses the pkg_config data) to the find_path calls here?

I think we might; but that was there before my PR and i didn't want to touch it (gRPC cmake is a bit like the ultimate boss to fight against! Now i unlocked holiness badge :P)
I mean, that checking code is the same as before.
Btw we could adjust it, if you think it is needed :)

yeah, the grpc build scripts are on a whole different level :D

my reasoning for the requested change is:

the old code always did find_library etc. without any hints so only searched the default paths, whatever they are

the new code can potentially find grpc elsewhere, but we do:

get the GRPC_INCLUDE property from the grpc cmake module (good)

look for grpc++/grpc++.h in the default locations (bad)

if not found, look for grpcpp/grpcpp.h in the default locations again (bad too)

so, this new code appears more flexible but in the end still requires the grpc headers to live in the default location

... or I'm completely wrong :D

Nope! The problem was that the old code assumed that grpc libraries had built-in all their deps, ie: statically linked.
The new code instead is able to correctly use a grpc that dynamically links its dependencies, while still managing statically linked one.

The old case was left still because we found out that find_package(gRPC) can fail on some distro because gRPC cmake config module is not installed.

To summarize:

package found -> both dynamically and statically linked gRPC work fine

package not found -> current libs master behavior

the new code can potentially find grpc elsewhere, but we do:

BTW you are right, i will update that!

Note however that

find_path(GRPCXX_INCLUDE NAMES grpc++/grpc++.h) if(NOT GRPCXX_INCLUDE) find_path(GRPCPP_INCLUDE NAMES grpcpp/grpcpp.h) add_definitions(-DGRPC_INCLUDE_IS_GRPCPP=1) endif()

is just used to set the GRPC_INCLUDE_IS_GRPCPP define ;)

Yes, but that assumes that grpc++/grpc++.h can be found in the default include paths.

If GRPC_INCLUDE is non-standard, we'll still look for grpc++.h in the default locations, so we won't find it if it exists in e.g. /opt/grpc/include/grpc++.h` (where the grpc cmake pointed us to).

If we simply add PATHS ${GRPC_INCLUDE} to the find_path invocation, we should be good (I think).

the new code can potentially find grpc elsewhere, but we do:

Should be fixed now! Thanks for your input!

gnosek · 2021-11-25T17:46:05Z

cmake/modules/grpc.cmake

+		set(GPR_LIB gRPC::gpr)
+		set(GRPC_LIB gRPC::grpc)
+		set(GRPCPP_LIB gRPC::grpc++)


What does this do? Does it want to link with -lgRPC::gpr or is it some cmake syntax I'm not aware of that pulls the values from the find_package result?

gRPC::* are namespaced targets names exported by find_package (those targets are usually defined in /usr/lib/cmake/grpc/gRPCTargets.cmake). They can be used as items in target_link_libraries and are automatically resolved.

To be more precise, the documentation says that an item can be:

A library target name: The generated link line will have the full path to the linkable library file associated with the target. The buildsystem will have a dependency to re-link if the library file changes.

Regarding the namespaced targets also adds:

Items containing ::, such as Foo::Bar, are assumed to be IMPORTED or ALIAS library target names and will cause an error if no such target exists.

gnosek · 2021-11-25T17:53:19Z

Okay, I actually read the linked issue ;) So AIUI, gRPC can have some extra dependencies we don't know about so we don't link them. The patch looks mostly fine but please see the comments (for my education, mostly :D).

leogr · 2021-11-26T07:59:03Z

Hey @gnosek

i.e. are there environments where find_package works while find_library doesn't?

Yes, there are.
Basically, if /usr/lib/cmake/grpc/gRPCTargets.cmake is present in your distribution find_package will work but find_library not. If it is not present the vice-versa applies.

FedeDP · 2021-11-26T08:17:11Z

Basically, if /usr/lib/cmake/grpc/gRPCTargets.cmake is present in your distribution find_package will work but find_library not. If it is not present the vice-versa applies.

Well, find_library will work all the time; the issue is that it is not able to provide a list of gRPC deps, thus we would fail at the linking stage if gRPC is not statically linked.
find_package does instead include all the required deps.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com> Co-authored-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

FedeDP · 2021-11-26T08:23:10Z

Force-pushed to fix dco (i always get wrong github "commit suggestion" ...)

leogr · 2021-11-26T09:09:03Z

/test build-libs

…path hint. Signed-off-by: Federico Di Pierro <nierro92@gmail.com> Co-authored-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

leogr

/approve

👏

poiana · 2021-11-29T16:09:56Z

LGTM label has been added.

Git tree hash: 5fe6f6a132b61e66cbbda08fb3b1853df89c4873

poiana · 2021-11-29T16:10:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, 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:

~~OWNERS~~ [leogr]

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

gnosek

Thanks!

…cosecurity/libs#144. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

fix(build): update grpc cmake module to use find_package() when build…

4b64c71

…ing using system library. This fixes the linking of libraries at the end of Falco build. Signed-off-by: Federico Di Pierro <nierro92@gmail.com> Co-authored-by: Leonardo Grasso <me@leonardograsso.com>

poiana added release-note-none kind/bug Something isn't working dco-signoff: yes area/build labels Nov 23, 2021

poiana requested review from ldegio and mstemm November 23, 2021 16:19

poiana added the needs-ok-to-test label Nov 23, 2021

poiana added the size/M label Nov 23, 2021

FedeDP added a commit to FedeDP/falco that referenced this pull request Nov 23, 2021

update(build): updated libs pointing to my repo to correctly test fal…

f784b30

…cosecurity/libs#144. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

FedeDP mentioned this pull request Nov 23, 2021

update(build): update libs to fix build with grpc system libraries falcosecurity/falco#1804

Closed

FedeDP added a commit to FedeDP/falco that referenced this pull request Nov 23, 2021

update(build): updated libs pointing to my repo to correctly test fal…

9830ee8

…cosecurity/libs#144. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

FedeDP added a commit to FedeDP/falco that referenced this pull request Nov 23, 2021

update(build): updated libs pointing to my repo to correctly test fal…

d2c8d96

…cosecurity/libs#144. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

poiana added ok-to-test and removed needs-ok-to-test labels Nov 23, 2021

update(build): fallback to manually find libraries when grpc config c…

4cec20a

…make module is not found on system. Ubuntu focal is not shipping the module. Avoid breaking CI and builds on it. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

FedeDP mentioned this pull request Nov 24, 2021

Sysdig fails to build with grpc 1.41 draios/sysdig#1778

Closed

poiana added release-note and removed release-note-none labels Nov 25, 2021

leogr previously approved these changes Nov 25, 2021

View reviewed changes

poiana assigned leogr Nov 25, 2021

poiana added the lgtm label Nov 25, 2021

poiana added the approved label Nov 25, 2021

gnosek reviewed Nov 25, 2021

View reviewed changes

FedeDP dismissed leogr’s stale review via 8f911eb November 26, 2021 08:15

poiana added dco-signoff: no and removed lgtm dco-signoff: yes labels Nov 26, 2021

Update cmake/modules/grpc.cmake

cfdc21d

Signed-off-by: Federico Di Pierro <nierro92@gmail.com> Co-authored-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

FedeDP force-pushed the build_w_system_libs branch from 8f911eb to cfdc21d Compare November 26, 2021 08:22

poiana added dco-signoff: yes and removed dco-signoff: no labels Nov 26, 2021

fix(build): check GRPC_INCLUDE_IS_GRPCPP macro using GRPC_INCLUDE as …

6f94b93

…path hint. Signed-off-by: Federico Di Pierro <nierro92@gmail.com> Co-authored-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

leogr approved these changes Nov 29, 2021

View reviewed changes

poiana added the lgtm label Nov 29, 2021

leogr requested a review from gnosek November 30, 2021 12:04

gnosek approved these changes Nov 30, 2021

View reviewed changes

poiana assigned gnosek Nov 30, 2021

poiana merged commit 8808a06 into falcosecurity:master Nov 30, 2021

FedeDP deleted the build_w_system_libs branch November 30, 2021 13:04

FedeDP added a commit to FedeDP/falco that referenced this pull request Dec 3, 2021

update(build): updated libs pointing to my repo to correctly test fal…

58940c9

…cosecurity/libs#144. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(build): update grpc cmake module to use find_package() when building using system library #144

fix(build): update grpc cmake module to use find_package() when building using system library #144

FedeDP commented Nov 23, 2021 •

edited

Loading

poiana commented Nov 23, 2021

leogr commented Nov 23, 2021

leogr left a comment

poiana commented Nov 25, 2021

gnosek left a comment

gnosek Nov 25, 2021

FedeDP Nov 26, 2021

gnosek Nov 29, 2021

FedeDP Nov 29, 2021

FedeDP Nov 29, 2021

gnosek Nov 29, 2021

FedeDP Nov 29, 2021

gnosek Nov 25, 2021

leogr Nov 26, 2021

gnosek commented Nov 25, 2021

leogr commented Nov 26, 2021 •

edited

Loading

FedeDP commented Nov 26, 2021

FedeDP commented Nov 26, 2021

leogr commented Nov 26, 2021

leogr left a comment

poiana commented Nov 29, 2021

poiana commented Nov 29, 2021

gnosek left a comment

fix(build): update grpc cmake module to use find_package() when building using system library #144

fix(build): update grpc cmake module to use find_package() when building using system library #144

Conversation

FedeDP commented Nov 23, 2021 • edited Loading

poiana commented Nov 23, 2021

leogr commented Nov 23, 2021

leogr left a comment

Choose a reason for hiding this comment

poiana commented Nov 25, 2021

gnosek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnosek commented Nov 25, 2021

leogr commented Nov 26, 2021 • edited Loading

FedeDP commented Nov 26, 2021

FedeDP commented Nov 26, 2021

leogr commented Nov 26, 2021

leogr left a comment

Choose a reason for hiding this comment

poiana commented Nov 29, 2021

poiana commented Nov 29, 2021

gnosek left a comment

Choose a reason for hiding this comment

FedeDP commented Nov 23, 2021 •

edited

Loading

leogr commented Nov 26, 2021 •

edited

Loading