Cross compiling fixes and improvements #59

eigendude · 2020-03-07T23:58:01Z

Description

This PR contains a set of fixes and improvements to the build system to allow for cross-compiling, and then applies several code fixes needed for newer GCC versions.

Structure of the PR

The PR is organized as follows:

Improvements needed for cross-compiling
Build fixes

Improvements for cross-compiling

Commits:

These changes enable more control over the build process when embedding in another build system.

CMake is controlled by user variables; the first one we need is -DHUNTER_ENABLED=OFF so that we can have control over dependencies.

Another useful variable is -DTESTING=OFF, because embedding test infra drastically increases the up-front effort. The first commit above allows this variable to exclude test infra from the dependency discovery in addition to the build.

Finally, I added two variables to allow the build system to provide the protobuf compiler and headers:

-DProtobuf_PROTOC_EXECUTABLE=$(NATIVEPREFIX)/bin/protoc
-DProtobuf_INCLUDE_DIR=$(PREFIX)/include

We might also want to disable -Werror for examples, but I haven't done that here.

Build fixes

Commits:

These fixes should be evident by visual inspection. Error log included in the commit messages.

How has this been tested?

With these changes in place, I'm able to compile and link libp2p and dependencies using two build systems: OpenEmbedded, and Kodi's depends system.

Here's my work on these two build systems. Commits are structured so that libp2p inclusion can be upstreamed.

Additions for OpenEmbedded (meta-cryptocurrency layer)

Commits:

libp2p: Import C++ implementation of libp2p

This commit enables libp2p support in the OpenEmbedded ecosystem.

Additions for Kodi

Commits for C++17 support:

Kodi currently uses C++14. These 3 commits can be PR'd to enable C++17 support, needed for libp2p and filecoin. Merging is probably a long way off, as Kodi supports a long tail of devices with embedded build systems that often use old compilers.

Commits for the depends build system:

With the commits in this PR, I was able to cross-compile libp2p for x86_64 Linux and macOS for runtime testing. Next is to test compilation for Raspberry Pi and Android NDK.

Commits for runtime testing:

testing libp2p: echo, kademlia and gossip

I didn't spend a lot of time on kademlia, but echo and gossip test code produces promising output. Tested on Linux x86_64 and macOS.

Benefits

IoT is the obvious goal, but Kodi opens up another little-known area of embedded: OTT (over-the-top), stretching across both set-top boxes and almost all mobile

Related PRs

This PR is needed for filecoin-project/cpp-filecoin#114.

igor-egorov

Many thanks for your contribution!

The only change I request - we have to decide what to do with commented out lines in cmake/functions.cmake

igor-egorov · 2020-03-10T12:58:49Z

cmake/functions.cmake

@@ -45,8 +45,8 @@ function(add_flag flag)
 endfunction()

 function(compile_proto_to_cpp PROTO_LIBRARY_NAME PB_H PB_CC PROTO)
-  get_target_property(Protobuf_INCLUDE_DIR protobuf::libprotobuf INTERFACE_INCLUDE_DIRECTORIES)
-  get_target_property(Protobuf_PROTOC_EXECUTABLE protobuf::protoc IMPORTED_LOCATION_RELEASE)
+  #get_target_property(Protobuf_INCLUDE_DIR protobuf::libprotobuf INTERFACE_INCLUDE_DIRECTORIES)


Our rule of thumb is that the commented code has to be either uncommented or removed.

Please check this. Probably the commented lines could be conditionally enabled (for example, depending on some configuration variable).

Right, I don't intend to leave commented code - commit is marked [WIP]. They should probably be conditionally enabled, but on what condition? They broke my cross compile attempt, so maybe we switch on cross compiling? I don't think we should expose a CMake option.

Do you know why these lines were added in the first place?

What I see here - in these two lines we are reading INTERFACE_INCLUDE_DIRECTORIES and IMPORTED_LOCATION_RELEASE parameters, which have to be set by find_package(Protobuf ...) command.

The error from the commit message says - it was not succeeded to find a proto compiler in your environment. Probably you can additionally specify own custom CMake toolchain file which will set_target_property(...). Otherwise, it looks like proto couldn't be compiled without a compiler :)

We could employ CMAKE_CROSSCOMPILING var:

if (NOT CMAKE_CROSSCOMPILING) get_target_property(Protobuf_INCLUDE_DIR protobuf::libprotobuf INTERFACE_INCLUDE_DIRECTORIES) get_target_property(Protobuf_PROTOC_EXECUTABLE protobuf::protoc IMPORTED_LOCATION_RELEASE) endif()

