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

build: add container and strings library #16144

Merged
merged 3 commits into from
Jan 18, 2024

Conversation

dotnwat
Copy link
Member

@dotnwat dotnwat commented Jan 18, 2024

The v::bytes library had implicit dependency on v::utils due to header-only dependencies. In reality it would be far better to have explicit dependencies, and allow v::utils to depend on v::bytes. Currently this would create a circular dependency problem.

This PR breaks out a few things from utils to partially break this dependency:

  • v::container - intrusive list helpers
  • v::strings - utf8/string_switch

Now v::bytes depends on these two new foundational libraries.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.3.x
  • v23.2.x
  • v23.1.x

Release Notes

  • none

Initially contains boost intrusive list helpers.

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Initially contains utf8 and string_switch utilities.

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Copy link
Contributor

@rockwotj rockwotj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, organizationally, do we want to start grouping some of these smaller utility libraries into a directory somewhere? I think it can be hard to grok all the top level libraries (not sure if you're planning on doing this later or not).

Similarly with v::container, that feels like it could grow a lot - should frag_vec go there for instance? Should we separate out v::container into v::container_list, v::container_fragvec, etc? That's a bit more of the absl organization path. Our current include scheme doesn't support multiple targets in the same directory, so I think we need mor directories

@dotnwat
Copy link
Member Author

dotnwat commented Jan 18, 2024

LGTM, organizationally, do we want to start grouping some of these smaller utility libraries into a directory somewhere? I think it can be hard to grok all the top level libraries (not sure if you're planning on doing this later or not).

Similarly with v::container, that feels like it could grow a lot - should frag_vec go there for instance? Should we separate out v::container into v::container_list, v::container_fragvec, etc? That's a bit more of the absl organization path. Our current include scheme doesn't support multiple targets in the same directory, so I think we need mor directories

Yep, completely agree. If we don't mind an extra directory level like util/container or rsl/container then we can do that today without any extra bits. Once we want to do more fine grained things like container_list or container_fragvec within the same thematic container directory we'll need a little bit of cmake magic, but it should be straight forward.

One thing i'm unclear about is how soon we should do that. Wait until we break things out fine grained and the build is strict then reconsolidate, or can we see how things should be now and just move to that point?

@dotnwat
Copy link
Member Author

dotnwat commented Jan 18, 2024

139 - cloud_storage_rpfixture (Failed)

bad test!

@dotnwat dotnwat merged commit 572562f into redpanda-data:dev Jan 18, 2024
17 of 19 checks passed
@rockwotj
Copy link
Contributor

Wait until we break things out fine grained and the build is strict then reconsolidate, or can we see how things should be now and just move to that point?

I don't have strong opinions here. I would personally do it now, but we can certainly wait until the dust settles first, I don't think there are any issues with waiting except for the idea that we're currently churning the code a bit here, and we may not want to do that too frequently.

@StephanDollberg
Copy link
Member

Did we intentionally not backport this? Feels like this will make lots of backports require explicit changes.

@dotnwat
Copy link
Member Author

dotnwat commented Mar 27, 2024

Did we intentionally not backport this? Feels like this will make lots of backports require explicit changes.

not intentionally. i guess we could. it should only affect changes to the containers that would be problematic, right? all the include paths are the same.

@StephanDollberg
Copy link
Member

all the include paths are the same.

Maybe I am misunderstanding but they change from:

<util/foo.hpp>

to

<container/foo.hpp>

no?

@dotnwat
Copy link
Member Author

dotnwat commented Mar 27, 2024

ahh, yes you are correct. most of the module changes (e.g. http,bytes,compression,etc...) don't have include path changes. container and strings yes are new modules so they'll change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants