search

ledatelescope / bifrost

A stream processing framework for high-throughput applications.
BSD 3-Clause "New" or "Revised" License
66 stars 29 forks source link

Include some more headers when installing #188

Open telegraphic opened 1 year ago

telegraphic commented 1 year ago

With the plugin system, I've found that I need utils.hpp and assert.hpp,which are in src/, not src/bifrost, so are not copied over to /usr/local (or --prefix if that is set). So, I need to manually pass the path of the bifrost source code before I can get things to compile.

Can I ask:

I would think any part of the C++ API that we want to expose should have header included in src/bifrost, so they can be copied over to /usr/local/include/bifrost upon installation?

telegraphic commented 1 year ago

The follow-up question: If I were to just move all headers to src/bifrost and refactor the code, would a PR be accepted?

jaycedowell commented 1 year ago

what the reasoning is behind some headers being placed in src/bifrost, and others not? (it looks like these have extern C set, and are .h, not .hpp).

Because this is how it's always been? 😃

I'm not really sure but, if I had to guess, I would say that the headers in src/bifrost/ are the ones that represent the C++ API and are needed to build a C++ pipeline or wrap the C++ API with Python. They are wrapped as extern C so that we can use ctypesgen to generate the Python bindings.

Is there a good reason to have a cuda/ directory with only one file, stream.hpp in it? Follow-up: why isn't cuda.hpp in this directory?

This is something that I don't have a good answer for. It almost looks like src/cuda/ was meant to be something more but that something never materialized.

If I were to just move all headers to src/bifrost and refactor the code, would a PR be accepted?

Yes, I think this is something we need to do if we want to make a plugin system that allows people to write new classes in the same style as what ships in Bifrost. Even my plugin-wrapper branch ran into problems with not having assert.hpp and utils.hpp and I had to get around be giving a path to the Bifrost source. The question I see is not if but how to arrange the header files. It would be nice to have something that conveys what they are for, i.e., a directory structure that makes a distinction between what is part of the C++ API and what is needed to write new class/for plugin support. So maybe something like a:

jaycedowell commented 1 year ago

Maybe also a src/bifrost/network directory so we can try to extend this to packet capture plugins?

league commented 1 year ago

I guess IMO, src/bifrost should represent the public interface, and in turn, anything needed to build solutions/plugins using Bifrost would go there. It does seem like assert and utils might be in that category… [I'm aware of a perspective that so-called utils modules can be a “code smell!” – but I don't have a problem with it in particular.]

But there is a cost to making something part of the public interface if it doesn't need to be – in terms of writing/maintaining documentation, limiting changes to the API, etc. I wouldn't want to put things that are just implementation-only but ideally not client-facing in there. One thing I'm thinking of is our unholy ifdef’d filesystem code for maintaining caches.

I think using subdirectories within the includes seems a little over-engineered, perhaps? Unless it's really clear that these are distinct sub-components with crisp boundaries somehow. Maybe different plugin interfaces qualify? I don't think I'd do it for different types – extensions like .h vs .hpp vs .hu are distinction enough? But I feel the same thing about web devs who do mkdir assets/{css,images,fonts,icons}. Just 2¢.

jaycedowell commented 1 year ago

That's a good point about stability of the public interface. I guess my goal with the subdirectories was to try to convey a sense of that by separating out what defines the rings/status codes/classes (this .hs) with what is more suitable for plugins (the .hpps and .hus). I just took it a step farther to separate C++ from CUDA. And threw in an additional one for good measure.

What about calling it src/bifrost/plugins or src/bifrost/internal with a suitable README that says that the interfaces defined there are subject to change?