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

chore(cmake/modules) Bump bundled GRPC version to 1.44.0 #222

Merged
merged 2 commits into from
Feb 23, 2022

Conversation

Molter73
Copy link
Contributor

What type of PR is this?

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

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

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

/area build

/area driver-kmod

/area driver-ebpf

/area libscap

/area libsinsp

/area tests

/area proposals

What this PR does / why we need it:

While running make sinsp using the bundled dependencies in a Fedora 35 container with GCC 11.2.1, compilation of the GRPC dependency was failing with the following message:

error: no matching function for call to ‘max(long int, int)’
[build] /workspaces/libs/build/grpc-prefix/src/grpc/third_party/abseil-cpp/absl/debugging/failure_signal_handler.cc: In function ‘bool absl::lts_20210324::SetupAlternateStackOnce()’:
[build] /workspaces/libs/build/grpc-prefix/src/grpc/third_party/abseil-cpp/absl/debugging/failure_signal_handler.cc:139:32: error: no matching function for call to ‘max(long int, int)’
[build]   139 |   size_t stack_size = (std::max(SIGSTKSZ, 65536) + page_mask) & ~page_mask;
[build]       |                        ~~~~~~~~^~~~~~~~~~~~~~~~~
[build] In file included from /usr/include/c++/11/algorithm:61,
[build]                  from /workspaces/libs/build/grpc-prefix/src/grpc/third_party/abseil-cpp/absl/debugging/failure_signal_handler.cc:36:
[build] /usr/include/c++/11/bits/stl_algobase.h:254:5: note: candidate: ‘template<class _Tp> const _Tp& std::max(const _Tp&, const _Tp&)’
[build]   254 |     max(const _Tp& __a, const _Tp& __b)
[build]       |     ^~~
[build] /usr/include/c++/11/bits/stl_algobase.h:254:5: note:   template argument deduction/substitution failed:
[build] /workspaces/libs/build/grpc-prefix/src/grpc/third_party/abseil-cpp/absl/debugging/failure_signal_handler.cc:139:32: note:   deduced conflicting types for parameter ‘const _Tp’ (‘long int’ and ‘int’)
[build]   139 |   size_t stack_size = (std::max(SIGSTKSZ, 65536) + page_mask) & ~page_mask;
[build]       |                        ~~~~~~~~^~~~~~~~~~~~~~~~~
[build] In file included from /usr/include/c++/11/algorithm:61,
[build]                  from /workspaces/libs/build/grpc-prefix/src/grpc/third_party/abseil-cpp/absl/debugging/failure_signal_handler.cc:36:
[build] /usr/include/c++/11/bits/stl_algobase.h:300:5: note: candidate: ‘template<class _Tp, class _Compare> const _Tp& std::max(const _Tp&, const _Tp&, _Compare)’
[build]   300 |     max(const _Tp& __a, const _Tp& __b, _Compare __comp)
[build]       |     ^~~
[build] /usr/include/c++/11/bits/stl_algobase.h:300:5: note:   template argument deduction/substitution failed:
[build] /workspaces/libs/build/grpc-prefix/src/grpc/third_party/abseil-cpp/absl/debugging/failure_signal_handler.cc:139:32: note:   deduced conflicting types for parameter ‘const _Tp’ (‘long int’ and ‘int’)
[build]   139 |   size_t stack_size = (std::max(SIGSTKSZ, 65536) + page_mask) & ~page_mask;
[build]       |                        ~~~~~~~~^~~~~~~~~~~~~~~~~
[build] In file included from /usr/include/c++/11/algorithm:62,
[build]                  from /workspaces/libs/build/grpc-prefix/src/grpc/third_party/abseil-cpp/absl/debugging/failure_signal_handler.cc:36:
[build] /usr/include/c++/11/bits/stl_algo.h:3461:5: note: candidate: ‘template<class _Tp> _Tp std::max(std::initializer_list<_Tp>)’
[build]  3461 |     max(initializer_list<_Tp> __l)
[build]       |     ^~~
[build] /usr/include/c++/11/bits/stl_algo.h:3461:5: note:   template argument deduction/substitution failed:
[build] /workspaces/libs/build/grpc-prefix/src/grpc/third_party/abseil-cpp/absl/debugging/failure_signal_handler.cc:139:32: note:   mismatched types ‘std::initializer_list<_Tp>’ and ‘long int’
[build]   139 |   size_t stack_size = (std::max(SIGSTKSZ, 65536) + page_mask) & ~page_mask;
[build]       |                        ~~~~~~~~^~~~~~~~~~~~~~~~~
[build] In file included from /usr/include/c++/11/algorithm:62,
[build]                  from /workspaces/libs/build/grpc-prefix/src/grpc/third_party/abseil-cpp/absl/debugging/failure_signal_handler.cc:36:
[build] /usr/include/c++/11/bits/stl_algo.h:3467:5: note: candidate: ‘template<class _Tp, class _Compare> _Tp std::max(std::initializer_list<_Tp>, _Compare)’
[build]  3467 |     max(initializer_list<_Tp> __l, _Compare __comp)
[build]       |     ^~~
[build] /usr/include/c++/11/bits/stl_algo.h:3467:5: note:   template argument deduction/substitution failed:
[build] /workspaces/libs/build/grpc-prefix/src/grpc/third_party/abseil-cpp/absl/debugging/failure_signal_handler.cc:139:32: note:   mismatched types ‘std::initializer_list<_Tp>’ and ‘long int’
[build]   139 |   size_t stack_size = (std::max(SIGSTKSZ, 65536) + page_mask) & ~page_mask;
[build]       |                        ~~~~~~~~^~~~~~~~~~~~~~~~~
[build] gmake[6]: *** [third_party/abseil-cpp/absl/debugging/CMakeFiles/absl_failure_signal_handler.dir/build.make:76: third_party/abseil-cpp/absl/debugging/CMakeFiles/absl_failure_signal_handler.dir/failure_signal_handler.cc.o] Error 1
[build] gmake[5]: *** [CMakeFiles/Makefile2:2001: third_party/abseil-cpp/absl/debugging/CMakeFiles/absl_failure_signal_handler.dir/all] Error 2
[build] gmake[5]: *** Waiting for unfinished jobs....

