CMake support and project structure
Hey there! I'm interested in packaging this for use in another CMake project. Currently, I've vendored in these sources as part of my POC work, but changing up the structure would allow me to package this internally a lot easier.
I notice a lot of the hpp files use very common names. That will lead to possible collisions with other packages, making placing them in a system include dir challenging. e.g. #include <common.hpp> and #include <Error.hpp>.
I'm thinking we could update the includes in the project to specify a base directory. e.g. instead of
#include <FasterVector.hpp>
#include <BZ2Reader.hpp>
#include <rapidgzip.hpp>
We could try something like this:
#include <rapidgzip/core/FasterVector.hpp>
#include <rapidgzip/indexed_bzip2/BZ2Reader.hpp>
#include <rapidgzip/rapidgzip.hpp>
I'm willing to contribute this change, but want to get your thoughts before I moved forward on that!
Sounds reasonable. Renaming src to rapidgzip (and adjusting CMake) seems sufficient to implement this, right?
Sounds reasonable. Renaming src to rapidgzip (and adjusting CMake) seems sufficient to implement this, right?
And updating the paths listed in the include lines. i.e. #include <common.hpp> -> #include <rapidgzip/common.hpp>.
I can take a stab at this and we can iterate on the structure in the PR.
Separate note, in this change I'd like to also put everything namespaces to avoid name collisions. I notice some files are wrapped in namespace rapidgzip { } while others are not. Is that intended?
Separate note, in this change I'd like to also put everything namespaces to avoid name collisions. I notice some files are wrapped in
namespace rapidgzip { }while others are not. Is that intended?
I probably couldn't think of a good generic namespace, especially as this project is part of indexed_bzip2, which should have its own namespace and then there is the shared core stuff between those two projects.
Ah, thanks for the clarification.
For the namespacing, maybe something like this?:
- core ->
namespace rapidgzipcore {} - indexed_bzip ->
namespace indexedbzip {} - rapidgzip ->
namespace rapidgzip {}
Also I'm now seeing the note I overlooked about this being a fork. How would you like to handle PRs in that case? Should they be opened against this repo or to indexed_bzip2 ?
For the namespacing, maybe something like this?:
* core -> `namespace rapidgzipcore {}`
Maybe namespace rapidgzip::core {} instead.
* indexed_bzip -> `namespace indexedbzip {}`
namespace indexed_bzip2 {} would be the most consistent to me.
* rapidgzip -> `namespace rapidgzip {}`Also I'm now seeing the note I overlooked about this being a fork. How would you like to handle PRs in that case? Should they be opened against this repo or to indexed_bzip2 ?
Against indexed_bzip2 would be better. But, for those two smaller ones, I'd also be fine with simply cherry-picking the commits instead of merging via Github.
Sounds good. I'll need to get approval for contributing to the other repo, but I can start in off of this fork. I can issue them over there if you do not get to it first!
Has been merged into indexed_bzip2. I'll do a release and merge it into this repository probably next week.