rapidgzip icon indicating copy to clipboard operation
rapidgzip copied to clipboard

CMake support and project structure

Open cal-pratt opened this issue 1 year ago • 6 comments

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!

cal-pratt avatar Jan 16 '25 17:01 cal-pratt

Sounds reasonable. Renaming src to rapidgzip (and adjusting CMake) seems sufficient to implement this, right?

mxmlnkn avatar Jan 16 '25 20:01 mxmlnkn

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?

cal-pratt avatar Jan 16 '25 21:01 cal-pratt

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.

mxmlnkn avatar Jan 16 '25 23:01 mxmlnkn

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 ?

cal-pratt avatar Jan 16 '25 23:01 cal-pratt

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.

mxmlnkn avatar Jan 16 '25 23:01 mxmlnkn

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!

cal-pratt avatar Jan 17 '25 01:01 cal-pratt

Has been merged into indexed_bzip2. I'll do a release and merge it into this repository probably next week.

mxmlnkn avatar Aug 01 '25 23:08 mxmlnkn