Add Bazel build with BoringSSL support#501
Conversation
5ff5053 to
a2c545a
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds Bazel (bzlmod) build support for clickhouse-cpp and adapts TLS codepaths to work with BoringSSL.
Changes:
- Added
MODULE.bazelwith bzlmod deps (abseil, boringssl, lz4, zstd, rules_cc). - Added
BUILD.bazelwith:clickhousecc_library and vendored:cityhash. - Updated TLS implementation to compile under BoringSSL by disabling
SSL_CONF_*usage and usingSSL_readinstead ofSSL_read_ex.
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| clickhouse/base/sslsocket.cpp | Adds BoringSSL-specific branches for SSL configuration and reading APIs. |
| MODULE.bazel | Introduces a bzlmod module definition and external deps for Bazel builds. |
| BUILD.bazel | Adds Bazel targets to build/export the library and its vendored cityhash dependency. |
| .gitignore | Ignores Bazel output symlinks/directories. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Expose headers at their workspace path so both internal | ||
| # (`#include "client.h"` from clickhouse/*.cpp via quote-form | ||
| # source-dir search) and external (`#include <clickhouse/client.h>`) | ||
| # include forms resolve. | ||
| strip_include_prefix = "/", |
| void configureSSL(const clickhouse::SSLParams::ConfigurationType & configuration, SSL * ssl, SSL_CTX * context = nullptr) { | ||
| #ifdef USE_BORINGSSL | ||
| // BoringSSL doesn't ship the SSL_CONF_* command API, so this becomes | ||
| // a no-op. The configuration vector is ignored. | ||
| (void)configuration; | ||
| (void)ssl; | ||
| (void)context; |
| const int max_read = static_cast<int>( | ||
| std::min<std::size_t>(len, std::numeric_limits<int>::max())); | ||
| return static_cast<size_t>( | ||
| HANDLE_SSL_ERROR(ssl_, SSL_read(ssl_, buf, max_read))); |
|
Why BoringSSL (and not OpenSSL) in this PR A few people will probably ask why TLS goes through
The Follow-up: once OpenSSL's BCR story stabilizes, it's straightforward to add a Bazel |
Summary
@boringssl(BCR ships BoringSSL natively, no OpenSSL module yet). The two OpenSSL-only surfaces —SSL_CONF_*command API andSSL_read_ex— are gated behind a newUSE_BORINGSSLdefine inclickhouse/base/sslsocket.cpp. The patch is 17 lines, purely additive; whenUSE_BORINGSSLis undefined the file is byte-identical to before.What's added
MODULE.bazelabseil-cpp,boringssl,lz4,rules_cc,zstd(all latest on BCR)MODULE.bazel.lockBUILD.bazelcc_librarytargets::cityhash(private, fromcontrib/cityhash/cityhash/) and:clickhouse(public).gitignorebazel-*symlinksclickhouse/base/sslsocket.cppsslsocket.cpp patch detail
Two
#ifdef USE_BORINGSSL ... #else ... #endifregions:configureSSL()body — underUSE_BORINGSSLbecomes a no-op (the function is still defined and called, just does nothing). BoringSSL has noSSL_CONF_*API.SSLSocketInput::DoRead()— underUSE_BORINGSSLfalls back toSSL_readwithlenclamped toINT_MAX.SSL_read_exis an OpenSSL 3.0+ API not in BoringSSL.Function signatures, declarations,
SSLParams::ConfigurationType,validateParams, etc. all stay intact, so the public API is unchanged regardless of which TLS backend is linked.Test plan
bazel build //:clickhouseproduceslibclickhouse.aon darwin-arm64 (Bazel 9.1.0) and Linux.Notes for reviewers
cityhashstays vendored undercontrib/cityhash/cityhash/because it isn't on BCR.lz4/zstd/abseilare pulled from BCR;contrib/{lz4,zstd,absl}are still used by the CMake build.