feat: cbindings - allowing libwaku.so (dynamic) library

[Ivansete-status]

Description

This PR aims for adding the option to create a dynamic linking library (libwaku.so.)

To change the type of library just pass STATIC=true to the Makefile command, and it will generate a libwaku.a. If nothing is passed, it builds dynamic library by default.

How to use it

Create cwaku_example

1. To generate a static library (libwaku.a) and cwaku_example binary with the libwaku embbeded into it:
   make cwaku_example STATIC=true -j8
2. To generate a dynamic library (libwaku.so) and cwaku_example binary with the libwaku embbeded into it:
   make cwaku_example -j8

Generate the library itself:

1. dynamic library: make libwaku -j8.
   `-> produces: build/libwaku.so
2. static library: make libwaku STATIC=true -j8
   `-> produces: build/libwaku.a

Issue

#1632

[vpavlin]

Do you know why is the confutils imported in the nat.nim? It seems like a pretty low-level library? Would it make sense to take a look at that and try to remove the dependency on confutils (if possible/feasible), rather than copy over the file?

[Ivansete-status]

Do you know why is the confutils imported in the nat.nim? It seems like a pretty low-level library? Would it make sense to take a look at that and try to remove the dependency on confutils (if possible/feasible), rather than copy over the file?

Yes, it imports confutils because the ConfigurationError is used in the next proc:
https://github.com/status-im/nim-eth/blob/72c98589278aec949c13435d9bcacdb306faa5a8/eth/net/nat.nim#L326

Actually, I think the parseCmdArg should be out of the nat.nim's responsability. I don't know if we can modify this eth/net/nat.nim ourselves.

[vpavlin]

Well, I meant to contribute to the nim-eth repository and then update the dependency if/when they merge it - It's fine to keep the file around until that happens with a note and a link to the upstream PR/issue.

But up to you, I am just not a big fan of keeping copied files from external repositories around:) Makes maintenance harder in case of security issues in dependencies etc.

Or at least file an issue on nim-eth asking if it is possible for them to remove the dependency by moving the parseCmdArg outside of nat.nim and see what they say?:)

[Ivansete-status]

Well, I meant to contribute to the nim-eth repository and then update the dependency if/when they merge it - It's fine to keep the file around until that happens with a note and a link to the upstream PR/issue.

But up to you, I am just not a big fan of keeping copied files from external repositories around:) Makes maintenance harder in case of security issues in dependencies etc.

Or at least file an issue on nim-eth asking if it is possible for them to remove the dependency by moving the parseCmdArg outside of nat.nim and see what they say?:)

I absolutely agree mate. I don't like either that so I will raise an issue for them and see what they say :)

[Ivansete-status]

Hi @vpavlin, I've created the next PR but I can't add reviewers into it.
status-im/nim-eth#609

[vpavlin]

Awesome!

[jm-clius]

In order to help the review, perhaps it's a good idea to split the nat changes to a separate PR first? What changes were introduced from the original eth/net/nat other than not importing confutils? It seems e.g. that we've added a Result rather than raise exceptions, which is a good change IMO.

Also, I assume there's no way we can create a wrapper for the nim-eth nat module that reuses some of the functionality rather than rewrite everything (I haven't checked how widely the imported ConfigurationError is used)?

[vpavlin]

I believe the only real solution is to get this fixed upstream (i.e. in the dependency), but I agree that to unblock this PR until upstream fixed, very clean and explicit changes to the forked nat.nim are necessary.

[Ivansete-status]

Following @jm-clius's advise, please take first a look at the next PR. Once it is merged I will make this one simpler :)
#1740
Thanks!

[jm-clius]

@Ivansete-status Good idea to convert PRs like this one to "Draft" while it's not ready to be merged.

[Ivansete-status]

Morning all!
Thanks for the comments so far @vpavlin , @jm-clius!
That can be re-reviewed again when you have some time :)

[jm-clius]

LGTM, thanks!

[vpavlin]

THis looks way cleaner:) Thanks! LGTM