Skip to content

plugins: fix compilation if FLB_SQLDB (sqlite3) is disabled#10239

Open
ThomasDevoogdt wants to merge 7 commits intofluent:masterfrom
ThomasDevoogdt:bugfix/sqlite3-disabled
Open

plugins: fix compilation if FLB_SQLDB (sqlite3) is disabled#10239
ThomasDevoogdt wants to merge 7 commits intofluent:masterfrom
ThomasDevoogdt:bugfix/sqlite3-disabled

Conversation

@ThomasDevoogdt
Copy link
Copy Markdown
Contributor

@ThomasDevoogdt ThomasDevoogdt commented Apr 22, 2025 •
edited by coderabbitai Bot
Loading

Fixes:

Summary by CodeRabbit

  • New Features

    • SQLDB support is now optional; enabling it restores partial-match checklist behavior and improved blob upload tracking.
    • Added a new post-upload action: "Add Suffix".

  • Bug Fixes

    • Non-database builds now fail fast for DB-only modes/events to avoid unexpected behavior.
    • Improved initialization and cleanup to prevent leftover state on failures.

  • Tests

    • CI and unit tests now run with DB-enabled and DB-disabled variants; DB-only tests are conditional.

@ThomasDevoogdt ThomasDevoogdt mentioned this pull request Apr 22, 2025
Closed
@ThomasDevoogdt ThomasDevoogdt force-pushed the bugfix/sqlite3-disabled branch from 4b2f2ad to a767a54 Compare April 23, 2025 09:43
@patrick-stephens patrick-stephens added ok-package-test Run PR packaging tests and removed docs-required labels Apr 24, 2025
@patrick-stephens
Copy link
Copy Markdown
Contributor

patrick-stephens commented Apr 24, 2025

Let's make sure this compiles for all existing targets as well.

Would it be better to disable the features within the plugins that need the DB rather than the whole plugin @leonardo-albertovich ? It feels a bit like a large hammer, e.g. no tail input even if you don't use db. We would have to make the config options and any other usage conditional though so it may be worse.

@ThomasDevoogdt
Copy link
Copy Markdown
Contributor Author

ThomasDevoogdt commented Apr 24, 2025

Let's make sure this compiles for all existing targets as well.

How can it not? Currently, FLB_SQLDB is enabled by default, and will stay like that. The problem is the other way around, if not enabled, then compilation breaks.

Would it be better to disable the features within the plugins that need the DB rather than the whole plugin @leonardo-albertovich ? It feels a bit like a large hammer, e.g. no tail input even if you don't use db. We would have to make the config options and any other usage conditional though so it may be worse.

For me fine, but I would do that on a per feature basis. Perhaps just continue with this PR, and then fine-tune some features that might compile with some small fixups.

In general, it would be much better if all options are toggled in the automated tests, on one reference compilation system. Perhaps even by incremental builds. But either way, I just want to fix compilation now.

@ThomasDevoogdt ThomasDevoogdt force-pushed the bugfix/sqlite3-disabled branch from a767a54 to 8cf7987 Compare May 2, 2025 11:53
@ThomasDevoogdt ThomasDevoogdt force-pushed the bugfix/sqlite3-disabled branch from 8cf7987 to 7092bc1 Compare May 2, 2025 11:54
@ThomasDevoogdt
Copy link
Copy Markdown
Contributor Author

ThomasDevoogdt commented May 2, 2025 •
edited
Loading

@patrick-stephens @edsiper I changed this PR so that plugins are still compiled, but without database support. I hope this answers #10239 (comment).

Tested by doing this:

cd build/
cmake -GNinja -DFLB_SQLDB=OFF ../
ninja

and

cd build/
cmake -GNinja -DFLB_PREFER_SYSTEM_LIBS=ON -DFLB_SQLDB=OFF ../
ninja

@patrick-stephens
Copy link
Copy Markdown
Contributor

patrick-stephens commented May 2, 2025

Not sure if the CI failure is relevant or something else

@ThomasDevoogdt
Copy link
Copy Markdown
Contributor Author

ThomasDevoogdt commented May 2, 2025

Not sure if the CI failure is relevant or something else