After doing some research, it looks like this has already been addressed by the abseil-cpp project and the change is present in GRPC's tag v1.44.0, in turn bumping to that version fixes the compilation error.

Dockerfile I'm using as a devcontainer just in case it is useful
FROM fedora:35

RUN dnf install -y \
    gcc \
    gcc-c++ \
    git \
    make \
    cmake \
    autoconf \
    automake \
    pkg-config \
    patch \
    ncurses-devel \
    libtool \
    elfutils-libelf-devel \
    diffutils \
    which \
    perl-core \
    clang && \
    kernel_version=$(uname -r) && \
    ln -s "/host/lib/modules/$kernel_version" "/lib/modules/$kernel_version" && \
    ln -s "/host/usr/src/kernels/$kernel_version" "/usr/src/kernels/$kernel_version"

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

/cc @leogr
/cc @FedeDP

Does this PR introduce a user-facing change?:

NONE

@poiana
Copy link
Contributor

poiana commented Feb 22, 2022

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

@poiana
Copy link
Contributor

poiana commented Feb 22, 2022

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

@poiana poiana added the size/XS label Feb 22, 2022
@FedeDP
Copy link
Contributor

FedeDP commented Feb 22, 2022

Hi @Molter73 ! Thanks for opening this PR!
Great catch too; weird that it didn't happen for me on Arch ;)
/ok-to-test

@FedeDP
Copy link
Contributor

FedeDP commented Feb 22, 2022

Mmmh CI fails with:

make[2]: *** No rule to make target 'grpc-prefix/src/grpc/third_party/abseil-cpp/absl/hash/libabsl_wyhash.a', needed by 'libsinsp/examples/sinsp-example'. Stop.

Can you try to build sinsp-example target in your container?

@Molter73
Copy link
Contributor Author

Looks like they replaced wyhash.a with low_level_hash.a in this commit: abseil/abseil-cpp@8910297

Let me try recompiling, I may have some residual binaries in my env or something. I'm pretty sure changing the dependency here will get rid of the error though.

@FedeDP
Copy link
Contributor

FedeDP commented Feb 22, 2022

Makes lot of sense!
Thanks for looking into it!

@FedeDP
Copy link
Contributor

FedeDP commented Feb 22, 2022

While you are at it, can you also double check all other "enforced" deps running, as advised, pkg-config --libs grpc++ ?

@Molter73
Copy link
Contributor Author

I've managed to get the same error locally and after fixing that dependency a bunch more unresolved dependencies showed up (because why wouldn't they?).

I'm trying to fix compilation and will upload the change ASAP

@Molter73
Copy link
Contributor Author

Ok, I'm in a bit of a pickle and could use a hand. I've run pkg-config --libs grpc++ and got the following output