BUT, the variables still have to be defined in another way.

I tackled this the following way:

CMake: Allow build system to provide protobuf compiler

igor-egorov · 2020-03-10T13:02:38Z

src/common/literals.cpp

 namespace libp2p::common {
  libp2p::common::Hash256 operator""_hash256(const char *c, size_t s) {
    libp2p::common::Hash256 hash{};
-    std::copy_n(c, std::min(s, 32ul), hash.rbegin());
+    std::copy_n(c, std::min(s, static_cast<size_t>(32ul)), hash.rbegin());


just a minor note.
It looks like l type specifier for constants can be omitted due to explicit static_cast.

Should I drop the u type specifier too?

u could still be kept as an indicator of the value's "nature".
That is not an issue at all. We may leave it as is even with l.

I removed the l and left the u. this leaves the indicator of its "nature", and de-duplicates the notion of its "size".

imho, casting to the same type explicitly looks more clear.

igor-egorov · 2020-03-10T13:09:42Z

src/protocol/gossip/impl/remote_subscriptions.cpp

@@ -10,6 +10,9 @@
 #include <libp2p/protocol/gossip/impl/connectivity.hpp>
 #include <libp2p/protocol/gossip/impl/message_builder.hpp>

+#define SPDLOG_TRACE_ON


The fact you had to define the macro is looking suspicious

not sure if needed, or resulted from grasping at straws...

spd was a headache and I had to comment random log lines just to get libp2p to link. I'll put some more time into this.

Please check if it works when the macro is not defined.

I dropped the commit entirely.

igor-egorov · 2020-03-16T11:06:07Z

An update: it seems illegal to remove CONFIG from find_package commands :(

eigendude · 2020-03-17T20:10:13Z

Hm, CONFIG requires a cmake config file, but it seems this is provided by hunter, whereas cmake natively provides a find module. how would we find packages without hunter?

eigendude · 2020-03-18T03:59:05Z

I think I figured it out. I'm re-approaching without removing CONFIG.

eigendude · 2020-03-18T12:10:03Z

I've updated the PR. All CONFIG changes were dropped. Instead, I updated my build system to use the hunterized repos. The down side is that these don't seem to be fully up to date.

I tested cross compiling to Android NDK with clang. Only change needed was a compiler warning fix, which I've added.

eigendude · 2020-03-22T05:34:18Z

Any word? I compiled libp2p into an NDK app (kodi) and ran echo and kademlia examples successfully on Android.

masterjedy · 2020-04-13T11:45:30Z

Any word? I compiled libp2p into an NDK app (kodi) and ran echo and kademlia examples successfully on Android.

sounds great

This change shuffles the order of several CMake steps to expose CMake options to included files. By defining options before inclusion, included files are able to access the input provided by the build system.

When building without Hunter, it is desirable to pass the path to protoc and the protobuf include directory from the build system. Allow these variables to be overridden. Fixes the error: | CMake Error at cmake/functions.cmake:52 (message): | Protobuf_PROTOC_EXECUTABLE is empty | Call Stack (most recent call first): | cmake/functions.cmake:96 (compile_proto_to_cpp) | src/crypto/protobuf/CMakeLists.txt:6 (add_proto_library)

Error was: | literals.cpp: In function 'libp2p::common::Hash256 libp2p::common::operator""_hash256(const char*, size_t)': | literals.cpp:17:36: error: no matching function for call to 'min(size_t&, long unsigned int)' | 17 | std::copy_n(c, std::min(s, 32ul), hash.rbegin()); | | ^

Error was: | literals.cpp: In function 'libp2p::common::Hash256 libp2p::common::operator""_hash256(const char*, size_t)': | literals.cpp:19:36: error: no matching function for call to 'min(size_t&, long unsigned int)' | 19 | std::copy_n(c, std::min(s, 32ul), hash.rbegin()); | | ^

When compiling with -Werror, this causes the build to fail. Warning was: | yamux_frame.cpp:103:28: error: comparison of integers of different signs: 'gsl::span::index_type' (aka 'int') and 'const uint32_t' (aka 'const unsigned int') [-Werror,-Wsign-compare] | if (frame_bytes.size() < YamuxFrame::kHeaderLength) { | ~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~

igor-egorov · 2020-09-13T18:37:27Z

wow, this pr is still alive :) queued for check

eigendude · 2020-09-15T10:00:32Z

yup, the PR lives. I started a new project, embedded Interplanetary Identifiers, to see if an identity solution could solve the problem I'm working on. In the course of implementing IPIDs I rebased this PR and run-time tested it again.

FlorianFranzen · 2021-03-29T15:41:19Z

@eigendude; Thank you for this contribution. I opened a PR in qdrvm/kagome#703 to apply the relevant patches to kagome as well.

eigendude · 2021-03-31T20:09:58Z

@FlorianFranzen thanks!

eigendude force-pushed the cross-compile branch 2 times, most recently from 69bf0c5 to f9ab34f Compare March 8, 2020 02:24

This was referenced Mar 8, 2020

Cross compiling fixes, improvements and additions filecoin-project/cpp-filecoin#128

Merged

Embedded Filecoin: Enable cross-compiling filecoin-project/cpp-filecoin#114

Open

eigendude force-pushed the cross-compile branch from f9ab34f to ec60132 Compare March 8, 2020 22:35

igor-egorov self-requested a review March 10, 2020 12:47

igor-egorov requested changes Mar 10, 2020

View reviewed changes

igor-egorov requested a review from masterjedy March 12, 2020 12:14

eigendude force-pushed the cross-compile branch from ec60132 to b6ab1c0 Compare March 18, 2020 12:01

masterjedy approved these changes Apr 13, 2020

View reviewed changes

eigendude force-pushed the cross-compile branch from b6ab1c0 to 43e7f97 Compare July 10, 2020 21:33

eigendude added 6 commits September 11, 2020 11:44

CMake: Expose CMake options to includes

c53767b

This change shuffles the order of several CMake steps to expose CMake options to included files. By defining options before inclusion, included files are able to access the input provided by the build system.

CMake: Exclude test dependencies when building tests

3489d71

eigendude force-pushed the cross-compile branch from 43e7f97 to 0ecc452 Compare September 11, 2020 18:47

igor-egorov requested review from igor-egorov and kamilsa September 13, 2020 18:37

igor-egorov approved these changes Nov 18, 2020

View reviewed changes

kamilsa merged commit 0ba4a26 into libp2p:master Nov 26, 2020

eigendude deleted the cross-compile branch November 30, 2020 22:58

FlorianFranzen mentioned this pull request Mar 29, 2021

CMake: Allow build system to provide protobuf compiler qdrvm/kagome#703

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cross compiling fixes and improvements #59

Cross compiling fixes and improvements #59

eigendude commented Mar 7, 2020 •

edited

Loading

igor-egorov left a comment

igor-egorov Mar 10, 2020

eigendude Mar 10, 2020

igor-egorov Mar 11, 2020

eigendude Mar 18, 2020

igor-egorov Mar 10, 2020

eigendude Mar 10, 2020

igor-egorov Mar 11, 2020

eigendude Mar 18, 2020

masterjedy Apr 13, 2020

igor-egorov Mar 10, 2020

eigendude Mar 10, 2020

igor-egorov Mar 11, 2020

eigendude Mar 18, 2020

igor-egorov commented Mar 16, 2020

eigendude commented Mar 17, 2020

eigendude commented Mar 18, 2020

eigendude commented Mar 18, 2020 •

edited

Loading

eigendude commented Mar 22, 2020

masterjedy commented Apr 13, 2020

igor-egorov commented Sep 13, 2020

eigendude commented Sep 15, 2020

FlorianFranzen commented Mar 29, 2021 •

edited

Loading

eigendude commented Mar 31, 2021

Cross compiling fixes and improvements #59

Cross compiling fixes and improvements #59

Conversation

eigendude commented Mar 7, 2020 • edited Loading

Description

Structure of the PR

Improvements for cross-compiling

Build fixes

How has this been tested?

Additions for OpenEmbedded (meta-cryptocurrency layer)

Additions for Kodi

Benefits

Related PRs

igor-egorov 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

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

igor-egorov commented Mar 16, 2020

eigendude commented Mar 17, 2020

eigendude commented Mar 18, 2020

eigendude commented Mar 18, 2020 • edited Loading

eigendude commented Mar 22, 2020

masterjedy commented Apr 13, 2020

igor-egorov commented Sep 13, 2020

eigendude commented Sep 15, 2020

FlorianFranzen commented Mar 29, 2021 • edited Loading

eigendude commented Mar 31, 2021

eigendude commented Mar 7, 2020 •

edited

Loading

eigendude commented Mar 18, 2020 •

edited

Loading

FlorianFranzen commented Mar 29, 2021 •

edited

Loading