Skip to content

Conversation

@ThomasDevoogdt
Copy link

@ThomasDevoogdt ThomasDevoogdt commented Jul 25, 2023

e.g. fluent-bit is a c-only library, so allow compilation without cxx

@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/cmake-add-option-to-disable-cxx-support branch 2 times, most recently from de88d6f to 98320bb Compare July 26, 2023 06:54
@ThomasDevoogdt ThomasDevoogdt changed the title CMakeLists.txt: allow compilation without CXX support #4364 CMakeLists.txt: allow compilation without CXX support (#4364) Jul 26, 2023
ThomasDevoogdt added a commit to ThomasDevoogdt/fluent-bit that referenced this pull request Jul 26, 2023
…fluent#7741

e.g. fluent-bit is a c-only library, so allow compilation without cxx

Upstream: confluentinc/librdkafka#4366
Signed-off-by: Thomas Devoogdt <[email protected]>
ThomasDevoogdt added a commit to ThomasDevoogdt/fluent-bit that referenced this pull request Jul 26, 2023
…fluent#7741

e.g. fluent-bit is a c-only library, so allow compilation without cxx

Upstream: confluentinc/librdkafka#4366
Signed-off-by: Thomas Devoogdt <[email protected]>
@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/cmake-add-option-to-disable-cxx-support branch from 98320bb to 8a74726 Compare August 7, 2023 17:24
@cla-assistant
Copy link

cla-assistant bot commented Aug 7, 2023

CLA assistant check
All committers have signed the CLA.

@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/cmake-add-option-to-disable-cxx-support branch from deca571 to 12520e5 Compare August 16, 2023 21:01
@ThomasDevoogdt
Copy link
Author

@emasab, a kind remember

@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/cmake-add-option-to-disable-cxx-support branch 2 times, most recently from 261eb15 to e9183d5 Compare September 26, 2023 13:43
@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/cmake-add-option-to-disable-cxx-support branch from e9183d5 to 9590d99 Compare October 9, 2023 11:22
@ThomasDevoogdt
Copy link
Author

ThomasDevoogdt commented Oct 9, 2023

@emasab, @NZKoz, @ctrochalakis, @thijsc, @janmejay, @milindl

Hi all,

This PR has been open for quite some time, is it possible to review and/or approve?
I'm more than happy to change whatever is needed to get this merged.

This commit solves this issue:

Also solves this issue in fluent-bit:

Which in turn would supersede a patch in buildroot:

Kr,

Thomas Devoogdt

@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/cmake-add-option-to-disable-cxx-support branch from 9590d99 to 941da36 Compare October 13, 2023 16:42
@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/cmake-add-option-to-disable-cxx-support branch 2 times, most recently from 8f2c14e to d86a7d1 Compare October 28, 2023 08:18
@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/cmake-add-option-to-disable-cxx-support branch from d86a7d1 to 9a5609d Compare December 13, 2023 21:40
@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/cmake-add-option-to-disable-cxx-support branch from 9a5609d to 0a7cef8 Compare January 10, 2024 21:53
@ThomasDevoogdt
Copy link
Author

@emasab, @NZKoz, @ctrochalakis, @thijsc, @janmejay, @milindl

Hi all,

This PR has been open for quite some time, is it possible to review and/or approve? I'm more than happy to change whatever is needed to get this merged.

This commit solves this issue:

Also solves this issue in fluent-bit:

Which in turn would supersede a patch in buildroot:

Kr,

Thomas Devoogdt

@emasab, @NZKoz, @ctrochalakis, @thijsc, @janmejay, @milindl

Please, can anyone review this PR? Or just being polite and reject it with a small comment?

@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/cmake-add-option-to-disable-cxx-support branch from 0a7cef8 to f50f5c8 Compare February 12, 2024 21:43
@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/cmake-add-option-to-disable-cxx-support branch from f50f5c8 to 2114bd2 Compare March 30, 2024 21:48
@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/cmake-add-option-to-disable-cxx-support branch from 2114bd2 to d2563e6 Compare April 20, 2024 09:29
@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/cmake-add-option-to-disable-cxx-support branch from d2563e6 to 89b1f50 Compare May 24, 2024 07:20
@ThomasDevoogdt
Copy link
Author

@emasab can you consider this PR?