[root@30692c2c01b9 build]# PKG_CONFIG_PATH=./grpc-prefix/src/grpc/target/lib/pkgconfig/:./grpc-prefix/src/grpc/third_party/re2/ pkg-config --libs grpc++
Package absl_base was not found in the pkg-config search path.
Perhaps you should add the directory containing `absl_base.pc'
to the PKG_CONFIG_PATH environment variable
Package 'absl_base', required by 'gpr', not found
Package 'absl_cord', required by 'gpr', not found
Package 'absl_core_headers', required by 'gpr', not found
Package 'absl_memory', required by 'gpr', not found
Package 'absl_optional', required by 'gpr', not found
Package 'absl_random_random', required by 'gpr', not found
Package 'absl_status', required by 'gpr', not found
Package 'absl_str_format', required by 'gpr', not found
Package 'absl_strings', required by 'gpr', not found
Package 'absl_synchronization', required by 'gpr', not found
Package 'absl_time', required by 'gpr', not found
Package 'openssl', required by 'grpc', not found
Package 'absl_base', required by 'grpc', not found
Package 'absl_bind_front', required by 'grpc', not found
Package 'absl_cord', required by 'grpc', not found
Package 'absl_core_headers', required by 'grpc', not found
Package 'absl_flat_hash_map', required by 'grpc', not found
Package 'absl_hash', required by 'grpc', not found
Package 'absl_inlined_vector', required by 'grpc', not found
Package 'absl_memory', required by 'grpc', not found
Package 'absl_optional', required by 'grpc', not found
Package 'absl_random_random', required by 'grpc', not found
Package 'absl_status', required by 'grpc', not found
Package 'absl_statusor', required by 'grpc', not found
Package 'absl_str_format', required by 'grpc', not found
Package 'absl_strings', required by 'grpc', not found
Package 'absl_synchronization', required by 'grpc', not found
Package 'absl_time', required by 'grpc', not found
Package 'absl_utility', required by 'grpc', not found
Package 'absl_variant', required by 'grpc', not found
Package 'absl_base', required by 'grpc++', not found
Package 'absl_bind_front', required by 'grpc++', not found
Package 'absl_cord', required by 'grpc++', not found
Package 'absl_core_headers', required by 'grpc++', not found
Package 'absl_flat_hash_map', required by 'grpc++', not found
Package 'absl_hash', required by 'grpc++', not found
Package 'absl_inlined_vector', required by 'grpc++', not found
Package 'absl_memory', required by 'grpc++', not found
Package 'absl_optional', required by 'grpc++', not found
Package 'absl_random_random', required by 'grpc++', not found
Package 'absl_status', required by 'grpc++', not found
Package 'absl_statusor', required by 'grpc++', not found
Package 'absl_str_format', required by 'grpc++', not found
Package 'absl_strings', required by 'grpc++', not found
Package 'absl_synchronization', required by 'grpc++', not found
Package 'absl_time', required by 'grpc++', not found
Package 'absl_utility', required by 'grpc++', not found
Package 'absl_variant', required by 'grpc++', not found

Removing duplicates and comparing the libraries to the ones listed in grpc.cmake I've found which of them are missing and added the following paths to GRPC_LIBRARIES:

"${GRPC_SRC}/third_party/abseil-cpp/absl/base/libabsl_core_headers.a"
"${GRPC_SRC}/third_party/abseil-cpp/absl/memory/libabsl_memory.a"
"${GRPC_SRC}/third_party/abseil-cpp/absl/types/libabsl_optional.a"
"${GRPC_SRC}/third_party/abseil-cpp/absl/random/libabsl_random_random.a"
"${GRPC_SRC}/third_party/abseil-cpp/absl/strings/libabsl_str_format.a"
"${GRPC_SRC}/third_party/abseil-cpp/absl/functional/libabsl_bind_front.a"
"${GRPC_SRC}/third_party/abseil-cpp/absl/container/libabsl_flat_hash_map.a"
"${GRPC_SRC}/third_party/abseil-cpp/absl/container/libabsl_inlined_vector.a"
"${GRPC_SRC}/third_party/abseil-cpp/absl/utility/libabsl_utility.a"
"${GRPC_SRC}/third_party/abseil-cpp/absl/types/libabsl_variant.a"

Problem is that none of those libraries are being built, even though they are listed as dependencies and are defined in the abseil cmakelists, am I missing something here?

I've also tried adding all .a files compiled under abseil to the list and that ended up failing to resolve a function, maybe I'm missing some configuration flag, I'll keep looking but any help is welcomed.

@FedeDP
Copy link
Contributor

FedeDP commented Feb 23, 2022

Hi @Molter73 ! That is interesting; well bumping gRPC is always a somewhat interesting experience :)
Perhaps we need to enable some new cmake options?

@FedeDP
Copy link
Contributor

FedeDP commented Feb 23, 2022

PS:

pkg-config --libs grpc++

You ran it using grpc.pc script from your build folder, right? Not the system one.

@Molter73
Copy link
Contributor Author

I ended up looking into the CMakeCache.txt found under grpc-prefix/src/grpc, that seems to list all the libraries needed as absl_*_LIB_DEPENDS and each entry shows what that library depends on, so I added all of the missing ones and got a little further. The downside now is that the order in which the libraries are listed seems to matter, so now I'll start the dance of trying-to-get-everything-in-the-right-order.

As for the pkg-config, yes, this is how I set the PKG_CONFIG_PATH (the command was run from libs/build):

PKG_CONFIG_PATH=./grpc-prefix/src/grpc/target/lib/pkgconfig/:./grpc-prefix/src/grpc/third_party/re2/

@FedeDP
Copy link
Contributor

FedeDP commented Feb 23, 2022

Top you are doing a superb job man! I am currently trying to build gRPC too on a fedora:35 container myself, trying to be more helpful!

@FedeDP
Copy link
Contributor

FedeDP commented Feb 23, 2022

gcc version 11.2.1 20220127 (Red Hat 11.2.1-9) (GCC)

It built with no problems by just bumping grpc version to 1.44.0 on fedora:35 container O.O

[100%] Linking CXX static library libsinsp.a
[100%] Built target sinsp

That's weird; i just updated the grpc cmake submodule and run make sinsp -j4.

EDIT:

diff --git a/cmake/modules/grpc.cmake b/cmake/modules/grpc.cmake
index ad385ed0..bcb6c56b 100644
--- a/cmake/modules/grpc.cmake
+++ b/cmake/modules/grpc.cmake
@@ -117,7 +117,7 @@ else()
PREFIX "${PROJECT_BINARY_DIR}/grpc-prefix"
DEPENDS openssl protobuf c-ares zlib
GIT_REPOSITORY https://github.com/grpc/grpc.git
-                       GIT_TAG v1.38.1
+                       GIT_TAG v1.44.0
GIT_SUBMODULES "third_party/abseil-cpp third_party/re2"
CMAKE_CACHE_ARGS
-DCMAKE_INSTALL_PREFIX:PATH=${GRPC_INSTALL_DIR}

@Molter73
Copy link
Contributor Author

I had the same thing happen to me yesterday, it is possible that some of the libraries from the previous version are still lingering and it gets compiled against those instead of the new ones, try removing the whole grpc directory.

I just managed to compile locally and trimmed back some libraries that ended up not being needed, I'm gonna try a full compilation from scratch to double check and I'll push the change if it succeeds.

@FedeDP
Copy link
Contributor

FedeDP commented Feb 23, 2022

I had the same thing happen to me yesterday, it is possible that some of the libraries from the previous version are still lingering and it gets compiled against those instead of the new ones, try removing the whole grpc directory.

This is already using a clean build/ folder; i will retry.

@FedeDP
Copy link
Contributor

FedeDP commented Feb 23, 2022

aaaand it worked again:

[100%] Linking CXX static library libsinsp.a
[100%] Built target sinsp

@Molter73
Copy link
Contributor Author

Oh wait! sinsp does compile, sinsp-example is the target that fails.

@FedeDP
Copy link
Contributor

FedeDP commented Feb 23, 2022

Uh that makes sense, thanks! Can reproduce it now.

@poiana poiana removed the size/XS label Feb 23, 2022
@poiana poiana added the size/S label Feb 23, 2022
@Molter73
Copy link
Contributor Author

I'm guessing poiana is broken, I see the same failure in a separate unrelated PR.

@FedeDP
Copy link
Contributor

FedeDP commented Feb 23, 2022

Yep, i am currently trying to fix it!

@FedeDP
Copy link
Contributor

FedeDP commented Feb 23, 2022

Btw from your PR i was able to build sinsp-example .
Thanks!

@Molter73
Copy link
Contributor Author

Cool, thanks for letting me know it has worked for you too!

@FedeDP
Copy link
Contributor

FedeDP commented Feb 23, 2022

/test build-libs

@FedeDP
Copy link
Contributor

FedeDP commented Feb 23, 2022

The build will be fixed once #226 is merged in master (and you rebase).
Sorry for the inconvenience!

@FedeDP
Copy link
Contributor

FedeDP commented Feb 23, 2022

@Molter73 Can you rebase on master? Thanks!

Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

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

/approve

@poiana
Copy link
Contributor

poiana commented Feb 23, 2022

LGTM label has been added.

Git tree hash: 592f1ef39bd12286eea4e91d5affd4e943e1d3a8

@poiana
Copy link
Contributor

poiana commented Feb 23, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, Molter73

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

Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

/approve

@poiana poiana merged commit dcf1dc9 into falcosecurity:master Feb 23, 2022
mattnite pushed a commit to mattnite/falcosecurity-libs that referenced this pull request Oct 3, 2023
…security#222)

An unusual syntax of the declaration of a constructor (`libsinps::events::set<T>()`) raises a warning when the compiler is gcc-11 (of the Agentino’s builder) and the build is done with C++20.
The warning is a blocker as we treat warnings as errors.
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.

4 participants