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

build: upgrade deps and cmakefiles improvements #30

Merged
merged 11 commits into from
May 16, 2021
Merged

Conversation

leogr
Copy link
Member

@leogr leogr commented Apr 6, 2021

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 upgrades c-ares, protobuf and gRPC versions. It also changes the way we build gRPC when deps are bundled. The building of protobuf and c-ares is not delegated anymore to gRPC.

This PR also includes other changes (some required by the gRPC upgrade, some are just small improvements):

  • switching from gRPC _usecure libs to default ones
  • openSSL configured with no-shared
  • c-ares configured with --disable-shared

Finally, other changes are included to easier the libraries integration into the Falco build system, see also falcosecurity/falco#1552

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Unfortunately, this PR introduces a workaround, see https://github.com/falcosecurity/libs/pull/30/files#diff-80c09a7dee6a7fbc4e1648084e71598c7e28978e00ff50ab9381e7b8e20ec8b8R49

Since this PR took longer than expected and is blocking falcosecurity/falco#1552 I'd prefer to fix that later.

Does this PR introduce a user-facing change?:

build: switching from gRPC `_usecure` libs to default ones
build: upgrade gRPC to 1.32.0 when bundled
build: upgrade protobuf to 3.13.0 when bundled
build: upgrade c-ares to 1.15.0 when bundled
build: openSSL configured with `no-shared` when bundled
build: curl configure without brotli

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.

Lots of changes here, I hoped for a bit more explanation :)

cmake/modules/grpc.cmake Outdated Show resolved Hide resolved
cmake/modules/grpc.cmake Outdated Show resolved Hide resolved
cmake/modules/grpc.cmake Outdated Show resolved Hide resolved
cmake/modules/openssl.cmake Show resolved Hide resolved
userspace/libsinsp/CMakeLists.txt Outdated Show resolved Hide resolved
cmake/modules/protobuf.cmake Show resolved Hide resolved
@leogr leogr requested a review from gnosek April 19, 2021 11:26
@poiana poiana added size/L and removed size/M labels Apr 27, 2021
@leogr leogr force-pushed the build/upgrade-deps branch from 278e3fe to 24e5613 Compare April 27, 2021 11:16
@leogr leogr changed the title build: upgrade deps wip: build: upgrade deps Apr 30, 2021
@leogr
Copy link
Member Author

leogr commented Apr 30, 2021

Update: I'm re-working this PR again

@leogr leogr force-pushed the build/upgrade-deps branch 7 times, most recently from ce60c9a to 86a473a Compare May 5, 2021 14:34
@leogr leogr changed the title wip: build: upgrade deps build: upgrade deps May 5, 2021
@leogr leogr mentioned this pull request May 6, 2021
6 tasks
@leogr
Copy link
Member Author

leogr commented May 6, 2021

This PR is ready for review
/cc @fntlnz @gnosek @leodido

leogr and others added 11 commits May 11, 2021 09:45
This commit also introduces other relevant changes:

- now using gRPC cmakefiles (that requires a workaround, see the comment)
- added support for gpr lib (likely needed by dependant, for example Falco)
- switched from `_unsecure` grpc libs to default ones

Signed-off-by: Leonardo Grasso <[email protected]>
Signed-off-by: Leonardo Grasso <[email protected]>
The deprecation annotation produced warnings when building. Since the warning is not really relevant to libs code and might be problematic for dependant projects (especially when a project uses warnings as errors, like Falco), we are just suppressing it by removing the annotation.

Co-authored-by: Grzegorz Nosek <[email protected]>
Signed-off-by: Leonardo Grasso <[email protected]>
@leogr leogr force-pushed the build/upgrade-deps branch from 86a473a to 8b290f0 Compare May 11, 2021 07:45
Copy link
Member

@leodido leodido left a comment

Choose a reason for hiding this comment

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

LGTM

I've tried out various combinations of builds.

Note:

when using make as a generator it can happen that (it seems because of OpenSSL) sinsp doesn't build at the first attempt, not able to reproduce this with ninja.

@poiana
Copy link
Contributor

poiana commented May 11, 2021

LGTM label has been added.

Git tree hash: 0c06397fb26ab3a437d6a0d858d3b24e62855d82

@leodido leodido mentioned this pull request May 11, 2021
1 task
@leogr
Copy link
Member Author

leogr commented May 11, 2021

/retest

@leogr leogr requested a review from ldegio May 12, 2021 06:43
@leogr leogr changed the title build: upgrade deps build: upgrade deps and cmakefiles improvements May 12, 2021
@leogr
Copy link
Member Author

leogr commented May 16, 2021

/approve

@poiana
Copy link
Contributor

poiana commented May 16, 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

@poiana poiana merged commit 70d9b31 into master May 16, 2021
@poiana poiana deleted the build/upgrade-deps branch May 16, 2021 11:13
@leogr leogr mentioned this pull request May 21, 2021
@poiana poiana mentioned this pull request Feb 22, 2022
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.

5 participants