Module loader design

[afrind]

This change is 

[michalhosna]

I am not sure if this is the right decision.

Given that this will be an OSS project and we want to ship pre-built binaries (see [spec 2.1.8])(https://github.com/OpenMOQ/relay-requirements/blob/f9e11d6ebfc6298458b7a3f8f554b0e4ec9a6785/relay-requirements.md#21-core-technology-stack).

I would expect that people would like to ship pre-built 3rd party modules. For the adoption of this project, I think it is beneficial to be able to ship pre-compiled modules.

Sure, this can be solved by requiring a specific toolchain, but I am not sure if that's the greatest solution?

Having a stable ABI is, I think, beneficial for the adoption of this project.

I really do not like pure C APIs, but here they seem appropriate not only for ABI stability, but also for the ability to provide modules in different (non-C++) languages.

Having a C++ -> C "adapter" for both sides (relay and modules) should be able to remove the C pain here fairly easily, so I think we should go with it right away and avoid a API split down the line.

[afrind]

Ok that sounds safer. Sorry the AI was like "Decision: this is fine" lol.

Given the sophistication of some of the modules we're talking about (eg: black box, caching), that's a lot of C++ <-> C translation. Does this decision mean we want to try to scope down the interfaces to something more simplistic that's easier to express in C?

[michalhosna]

I'd consider the C-API just a well-understood method to define a stable ABI, not the greatest one, but the universal one. It doesn't need to be the target for human programmers. We should avoid abominations in the C layer, but simplicity there should not be a focus. I would still push the C++ wrapper API as the primary interface

[afrind]

I think we should do as you say and plumb the modules via C, but it's a pain. Let me update the PR.

[michalhosna]

@michalhosna reviewed 1 file and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on afrind).

[afrind]

@afrind made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on michalhosna).

[afrind]

I think we should do as you say and plumb the modules via C, but it's a pain. Let me update the PR.

[michalhosna]

@michalhosna made 1 comment and resolved 1 discussion.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on afrind).

---

design/module-loader.md line 432 at r2 (raw file):

**Current approach (C++ returns)**: Simpler code, works when plugins are built with the same toolchain as the relay.

**Alternative (pure C interface)**: More portable across compilers. Would use opaque handles and C function pointers:

I think the whole "C++ vs Pure C Interface" section needs little bit of updating, if nothing else to not confuse AI later.

[TimEvens]

@TimEvens made 6 comments.
Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on afrind and michalhosna).

---

design/module-loader.md line 33 at r3 (raw file):

 public:
  virtual ~Plugin() = default;
  virtual const char* name() const = 0;

Prefer these be part of the constructor(s)

---

design/module-loader.md line 34 at r3 (raw file):

  virtual ~Plugin() = default;
  virtual const char* name() const = 0;
  virtual uint32_t version() const = 0;

Constructor is better to enforce expected variables.

32bit is fine but likely will be encoded in dot notation such as SemVer. Might be better add string overload for version number to Set. We should have a Get() as well.

---

design/module-loader.md line 40 at r3 (raw file):

### Type-Specific Interfaces

Each plugin type defines its interface:

I feel that we will need to define additional abstract classes/interfaces for each plugin type. For example, type Auth it is expected that there is authenticate(token, resource) method.

---

design/module-loader.md line 78 at r3 (raw file):

  // Returns nullptr on failure, logs error
  template <typename T>
  std::shared_ptr<T> load(const std::string& name, const std::string& path);

We need better documentation on params and methods. Not just for this method, but for all abstract/interface classes.

Let's use doxygen syntax, which is well supported by IDEs.

---

design/module-loader.md line 82 at r3 (raw file):

  // Get a previously loaded plugin by name
  template <typename T>
  std::shared_ptr<T> get(const std::string& name);

should this be virtual?

---

design/module-loader.md line 86 at r3 (raw file):

  // Get all loaded plugins of a type
  template <typename T>
  std::vector<std::shared_ptr<T>> getAll();

virtual?