I just added #ifdef FLB_HAVE_SQLDB, which is true by default, so I don't think it's relevant.

This was referenced May 5, 2025
Merged
Closed
@ThomasDevoogdt ThomasDevoogdt force-pushed the bugfix/sqlite3-disabled branch from 7092bc1 to 24532cd Compare May 7, 2025 07:28
@ThomasDevoogdt
Copy link
Copy Markdown
Contributor Author

ThomasDevoogdt commented Nov 20, 2025

This looks good to me. However, this review comment seems not to be addressed: https://github.com/fluent/fluent-bit/pull/10239/files#r2492077239 Is it enough with this implementation?

I fixed/added some warnings for when FLB_SQLDB is not compiled. I would think its good to go now?

@cosmo0920
Copy link
Copy Markdown
Contributor

cosmo0920 commented Nov 21, 2025

We fixed no left device error on our CI. So, could you rebase off current master?
We need to confirm all of the CI tasks are passed before processing review.

@ThomasDevoogdt
Copy link
Copy Markdown
Contributor Author

ThomasDevoogdt commented Nov 21, 2025

We fixed no left device error on our CI. So, could you rebase off current master? We need to confirm all of the CI tasks are passed before processing review.

Rebased, and all tests seems to be fine. Can we merge?

@patrick-stephens
Copy link
Copy Markdown
Contributor

patrick-stephens commented Nov 21, 2025

@leonardo-albertovich just checking your comments were covered?

@leonardo-albertovich
Copy link
Copy Markdown
Contributor

leonardo-albertovich commented Nov 21, 2025

I'll review this PR again tomorrow. @ThomasDevoogdt if you have any questions that were unanswered (I think I missed some) please repeat them so I can address them tomorrow.

@ThomasDevoogdt
Copy link
Copy Markdown
Contributor Author

ThomasDevoogdt commented Nov 21, 2025

I'll review this PR again tomorrow. @ThomasDevoogdt if you have any questions that were unanswered (I think I missed some) please repeat them so I can address them tomorrow.

I don't have any open questions. If you see any shortcomings in this PR, then I will try to address them. But beware that I won't do any in depth refactorings, for that, someone closser to the matter should step in.

@ThomasDevoogdt
Copy link
Copy Markdown
Contributor Author

ThomasDevoogdt commented Nov 25, 2025

I'll review this PR again tomorrow. @ThomasDevoogdt if you have any questions that were unanswered (I think I missed some) please repeat them so I can address them tomorrow.

@leonardo-albertovich I think this PR is finalized, it would be nice having it merged. By this, the runtime tests would also start to work, limiting the chance that I have to rework this anytime soon.

@leonardo-albertovich
Copy link
Copy Markdown
Contributor

leonardo-albertovich commented Nov 26, 2025

I don't have merge privileges anymore so you should ask @edsiper about it and wait for him to have time to dedicate to this.

@ThomasDevoogdt
Copy link
Copy Markdown
Contributor Author

ThomasDevoogdt commented Dec 5, 2025

@edsiper @patrick-stephens ping ...

@ThomasDevoogdt
Copy link
Copy Markdown
Contributor Author

ThomasDevoogdt commented Dec 22, 2025

@edsiper @patrick-stephens ping ...

Hi, this week is perhaps the ideal moment to merge it? @edsiper

@ThomasDevoogdt
Copy link
Copy Markdown
Contributor Author

ThomasDevoogdt commented Dec 22, 2025

@patrick-stephens Thx for the approval, can you also merge this?

@ThomasDevoogdt
Copy link
Copy Markdown
Contributor Author

ThomasDevoogdt commented Jan 5, 2026

@patrick-stephens @edsiper Happy New Year! 🎉

It's perhaps the ideal moment to merge this pull request now.

@ThomasDevoogdt
Copy link
Copy Markdown
Contributor Author

ThomasDevoogdt commented Jan 14, 2026

@patrick-stephens @edsiper Again a week later. Can this be merged please?

@ThomasDevoogdt
Copy link
Copy Markdown
Contributor Author

ThomasDevoogdt commented Jan 19, 2026

@patrick-stephens @edsiper ping

@ThomasDevoogdt
Copy link
Copy Markdown
Contributor Author

