Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMake support and project structure #48

Open
cal-pratt opened this issue Jan 16, 2025 · 6 comments
Open

CMake support and project structure #48

cal-pratt opened this issue Jan 16, 2025 · 6 comments
Labels
enhancement New feature or request

Comments

@cal-pratt
Copy link

cal-pratt commented Jan 16, 2025

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 cal-pratt added the enhancement New feature or request label Jan 16, 2025
@mxmlnkn
Copy link
Owner

mxmlnkn commented Jan 16, 2025

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

@cal-pratt
Copy link
Author

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?

@mxmlnkn
Copy link
Owner

mxmlnkn commented Jan 16, 2025

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.

@cal-pratt
Copy link
Author

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 ?

@mxmlnkn
Copy link
Owner

mxmlnkn commented Jan 16, 2025

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.

@cal-pratt
Copy link
Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants