ledatelescope
Include some more headers when installing
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:
- what the reasoning is behind some headers being placed in
src/bifrost, and others not? (it looks like these haveextern Cset, and are.h, not.hpp). - Is there a good reason to have a
cuda/directory with only one file,stream.hppin it? Follow-up: why isn'tcuda.hppin this directory?
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?
The follow-up question: If I were to just move all headers to src/bifrost and refactor the code, would a PR be accepted?
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:
- src/bifrost/ - the
.hfiles - src/bifrost/c++ - the relevant
.hppfiles - src/bifrost/cuda - the relevant
.hufiles Something like that? @league do you have any thoughts on this?
Maybe also a src/bifrost/network directory so we can try to extend this to packet capture plugins?
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¢.
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?