CPACK: Improve debian packaging #10844
Conversation
aminvakil
requested review from
fujimotos,
niedbalski,
patrick-stephens,
celalettin1286,
edsiper and
koleini
as code owners
|
Warning Rate limit exceeded@aminvakil has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughAdds a SysV LSB init script for fluent-bit, introduces a platform-aware CMake variable Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin
participant APT as dpkg/APT
participant postinst as postinst
participant prerm as prerm
participant postrm as postrm
participant rc_d as update-rc.d
participant invoke as invoke-rc.d
participant initd as /etc/init.d/fluent-bit
participant daemon as fluent-bit
rect #D6F5FF
Admin->>APT: install/upgrade package
APT->>postinst: run postinst (configure)
postinst->>initd: chmod +x (if present)
postinst->>rc_d: register defaults
alt fresh install
postinst->>invoke: invoke-rc.d start
else upgrade/reconfigure
postinst->>invoke: invoke-rc.d restart
end
invoke->>initd: start/restart
initd->>daemon: launch/reload
end
rect #FFF2D6
Admin->>APT: remove/purge package
APT->>prerm: run prerm (remove)
prerm->>invoke: invoke-rc.d stop
invoke->>initd: stop
initd->>daemon: terminate
APT->>postrm: run postrm (action)
alt purge
postrm->>rc_d: update-rc.d remove
else other actions
postrm--x rc_d: no-op
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This is still WIP, there are a couple of comments from @patrick-stephens which needs to be addressed: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
conf/fluent-bit (1)
71-73: Usage mentions reload/status but they’re not implementedEither implement status (preferred) or remove from usage to avoid misleading users.
Minimal status implementation (additions shown):
+status_fluent_bit() { + start-stop-daemon --status --exec "$DAEMON" +} @@ - echo "Usage: $NAME {start|stop|restart|reload|force-reload|status|configtest}" >&2 + echo "Usage: $NAME {start|stop|restart|status|configtest}" >&2Hook into case:
@@ configtest|testconfig) test_config ;; + status) + status_fluent_bit + ;;cpack/debian/postinst (1)
5-8: chmod workaround becomes unnecessary if CMake installs as PROGRAMSIf you adopt install(PROGRAMS …), this chmod can be dropped.
- configure) - chmod +x /etc/init.d/fluent-bit - ;; + configure) + # kept for now; safe to remove once init script is installed via CMake as PROGRAMS + [ -x /etc/init.d/fluent-bit ] || chmod +x /etc/init.d/fluent-bit || true + ;;CMakeLists.txt (1)
1390-1390: Make FLB_INSTALL_INITDIR user-overridable and align path semanticsExpose as a CACHE PATH so packagers can override easily, and drop the trailing slash on Unix for consistency with typical CMake DESTINATION usage. Also ensure any install() using this dir is gated for non-Windows only.
Apply:
- set(FLB_INSTALL_INITDIR "init.d") + set(FLB_INSTALL_INITDIR "init.d" CACHE PATH "Install dir for SysV init script") - set(FLB_INSTALL_INITDIR "/etc/init.d/") + set(FLB_INSTALL_INITDIR "/etc/init.d" CACHE PATH "Install dir for SysV init script")Please confirm any install(FILES ... DESTINATION ${FLB_INSTALL_INITDIR}) for the SysV script is wrapped with if(NOT FLB_SYSTEM_WINDOWS). Would you like me to propose that guard in src/CMakeLists.txt?
Also applies to: 1396-1396
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CMakeLists.txt(2 hunks)conf/fluent-bit(1 hunks)cpack/debian/postinst(1 hunks)cpack/debian/postrm(1 hunks)cpack/debian/prerm(1 hunks)src/CMakeLists.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
PR: fluent/fluent-bit#9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
src/CMakeLists.txtCMakeLists.txt
🔇 Additional comments (3)
conf/fluent-bit (1)
27-30: No change required: --dry-run is the correct config-test flag
--dry-run (long) or -D (short) is supported in Fluent Bit 2.x for testing configurations; -t is the tag filter flag.cpack/debian/postinst (1)
5-18: Confirm prerm/postinst coverage and packaging tests
Local build script didn’t produce logs in this environment (missingtee). Run in your dev setup:./packaging/local-build-all.sh > local-build-all.log 2>&1Attach the resulting log and confirm your
prerm/postinstscripts handle all dpkg states as per Debian policy.CMakeLists.txt (1)
1475-1481: Verify DEB control file contents locally
cpack/debian/{prerm,postinst,postrm} already have 100755 mode; runcpack -G DEBthendpkg-deb -e <package>.debto confirm exactly one postinst/prerm and that conffiles/postrm are included.
aminvakil
force-pushed
the
cpack_debian
branch
from
0cd025a to
515786b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
conf/fluent-bit (1)
18-26: Fix start: wrong arg parsing, no daemonization, and exit-code mapping
-c $CONFFILEis being parsed by start-stop-daemon (not passed to fluent-bit). Also, fluent-bit will run in foreground without-d, and the function collapses all non‑zero exits to 2 (losing the “already running” 1). Quote vars and pass args after--.start_fluent_bit() { @@ - start-stop-daemon --start --quiet --exec $DAEMON -c $CONFFILE 2>/dev/null || return 2 + start-stop-daemon --start --quiet --exec "$DAEMON" -- -d -c "$CONFFILE" + rc=$? + case "$rc" in + 0|1) return "$rc" ;; # 0: started, 1: already running + *) return 2 ;; + esac }
🧹 Nitpick comments (4)
conf/fluent-bit (4)
28-31: Harden config test: quote vars and validate preconditionsEnsure binary is present and config readable; quote expansions.
test_config() { # Test the fluent_bit configuration - $DAEMON --dry-run -c $CONFFILE + if [ ! -x "$DAEMON" ]; then + echo "$DAEMON not installed or not executable" + return 5 + fi + if [ ! -r "$CONFFILE" ]; then + echo "$CONFFILE not found or not readable" + return 6 + fi + "$DAEMON" --dry-run -c "$CONFFILE" }
33-42: Align stop() docs with behavior of --oknodoWith
--oknodo, “already stopped” returns 0, not 1. Update comment to avoid confusion.stop_fluent_bit() { # Stops the daemon/service # - # Return - # 0 if daemon has been stopped - # 1 if daemon was already stopped + # Returns: + # 0 if daemon has been stopped or was already stopped (--oknodo) # 2 if daemon could not be stopped # other if a failure occurred start-stop-daemon --stop --quiet --exec "$DAEMON" --retry=TERM/5/KILL/5 --oknodo }
13-16: Keep /opt default, but allow override and quoteAcknowledging your packaging choice for non‑Debian sources (/opt) per prior discussion; make DAEMON overridable and quote assignments.
-PATH=/opt/fluent-bit/bin/:/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin -DAEMON=/opt/fluent-bit/bin/fluent-bit +PATH=/opt/fluent-bit/bin/:/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin +DAEMON="${DAEMON:-/opt/fluent-bit/bin/fluent-bit}" NAME=fluent-bit -CONFFILE=${CONFFILE:-/etc/fluent-bit/fluent-bit.conf} +CONFFILE="${CONFFILE:-/etc/fluent-bit/fluent-bit.conf}"
53-55: Nit: naming consistency in commentUse “fluent-bit” (hyphen) consistently.
- # Check configuration before stopping fluent_bit + # Check configuration before stopping fluent-bit
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
conf/fluent-bit(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: aminvakil
PR: fluent/fluent-bit#10844
File: conf/fluent-bit:13-15
Timestamp: 2025-09-14T09:46:09.504Z
Learning: For fluent-bit Debian packaging, /opt/fluent-bit/bin/ is the appropriate installation path since the package may be installed from non-official Debian sources, making /opt compliant with FHS for optional software packages.
📚 Learning: 2025-09-14T09:46:09.504Z
Learnt from: aminvakil
PR: fluent/fluent-bit#10844
File: conf/fluent-bit:13-15
Timestamp: 2025-09-14T09:46:09.504Z
Learning: For fluent-bit Debian packaging, /opt/fluent-bit/bin/ is the appropriate installation path since the package may be installed from non-official Debian sources, making /opt compliant with FHS for optional software packages.
Applied to files:
conf/fluent-bit
aminvakil
force-pushed
the
cpack_debian
branch
from
c157f31 to
4740b80
Compare
Signed-off-by: shaerpour <amirhosseinshaerpour1@gmail.com> Signed-off-by: Amin Vakil <info@aminvakil.com>
Signed-off-by: shaerpour <amirhosseinshaerpour1@gmail.com> Signed-off-by: Amin Vakil <info@aminvakil.com>
Signed-off-by: shaerpour <amirhosseinshaerpour1@gmail.com> Signed-off-by: Amin Vakil <info@aminvakil.com>
Signed-off-by: Amin Vakil <info@aminvakil.com>
Signed-off-by: Amin Vakil <info@aminvakil.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Amin Vakil <info@aminvakil.com>
Signed-off-by: Amin Vakil <info@aminvakil.com>
Signed-off-by: Amin Vakil <info@aminvakil.com>
Signed-off-by: Amin Vakil <info@aminvakil.com>
Also do not fail removal if stop errors Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Amin Vakil <info@aminvakil.com>
aminvakil
force-pushed
the
cpack_debian
branch
from
4740b80 to
04091cd
Compare
|
OK, so I'm trying to understand CMake stuff to address @patrick-stephens comment and I've just saw this: Line 631 in 60db310 And this has comment has been there since 2016 in d14d3fb. I suppose this PR has to change a lot more, and I'm not going to continue this work. I will open another PR addressing the real issue which is |
Thanks to @shaerpour for his work originally on this PR!
This PR attempts to take over #8341 as he cannot continue to work on that.
Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Chores