Add Bazel unit test target and Windows (MSVC) support#510
Conversation
|
This is a very good addition, thank you for taking care of that. There is one issue that I didn't notice at first. I tried importing This works fine as long as I don't use OpenSSL. However, when I enable OpenSSL, I get linker errors. I managed to solve the problem by linking against the libraries that provide the missing functions: I was wondering what the proper solution to this problem would be. I feel like it makes sense to link these libraries in |
|
I also noticed that you included some unit tests that can run in the test environment. What would be the recommended approach for handling non-hermetic E2E tests? Do you think we should split them into a separate executable and run it directly with |
|
Thanks @slabko, both are great catches. I've pushed fixes addressing your first point and the testing-gap behind it. 1. OpenSSL on Windows needs Why CI didn't catch it. This is the interesting part and it ties directly into your second comment. Two things combined:
In other words, we had no executable that links the OpenSSL code path — exactly the blind spot your consumer binary exposed. Fix for the blind spot: I added a small link/compile guard, 2. Non-hermetic E2E tests. Agreed — the server-dependent tests ( Does that split sound reasonable to you? |
|
Agreed! |
Adds the Windows portions of upstream ClickHouse/clickhouse-cpp#510 to the 2.6.1 overlay (BUILD.bazel + MODULE.bazel): * ws2_32 linkopt on Windows (Winsock, for base/socket.cpp et al.). * crypt32 + user32 linkopts for the OpenSSL-on-Windows configuration, gated by a tls_openssl_on_windows config_setting_group. * 'platforms' bazel_dep (for @platforms//os:windows). The unit-test target (ut/) and its googletest dev_dependency from bazelbuild#510 are intentionally not migrated.
Summary
Follow-up to #501, in two parts:
cc_testtarget covering the server-independent subset of the existing googletest suite inut/, wired into the Bazel CI workflow.windows-latestis added to the CI matrix. MSVC is the toolchain here because it is what Bazel supports on Windows — Bazel has no MinGW support (unlike the CMake build, which has a separate MinGW CI workflow), so the existingWindows mingwcoverage remains CMake-only.Part 1: Unit tests
ut/BUILD.bazel//ut:unit_testscc_test targetMODULE.bazelgoogletest 1.17.0.bcr.2from BCR, markeddev_dependency = Trueso library consumers never pull it in.github/workflows/bazel.ymlbazel test //ut:unit_testsstep on the matrixTest selection — only tests that run hermetically, i.e. no running ClickHouse server required:
types_ut,type_parser_ut,columns_ut,column_array_ut,block_ut,itemview_ut,stream_ut,utils_ut,CreateColumnByType_ut, plussocket_ut(spins up its ownLocalTcpServeron localhost, which works inside the Bazel test sandbox), together with the shared support code (utils.cpp,value_generators.cpp,tcp_server.cpp) and the gtestmain.cpp.$CLICKHOUSE_HOST—client_ut,roundtrip_tests,Column_ut,low_cardinality_nullable_tests,array_of_low_cardinality_tests,abnormal_column_names_test,readonly_client_test,connection_failed_client_test,performance_tests,ssl_ut. These keep running in the CMake CI, which provides a server via docker-compose. Running them under Bazel (e.g. with a dockerized server fixture) could be a future follow-up.The split is documented in a comment at the top of
ut/BUILD.bazel.googletestis adev_dependency, so it does not participate in version resolution for downstream consumers — it only exists when this module is the root.Part 2: Windows support
Adding
windows-latestto the matrix surfaced that the Bazel build didn't link on Windows except by luck:tls=noandtls=opensslfailed withLNK2019: unresolved external symbol __imp_ntohl / __imp_socket / ...— the Winsock library was never declared.tls=boringsslonly worked because BoringSSL's own BUILD file propagatesws2_32to its dependents.Fixes, mirroring what the CMake build already does:
BUILD.bazellinkopts:-DEFAULTLIB:ws2_32.libon WindowsIF (WIN32 OR MINGW) TARGET_LINK_LIBRARIES(... wsock32 ws2_32)BUILD.bazellinkopts:crypt32.lib+user32.libwhen OpenSSL + Windows (gated viaselects.config_setting_group)IF (MSVC) TARGET_LINK_LIBRARIES(... Crypt32)ut/BUILD.bazel/bigobjon WindowsIF (MSVC) TARGET_COMPILE_OPTIONS(... /bigobj)MODULE.bazelplatforms 1.1.0(for@platforms//os:windowsinselect())The OpenSSL backend pulls in the Win32 cert store (
crypt32) anduser32; BoringSSL bundles its own system-lib deps, which is why only the OpenSSL backend needed this. These are attached to the//:clickhouselibrary so downstream executables link transitively, without each consumer repeating it. Notably, the BCRopensslmodule itself compiled cleanly under MSVC — the failures were purely missing system-lib declarations in this repo's BUILD file.Part 3: SSL link guard
A library-only
bazel build //:clickhouseproduces a static archive and never invokes the linker, and the hermeticunit_testsbinary deliberately excludes every SSL source — so static-library linking never pulledsslsocket.obj(and itscrypt32/user32deps) into any executable. That's exactly why the OpenSSL/Windows link failure above didn't surface in CI; it only broke in a downstream consumer that links the SSL code path.To close the gap,
ut/ssl_link_ut.cppadds a single test that constructs anSSLContext. This dragssslsocket.objand its transitive system-lib dependencies into the test binary's link step, so a missingcrypt32/user32now fails in CI rather than downstream. It uses default params (no CA loading, no socket), so it stays hermetic, and it isselect()-ed out undertls=no, where the SSL sources aren't part of the library.Full end-to-end SSL connection testing (which needs a server and
bazel run+ runfiles) is left to a follow-up PR.CI
The Bazel workflow matrix is now 3 OS × 3 TLS modes = 9 combinations (Linux, macOS, Windows ×
boringssl,openssl,no), each running build + unit tests. All 9 are green on this branch.Test plan
bazel test //ut:unit_testspasses locally on darwin-arm64 for all three TLS modes: 192 tests, 191 passed, 1 skipped by design (Socketcase.connecttimeout), 1 pre-existingDISABLEDtest.SSLLink.ConstructContextruns undertls=boringssl/openssland is excluded undertls=no.