ThomasDevoogdt commented Jan 27, 2026 •
edited
Loading

@patrick-stephens @edsiper @cosmo0920 ping

@patrick-stephens
Copy link
Copy Markdown
Contributor

patrick-stephens commented Jan 28, 2026

@leonardo-albertovich I think you are happy the changes are done?

@leonardo-albertovich
Copy link
Copy Markdown
Contributor

leonardo-albertovich commented Feb 4, 2026

IIRC it looked fine but I deferred to Eduardo on this one.

@ThomasDevoogdt
Copy link
Copy Markdown
Contributor Author

ThomasDevoogdt commented Feb 15, 2026

@edsiper @leonardo-albertovich @patrick-stephens @cosmo0920 And any other maintainer, I can understand that this doesn't have a high priority, but how much patience do you expect me to have? It takes months and months until I get a proper review, and in the end an approve, to then get buried in the Fluent Bit Next graveyard. I'm fine with applying fixes to maintain fluent-bit in Buildroot, I also saw many of my commits and proposal being reused by other package maintainers, so I guess that they are somewhat worth it. So I would appreciate it that also this one gets merged over time.

E.g. Upstream usage of the FLB_PREFER_SYSTEM_LIB(S) usage:

@ThomasDevoogdt
Copy link
Copy Markdown
Contributor Author

ThomasDevoogdt commented Mar 5, 2026

@celalettin1286 @cosmo0920 @edsiper @niedbalski @patrick-stephens Can anyone review this pull request, and finally also merge this?

@ThomasDevoogdt
Copy link
Copy Markdown
Contributor Author

ThomasDevoogdt commented Mar 25, 2026

@celalettin1286 @cosmo0920 @edsiper @niedbalski @patrick-stephens ping ...

@ThomasDevoogdt
in_blob: fix compilation if FLB_SQLDB (sqlite3) is disabled
54502e8 
Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
@ThomasDevoogdt
filter_checklist: fix compilation if FLB_SQLDB (sqlite3) is disabled
273e7e0 
Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
@ThomasDevoogdt
out_azure_blob: fix compilation if FLB_SQLDB (sqlite3) is disabled
d136594 
Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
@ThomasDevoogdt
blob_db: fix missing methods if not FLB_HAVE_SQLDB
c40e584 
Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
@ThomasDevoogdt
Copy link
Copy Markdown
Contributor Author

ThomasDevoogdt commented Apr 21, 2026

@celalettin1286 @cosmo0920 @edsiper @niedbalski @patrick-stephens ping again ...

How many times do I still have to rebase this until someone merges this PR? This PR is a year old! I'm starting to loose my patience here.

@ThomasDevoogdt
tests: runtime: filter_checklist: fix flb_test_mode_partial if !FLB_H…
1c5521e 
…AVE_SQLDB

Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
@ThomasDevoogdt
tests: runtime_shell: exclude in_tail_expect if not FLB_HAVE_SQLDB
8494344 
Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
@ThomasDevoogdt
workflows: build with FLB_SQLDB=On|Off
3dfcf76 
Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
@ThomasDevoogdt
Copy link
Copy Markdown
Contributor Author

ThomasDevoogdt commented Apr 30, 2026

@leonardo-albertovich ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@edsiper edsiper Awaiting requested review from edsiper edsiper is a code owner

@fujimotos fujimotos Awaiting requested review from fujimotos

@koleini koleini Awaiting requested review from koleini

@niedbalski niedbalski Awaiting requested review from niedbalski niedbalski is a code owner

@celalettin1286 celalettin1286 Awaiting requested review from celalettin1286 celalettin1286 is a code owner

@leonardo-albertovich leonardo-albertovich Awaiting requested review from leonardo-albertovich

@patrick-stephens patrick-stephens Awaiting requested review from patrick-stephens patrick-stephens is a code owner

@cosmo0920 cosmo0920 Awaiting requested review from cosmo0920 cosmo0920 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

ok-package-test Run PR packaging tests

Projects

None yet

Milestone

Fluent Bit Next

Development

Successfully merging this pull request may close these issues.

5 participants

@ThomasDevoogdt @patrick-stephens @edsiper @leonardo-albertovich @cosmo0920