@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/cmake-add-option-to-disable-cxx-support branch from 89b1f50 to 69552ac Compare June 8, 2024 10:30
@ThomasDevoogdt ThomasDevoogdt requested a review from a team as a code owner June 8, 2024 10:30
@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/cmake-add-option-to-disable-cxx-support branch 2 times, most recently from 5f55e00 to 81aadb4 Compare July 9, 2024 15:56
ThomasDevoogdt added a commit to ThomasDevoogdt/fluent-bit that referenced this pull request May 8, 2025
@ThomasDevoogdt ThomasDevoogdt changed the title CMakeLists.txt: allow compilation without CXX support (#4364) plugins: kafka: fix cmake cross compile error May 8, 2025
@ThomasDevoogdt ThomasDevoogdt changed the title plugins: kafka: fix cmake cross compile error CMakeLists.txt: allow compilation without CXX support May 8, 2025
ThomasDevoogdt added a commit to ThomasDevoogdt/fluent-bit that referenced this pull request May 16, 2025
ThomasDevoogdt added a commit to ThomasDevoogdt/fluent-bit that referenced this pull request May 31, 2025
ThomasDevoogdt added a commit to ThomasDevoogdt/fluent-bit that referenced this pull request Jun 5, 2025
ThomasDevoogdt added a commit to ThomasDevoogdt/fluent-bit that referenced this pull request Jun 7, 2025
@ThomasDevoogdt
Copy link
Author

@emasab can you have a look at this PR? With some luck, it can be merged just within 2 years.

@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/cmake-add-option-to-disable-cxx-support branch from a7cb234 to f7f83f2 Compare July 9, 2025 05:52
ThomasDevoogdt added a commit to ThomasDevoogdt/fluent-bit that referenced this pull request Jul 14, 2025
ThomasDevoogdt added a commit to ThomasDevoogdt/fluent-bit that referenced this pull request Jul 14, 2025
ThomasDevoogdt added a commit to ThomasDevoogdt/fluent-bit that referenced this pull request Jul 19, 2025
@emasab
Copy link
Contributor

emasab commented Aug 1, 2025

@ThomasDevoogdt sorry for the late reply but as you can imagine we've a schedule of features to add to stay on parity with the Java client and about the community issues we give priority to the critical ones. In this case librdkafka has a C++ client too so it's fair it's requiring a C++ compiler.
It's an enhancement to be able to compile without it, if you need only the C library. I see if we can merge this after much time.

@ThomasDevoogdt
Copy link
Author

@ThomasDevoogdt sorry for the late reply but as you can imagine we've a schedule of features to add to stay on parity with the Java client and about the community issues we give priority to the critical ones. In this case librdkafka has a C++ client too so it's fair it's requiring a C++ compiler. It's an enhancement to be able to compile without it, if you need only the C library. I see if we can merge this after much time.

Hi, thx for the response. I know that this PR looks like an "enhancement", but I do consider it more a bug that the librdkafka C library requires C++. Not all targets do have a C++ compiler out of the box. I'm working on a PR in fluent-bit fluent/fluent-bit#9277 to check if compilation succeeds without a C++ compiler. But they are a bit waiting on this to be merged. Yes, I can help myself to apply patches on my build targets, but it would help me seriously if this patch doesn't have to wait "for much time" (Which it has been doing for 2 years). Beware that this PR doesn't really change anything if C++ is just available, so there is no real regression possible here.

@emasab
Copy link
Contributor

emasab commented Aug 5, 2025

It cannot be considered a bug as librdkafka never allowed to compile without a C++ compiler, if it was allowed an it was not working it would be a bug.

When changing it for CMake it should be allowed to compile without C++ with mklove configuration too. Tests should be built and runnable, only the C++ tests should be disabled in that case.

Disabling C++ should only be done on purpose like passing a --disable-cxx configure argument, not only when the compiler isn't present, to avoid unintended effects.

@ThomasDevoogdt
Copy link
Author

ThomasDevoogdt commented Aug 7, 2025

It cannot be considered a bug as librdkafka never allowed to compile without a C++ compiler, if it was allowed an it was not working it would be a bug.

I know, C++ support was indeed always expected, but that doesn't mean it should be. Sometimes you just don't need it.

When changing it for CMake it should be allowed to compile without C++ with mklove configuration too. Tests should be built and runnable, only the C++ tests should be disabled in that case.

I don't see any mention of mklove in any CMakeLists.txt file, not sure what you mean. How to enable it, so that I can check if compilation works with/without C++? About the tests, that is exactly what is done, only the C++ tests are disabled in this commit. (UPDATE: Now I see, the examples were handled correctly, but not the tests, will fix that!)

Disabling C++ should only be done on purpose like passing a --disable-cxx configure argument, not only when the compiler isn't present, to avoid unintended effects.

The current way of doing it on purpose is CXX=/bin/false, I don't see the added value of having to pass e.g. -DDISABLE_CXX=ON.
CFR: buildroot/buildroot@b34e0d2

See this output:

With C++

$ cmake -GNinja ../
-- The C compiler identification is GNU 13.3.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Looking for a CXX compiler
-- Looking for a CXX compiler - /usr/bin/c++
-- The CXX compiler identification is GNU 13.3.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
$ ninja
$ ls -al src-cpp/librdkafka++.so*
lrwxrwxrwx 1 thomas thomas     17 aug  7 22:16 src-cpp/librdkafka++.so -> librdkafka++.so.1
-rwxrwxr-x 1 thomas thomas 430600 aug  7 22:16 src-cpp/librdkafka++.so.1

Without C++

$ CXX=/bin/false cmake -GNinja ../
-- The C compiler identification is GNU 13.3.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Looking for a CXX compiler
-- Looking for a CXX compiler - NOTFOUND
-- C++ compiler not found, skipping C++ support
$ ninja
$ ls -al src/librdkafka.so* 
lrwxrwxrwx 1 thomas thomas      15 aug  7 22:14 src/librdkafka.so -> librdkafka.so.1
-rwxrwxr-x 1 thomas thomas 4346816 aug  7 22:14 src/librdkafka.so.1

I think that it's quite clear printed that C++ support has been skipped, you can't miss it either, since librdkafka++.so won't be there. So not sure how there could be an unintended effect.

@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/cmake-add-option-to-disable-cxx-support branch from f7f83f2 to a77e3bb Compare August 7, 2025 21:10
@ThomasDevoogdt
Copy link
Author

@emasab I updated the test part.

ThomasDevoogdt added a commit to ThomasDevoogdt/fluent-bit that referenced this pull request Aug 31, 2025
Upstream is not really amused with the proposal to allow compilation
without CXX support, see confluentinc/librdkafka#4366.
So disable it from the check for now.

Signed-off-by: Thomas Devoogdt <[email protected]>
ThomasDevoogdt added a commit to ThomasDevoogdt/fluent-bit that referenced this pull request Aug 31, 2025
Upstream is not really amused with the proposal to allow compilation
without CXX support, see confluentinc/librdkafka#4366.
So disable it from the check for now.

Signed-off-by: Thomas Devoogdt <[email protected]>
ThomasDevoogdt added a commit to ThomasDevoogdt/fluent-bit that referenced this pull request Sep 10, 2025
Upstream is not really amused with the proposal to allow compilation
without CXX support, see confluentinc/librdkafka#4366.
So disable it from the check for now.

Signed-off-by: Thomas Devoogdt <[email protected]>
ThomasDevoogdt added a commit to ThomasDevoogdt/fluent-bit that referenced this pull request Sep 10, 2025
Upstream is not really amused with the proposal to allow compilation
without CXX support, see confluentinc/librdkafka#4366.
So disable it from the check for now.

Signed-off-by: Thomas Devoogdt <[email protected]>
patrick-stephens pushed a commit to fluent/fluent-bit that referenced this pull request Oct 2, 2025
Upstream is not really amused with the proposal to allow compilation
without CXX support, see confluentinc/librdkafka#4366.
So disable it from the check for now.

Signed-off-by: Thomas Devoogdt <[email protected]>
@ThomasDevoogdt
Copy link
Author

@emasab its again a bit silent here. I assume that you don't really like the pull request update? Is e.g. adding a CMakeLists.txt option better? Which is more explicit to do...

e.g. fluent-bit is a c-only library, so allow compilation without cxx

Signed-off-by: Thomas Devoogdt <[email protected]>
@ThomasDevoogdt ThomasDevoogdt force-pushed the feature/cmake-add-option-to-disable-cxx-support branch from a77e3bb to 7576aa6 Compare October 3, 2025 15:52
ThomasDevoogdt added a commit to ThomasDevoogdt/fluent-bit that referenced this pull request Oct 4, 2025
SagiROosto pushed a commit to AnyVisionltd/fluent-bit that referenced this pull request Nov 5, 2025
Upstream is not really amused with the proposal to allow compilation
without CXX support, see confluentinc/librdkafka#4366.
So disable it from the check for now.

Signed-off-by: Thomas Devoogdt <[email protected]>
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.

3 participants