Fix OAuthBearer OIDC preprocessor directive when using CMake without CURL - #5136
Conversation
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a faster “quick” test runner configuration, updates plugin support detection in tests, fixes the OAuthBearer OIDC include guard, and extends the build‐configuration checks script to support both Make and CMake inside Docker.
- Added a new quick brokerless test case in
tests/CMakeLists.txt - Introduced runtime skip logic for unsupported
plugin.library.pathstests intests/0066-plugins.cpp - Switched
#ifdef WITH_OAUTHBEARER_OIDCto#if WITH_OAUTHBEARER_OIDCinsrc/rdkafka_conf.c - Enhanced
build-configurations-checks.shto accept a build tool and optional Docker image, with corresponding updates in.semaphore/semaphore.yml
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/CMakeLists.txt | Added RdKafkaTestBrokerLessQuick test target |
| tests/0066-plugins.cpp | Wrapped plugin.path set in a skip condition for unsupported builds |
| src/rdkafka_conf.c | Changed OIDC preprocessor to #if for proper zero/one testing |
| packaging/tools/build-configurations-checks.sh | Generalized to handle make or cmake modes and Docker images |
| .semaphore/semaphore.yml | Invokes the updated script with explicit tool/image combinations |
|
Hi there, is there anything we can do to help get this merged? |
8740d6a to
b9a0618
Compare
49d4a35 to
ce8959a
Compare
| #elif !HAVE_ATOMICS_32 | ||
| #elif HAVE_ATOMICS_32 && HAVE_ATOMICS_32_ATOMIC | ||
| return __atomic_exchange_n(&ra->val, v, __ATOMIC_SEQ_CST); | ||
| #elif HAVE_ATOMICS_32 && HAVE_ATOMICS_32_SYNC |
There was a problem hiding this comment.
Isn't this branch a dead code in case of CMake and eventually ending up using #else branch which is safe though because lock would be defined given HAVE_ATOMICS_32 is 0?
CMake ends up setting only HAVE_ATOMICS_32_SYNC reference and not HAVE_ATOMICS_32. They both seem mutually exclusive in case of CMake. So this branch would never be entered right?
Should we change to something like below which seems to handle all scenarios and isn't based on the invariant HAVE_ATOMICS_32_ATOMIC
static RD_INLINE int32_t RD_UNUSED rd_atomic32_set(rd_atomic32_t *ra,
int32_t v) {
#ifdef _WIN32
return InterlockedExchange((LONG *)&ra->val, v);
#elif HAVE_ATOMICS_32_SYNC
return __sync_lock_test_and_set(&ra->val, v);
#elif !HAVE_ATOMICS_32
int32_t r;
mtx_lock(&ra->lock);
r = ra->val;
ra->val = v;
mtx_unlock(&ra->lock);
return r;
#else
return __atomic_exchange_n(&ra->val, v, __ATOMIC_SEQ_CST);
#endif
}
The only tradeoff I see is that default assumes __atomic is available.
There was a problem hiding this comment.
I was uniforming cmake in this PR to set the same values as mklove, so HAVE_ATOMICS_32 for example is set to 1 in both cases and then either HAVE_ATOMICS_32_ATOMIC or HAVE_ATOMICS_32_SYNC is set to 1.
Just there's a case when using try_compile(HAVE_ATOMICS_32_SYNC where HAVE_ATOMICS_32 isn't set to true. I've pushed the cmake change. In mklove instead there are cases where HAVE_ATOMICS_32_SYNC or HAVE_ATOMICS_32_ATOMIC aren't defined if HAVE_ATOMICS_32 is false.
with alpine or manylinux
CMake builds defined HAVE_ATOMICS_{32,64} (and optionally _SYNC) but never
HAVE_ATOMICS_{32,64}_ATOMIC, so rd_atomic32_set()/rd_atomic64_set() fell
through to a non-atomic fallback that returned the new value instead of the
previous one.
This broke the planned-disconnection all-brokers-down handling, which relies
on rd_atomic32_set(&rkb_down_reported, 1) returning the previous value to
detect the 0 -> 1 transition. As a result the ALL_BROKERS_DOWN event was never
raised in CMake builds (test 0095 timed out).
- Detect and define HAVE_ATOMICS_{32,64}_ATOMIC in the CMake build when the
__atomic builtins are available, mirroring mklove's configure.atomics.
- Rework rd_atomic{32,64}_set to select the __atomic / __sync implementation
from the HAVE_ATOMICS_* flags and use the mutex path as fallback instead of
a non-atomic store.
The #elif chain placed the mutex fallback (!HAVE_ATOMICS_64) before the __atomic and __sync implementations, so the native atomic CAS branches were never reachable and an incorrect non-atomic fallback was used. Reorder the branches so HAVE_ATOMICS_64_ATOMIC and HAVE_ATOMICS_64_SYNC are selected first, with the mutex-based path as the final fallback. Also ignore the new share_consumer_commit_async and share_consumer_commit_sync example binaries.
ce8959a to
b2f6dc2
Compare
…tolerance
Variation 1 (retries) of do_test_fast_metadata_refresh expects ~7 Metadata
requests but could occasionally observe more, causing intermittent failures
with the previous expected+1 (8) upper bound.
The count comes from two independent, timing-dependent sources:
- The fast-leader-query loop ("partition leader query"), driven by an
exponential backoff timer with +/-20% jitter (rd_kafka_timer_exp_backoff),
normally fires 6 times within the ~3s window but jitter can let a 7th fire
land inside it.
- A "connected" metadata request issued when a broker connection transitions
to UP (rd_kafka_broker_connect_up); with 3 mock brokers connecting at
slightly different times a second one can land within the window.
These variances can stack, so the count can legitimately reach 9. Widen the
upper bound from expected+1 to expected+2 to account for it.
Pranav Shah (prashah-confluent)
left a comment
Pranav Shah (prashah-confluent)
left a comment
There was a problem hiding this comment.
LGTM !! Thanks a lot Emanuele Sabellico (@emasab)
Summary
Fixes several issues that surface when building librdkafka with CMake and a reduced
feature set, and adds CI coverage so these build-configuration regressions are caught
going forward.
Closes #5135, #5282
Changes
(librdkafka v2.11.0 can not build without curl #5135) — corrects the conditional so compilation succeeds when CURL is disabled.
images with the optional features disabled, to catch compile/runtime breakage in
minimal builds. Adds
packaging/tools/build-configurations-checks.sh,run-in-docker.sh,Dockerfile.alpine,Dockerfile.manylinux, and a Semaphore job.dlopenis unsupported, so the broker-less quicksuite passes in builds without dynamic loading.
rd_atomic{32,64}_setreturning the new value instead of the previous one inCMake builds. CMake defined
HAVE_ATOMICS_{32,64}(and optionally_SYNC) but neverHAVE_ATOMICS_{32,64}_ATOMIC, so the setters fell through to a non-atomic fallback thatreturned the new value. This broke the planned-disconnection all-brokers-down handling,
which relies on
rd_atomic32_set(&rkb_down_reported, 1)returning the previous value todetect the
0 -> 1transition — soALL_BROKERS_DOWNwas never raised and test 0095timed out under CMake. CMake now detects/defines
HAVE_ATOMICS_*_ATOMIC(mirroringmklove's
configure.atomics), and both setters select the__atomic/__syncimplementation from the flags with the mutex path as a correct fallback.
Testing
The new build-configuration checks run tests 0095 and 0121 under CMake/manylinux with all
optional features disabled; both pass (0095 previously timed out before the atomics fix).