-
Notifications
You must be signed in to change notification settings - Fork 592
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
chore: remove various build dependencies #18660
Conversation
Subsequent commits clean up header dependencies, but some break. This adds in the missing headers that would otherwise cause subsequent commits to not compile. Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
It's nice that the bottomless bucket exposes a max width that can be used in configuration system, but its not scalable to do this for everything in the tree otherwise the configuration subsystem is going to have too many dependencies. In this case the max width seems to be about type compatibility in that its not clear that choosing max int would ever be a reasonable choice anyway. So external configuration facing validation is just different and can live in config system. Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
This follows along the path that security::config library has taken which is to create a dependency-free library in the subsystem that exports the bits that are shared between the subsystem and the configuration. Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
The security_config library contains specific bits that are shared between security module and configuration. However, the header declarations for those bits are in headers that aren't easily included in configuration system because of all the additional things brought along. So this commit splits these out into a specific config.h header that is intended to be included by configuration system. Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
@@ -29,14 +30,6 @@ | |||
|
|||
namespace detail { | |||
|
|||
template<typename T, template<typename...> class C> | |||
struct is_specialization_of : std::false_type {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious why this moved and not the others? And why did it need to move?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with optional. I guess they're the ones that config uses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_specialization_of moved to base
because it's a foundational building block of type trait utilities. std::optional moved because its only dependent on std::
. the others were left alone because they either didn't need to move (yet) or they aren't foundational (e.g. is_fragmented_vector).
std::optional<ss::sstring> | ||
validate_rules(const std::optional<std::vector<ss::sstring>>& r) noexcept; | ||
|
||
} | ||
|
||
namespace security::oidc { | ||
|
||
std::optional<ss::sstring> | ||
validate_principal_mapping_rule(ss::sstring const& rule); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are these defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like ./security/config_bsl.cc. Should this be included there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's in config_rsl.cc, but i'm not sure what you mean by included "there"--this is header file declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry I wasn't clear. I would've thought config_rsl.cc should include this new header. I guess it's a little special since it's in its own security_config library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the compiler is happy, then I'm happy! But I just added a commit to add those headers because its a little weird to not have it as you point out.
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Various build dependencies are showing up while prototyping with a build system setup that is stricter than the current cmake setup.
Backports Required
Release Notes