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(cbindings): Adding cpp example that integrates the 'libwaku' #2079

Merged
merged 7 commits into from
Sep 28, 2023

Conversation

Ivansete-status
Copy link
Collaborator

Description

In this example, we can see how to integrate a nwaku node from within a cpp code. The code is very similar to the C version. The interesting point is the usage of cpp-lambdas and how they can seamlessly integrate with the execution context.

e.g.

    std::string defautPubsubTopic;
    WAKU_CALL(waku_default_pubsub_topic(cify([&](const char* msg, size_t len) {
        defautPubsubTopic = msg;
    })));

Changes

  • Adding cpp-nwaku example.

@github-actions
Copy link

github-actions bot commented Sep 26, 2023

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2079

Built from a11a0ba

@Ivansete-status Ivansete-status changed the title chore(cbindings): Adding cpp example that integrates the libwaku chore(cbindings): Adding cpp example that integrates the 'libwaku' Sep 26, 2023
Copy link
Contributor

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

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

I think I understand what's happening (unfamiliar with C++). Having another example is great.

Thank you!

@NagyZoltanPeter
Copy link
Contributor

Before leaving review comments, I would have some questions or maybe open a little discussion on it due I'm embarrassed a bit about it.

What is the exact aim of such example over the current ansi C example?
What is the minimum C++ standard we targeting (of coz its ok to still use C++98, though a bit uncommon)?

Lets put in other way around. If I would like to show some c++ example, maybe I would like to provide a c++ wrapper on the c-bindings first. Also maybe include base64 encode/decode into the library (just to be confirm fully our requirements).

You know I'm quite ok with such usage, but I would like to give a mean or value over the C version.
I hope you, @Ivansete-status will get the point, I would not like to be destructive on it surely and I know it's just a helper for the crowd, whom may use it differently as their taste.

@gabrielmer
Copy link
Contributor

Really like the use of lambdas for the callbacks!

To use them we need at least C++11. Even though they are not strictly necessary I think C++11 is a good minimum standard to target - even if we didn't use lambdas, it's one of the most widely used standards, and the ones before it are now pretty rarely used.

I do agree that we should either add the base64 encode/decode functions to the library or at least import those functions in the example from some existing external library.

I think that implementing the functions in the example may add noise and take attention from more important parts, and I don't know if we should encourage users to implement their own encoding/decoding functions instead of taking one from a library

@Ivansete-status Ivansete-status self-assigned this Sep 27, 2023
@Ivansete-status
Copy link
Collaborator Author

Thanks for the comments @NagyZoltanPeter @gabrielmer 🔥 !
This is a simple example of how to use nwaku from cpp. Of course, it can be optimized. Nevertheless, cpp is not our priority right now and this is just a simple example.

Re the base64 stuff, I think we have it separated in the C example.
Re the C++ I think C++11 is good as a minimum.

I wouldn't worry very much about this cpp example for now.

This is the roadmap I'd like to follow:

  • Make the libwaku implementation more solid and work properly in Rust, NodeJs, Python. For that, a simple relay example is enough.
  • Once we have libwaku super solid and thread-safe, create separate waku repos for each language, and from there, make each of them use the nwaku as a submodule and therefore compile and link the libwaku library. In those repos we will be able to invest our efforts to have better performance, test, etc.

Thanks again for the comments :)

Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

Sounds great! Thanks so much!

Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

Found some tiny stuff but all OK, main target is already discussed!
Thanks for this nice work!


#include "../../library/libwaku.h"

#define WAKU_CALL(call) \
Copy link
Contributor

Choose a reason for hiding this comment

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

If it can be done to outcast the return value to the caller would be nice.
I'm thinking of a templatized function wrapper, but definitely that is more complicated.


const char b64chars[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";

void b64_encode(char* in, size_t len, std::vector<char>& out)
Copy link
Contributor

Choose a reason for hiding this comment

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

If C++11 we should simple return with std::vector. Move semantics and RVO will solve it for free.

std::vector<char> msgPayload;
b64_encode(msg, strlen(msg), msgPayload);

// waku_content_topic("appName",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it something intentionally left commented our not working?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch! I leave those comments to indicate which libwaku should be invoked. If we uncomment they might not work because the signatures are changing due to making the libwaku thread-safe. We should revisit that once we have time to tackle it.

}

template <class F>
auto cify(F&& f) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not pretty sure that wrapping around lambdas is necessary. Maybe enough to simple give lamba in place to the call of C function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point! Afaik, this is needed. We'll revisit that point in the future.

examples/cpp/waku.cpp Outdated Show resolved Hide resolved
examples/cpp/waku.cpp Outdated Show resolved Hide resolved
examples/cpp/waku.cpp Outdated Show resolved Hide resolved
examples/cpp/waku.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

Great to have this too!

Ivansete-status and others added 6 commits September 28, 2023 11:02
Co-authored-by: NagyZoltanPeter <113987313+NagyZoltanPeter@users.noreply.github.com>
Co-authored-by: NagyZoltanPeter <113987313+NagyZoltanPeter@users.noreply.github.com>
Co-authored-by: NagyZoltanPeter <113987313+NagyZoltanPeter@users.noreply.github.com>
Co-authored-by: NagyZoltanPeter <113987313+NagyZoltanPeter@users.noreply.github.com>
@Ivansete-status
Copy link
Collaborator Author

Thanks for the comments!
I'll merge it now and leave it for the future to apply all the recommendations needed to have a proper cpp example.

@Ivansete-status Ivansete-status merged commit 8455b8d into master Sep 28, 2023
8 checks passed
@Ivansete-status Ivansete-status deleted the cbindings-cpp-ok branch September 28, 2023 09:10
ABresting pushed a commit that referenced this pull request Sep 30, 2023
…2079)

* Adding cpp example that integrates the `libwaku`

---------

Co-authored-by: NagyZoltanPeter <113987313+NagyZoltanPeter@users.noreply.github.com>
ABresting added a commit that referenced this pull request Sep 30, 2023
* chore: add retention policy with GB or MB limitation #1885

* chore: add retention policy with GB or MB limitation

* chore: updated code post review- retention policy

* ci: extract discordNotify to separate file

Signed-off-by: Jakub Sokołowski <jakub@status.im>

* ci: push images to new wakuorg/nwaku repo

Signed-off-by: Jakub Sokołowski <jakub@status.im>

* ci: enforce default Docker image tags strictly

Signed-off-by: Jakub Sokołowski <jakub@status.im>

* ci: push GIT_REF if it looks like a version

Signed-off-by: Jakub Sokołowski <jakub@status.im>

* fix: update wakuv2 fleet DNS discovery enrtree

https://github.com/status-im/infra-misc/issues/171

* chore: resolving DNS IP and publishing it when no extIp is provided (#2030)

* feat(coverage): Add simple coverage (#2067)

* Add test aggregator to all directories.
* Implement coverage script.

* fix(ci): fix name of discord notify method

Also use absolute path to load Groovy script.

Signed-off-by: Jakub Sokołowski <jakub@status.im>

* chore(networkmonitor): refactor setConnectedPeersMetrics, make it partially concurrent, add version (#2080)

* chore(networkmonitor): refactor setConnectedPeersMetrics, make it partially concurrent, add version

* add more metrics, refactor how most metrics are calculated

* rework metrics table fillup

* reset connErr to make sure we honour successful reconnection

* chore(cbindings): Adding cpp example that integrates the 'libwaku' (#2079)

* Adding cpp example that integrates the `libwaku`

---------

Co-authored-by: NagyZoltanPeter <113987313+NagyZoltanPeter@users.noreply.github.com>

* fix(ci): update the dependency list in pre-release WF (#2088)

* chore: adding NetConfig test suite (#2091)

---------

Signed-off-by: Jakub Sokołowski <jakub@status.im>
Co-authored-by: Jakub Sokołowski <jakub@status.im>
Co-authored-by: Anton Iakimov <yakimant@gmail.com>
Co-authored-by: gabrielmer <101006718+gabrielmer@users.noreply.github.com>
Co-authored-by: Álex Cabeza Romero <alex93cabeza@gmail.com>
Co-authored-by: Vaclav Pavlin <vaclav@status.im>
Co-authored-by: Ivan Folgueira Bande <128452529+Ivansete-status@users.noreply.github.com>
Co-authored-by: NagyZoltanPeter <113987313+NagyZoltanPeter@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants