CPACK: Improve debian packaging #10844
Conversation
Signed-off-by: shaerpour <amirhosseinshaerpour1@gmail.com>
Signed-off-by: shaerpour <amirhosseinshaerpour1@gmail.com>
Signed-off-by: shaerpour <amirhosseinshaerpour1@gmail.com>
…ntation' into cpack_debian
aminvakil
requested review from
fujimotos,
niedbalski,
patrick-stephens,
celalettin1286,
edsiper and
koleini
as code owners
WalkthroughAdds a SysV init script for fluent-bit, introduces FLB_INSTALL_INITDIR for platform-specific install paths, installs the init script into that directory, and extends Debian packaging to include prerm, postinst, and postrm scripts that manage service registration and lifecycle during install/upgrade/remove. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin
participant dpkg/APT as dpkg/APT
participant postinst as postinst
participant prerm as prerm
participant postrm as postrm
participant rc.d as update-rc.d
participant rcctl as invoke-rc.d
participant initd as /etc/init.d/fluent-bit
participant daemon as fluent-bit
rect rgba(200,235,255,0.25)
note over dpkg/APT: Install / Configure
Admin->>dpkg/APT: install/upgrade package
dpkg/APT->>postinst: postinst configure [args]
postinst->>initd: chmod +x (if present)
postinst->>rc.d: register defaults
alt first install
postinst->>rcctl: start fluent-bit
else reconfigure/upgrade
postinst->>rcctl: restart fluent-bit
end
rcctl->>initd: start/restart
initd->>daemon: launch or reload
end
rect rgba(255,230,200,0.25)
note over dpkg/APT: Remove / Purge
Admin->>dpkg/APT: remove/purge package
dpkg/APT->>prerm: prerm remove
prerm->>rcctl: stop fluent-bit
rcctl->>initd: stop
initd->>daemon: terminate
dpkg/APT->>postrm: postrm [action]
alt purge
postrm->>rc.d: remove service links
else remove/others
postrm--x rc.d: no-op
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
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.
| PATH=/opt/fluent-bit/bin/:/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin | ||
| DAEMON=/opt/fluent-bit/bin/fluent-bit | ||
| NAME=fluent-bit |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix hard-coded binary path; prefer distro path and robust lookup
/opt is non-FHS for Debian packages and likely wrong if the binary installs to /usr/bin. Use a safe PATH and resolve fluent-bit dynamically.
-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=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
+# Allow override via environment; otherwise resolve, defaulting to /usr/bin
+DAEMON="${DAEMON:-$(command -v fluent-bit 2>/dev/null || echo /usr/bin/fluent-bit)}"
NAME=fluent-bit📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| PATH=/opt/fluent-bit/bin/:/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin | |
| DAEMON=/opt/fluent-bit/bin/fluent-bit | |
| NAME=fluent-bit | |
| PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin | |
| # Allow override via environment; otherwise resolve, defaulting to /usr/bin | |
| DAEMON="${DAEMON:-$(command -v fluent-bit 2>/dev/null || echo /usr/bin/fluent-bit)}" | |
| NAME=fluent-bit |
🤖 Prompt for AI Agents
In conf/fluent-bit around lines 13 to 15, the DAEMON is hard-coded to
/opt/fluent-bit/bin/fluent-bit and PATH includes /opt first which is non-FHS and
may be incorrect on Debian; replace the hard-coded binary path with a dynamic
lookup (e.g., use `command -v` or `which` at startup to resolve fluent-bit into
DAEMON, and fall back to common distro locations like /usr/bin and
/usr/local/bin), update PATH to include standard system directories
(/usr/bin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin) but avoid forcing /opt
first, and ensure the script exits or logs a clear error if the binary cannot be
found.
| start_fluent_bit() { | ||
| # Start the daemon/service | ||
| # | ||
| # Returns: | ||
| # 0 if daemon has been started | ||
| # 1 if daemon was already running | ||
| # 2 if daemon could not be started | ||
| start-stop-daemon --start --quiet --exec $DAEMON 2>/dev/null || return 2 | ||
| } |
There was a problem hiding this comment.
Daemon doesn’t daemonize or use config; start will block or misbehave
Start currently launches without -d or -c; fluent-bit runs foreground by default and won’t pick the packaged config. Quote paths.
- start-stop-daemon --start --quiet --exec $DAEMON 2>/dev/null || return 2
+ start-stop-daemon --start --quiet --exec "$DAEMON" -- -d -c /etc/fluent-bit/fluent-bit.conf || return 2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| start_fluent_bit() { | |
| # Start the daemon/service | |
| # | |
| # Returns: | |
| # 0 if daemon has been started | |
| # 1 if daemon was already running | |
| # 2 if daemon could not be started | |
| start-stop-daemon --start --quiet --exec $DAEMON 2>/dev/null || return 2 | |
| } | |
| start_fluent_bit() { | |
| # Start the daemon/service | |
| # | |
| # Returns: | |
| # 0 if daemon has been started | |
| # 1 if daemon was already running | |
| # 2 if daemon could not be started | |
| start-stop-daemon --start --quiet --exec "$DAEMON" -- -d -c /etc/fluent-bit/fluent-bit.conf || return 2 | |
| } |
🤖 Prompt for AI Agents
In conf/fluent-bit around lines 17 to 25, start_fluent_bit currently calls
start-stop-daemon with unquoted variables and no Fluent Bit flags so it runs in
foreground and may ignore the packaged config; update the start-stop-daemon
invocation to quote $DAEMON and pass the Fluent Bit args after “--” including -c
"<path-to-config>" (or the script's CONF variable) and -d (or --daemonize) so
the daemonizes and uses the correct config file, keep the existing error
handling/return codes and stderr redirection.
| stop_fluent_bit() { | ||
| # Stops the daemon/service | ||
| # | ||
| # Return | ||
| # 0 if daemon has been stopped | ||
| # 1 if daemon was already stopped | ||
| # 2 if daemon could not be stopped | ||
| # other if a failure occurred | ||
| start-stop-daemon --stop --quiet --name $NAME | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
More reliable stop; align identifier with start and avoid failures when not running
Use --exec with the same binary, add retry and oknodo; quote variables.
- start-stop-daemon --stop --quiet --name $NAME
+ start-stop-daemon --stop --quiet --exec "$DAEMON" --retry=TERM/5/KILL/5 --oknodo📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| stop_fluent_bit() { | |
| # Stops the daemon/service | |
| # | |
| # Return | |
| # 0 if daemon has been stopped | |
| # 1 if daemon was already stopped | |
| # 2 if daemon could not be stopped | |
| # other if a failure occurred | |
| start-stop-daemon --stop --quiet --name $NAME | |
| } | |
| stop_fluent_bit() { | |
| # Stops the daemon/service | |
| # | |
| # Return | |
| # 0 if daemon has been stopped | |
| # 1 if daemon was already stopped | |
| # 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 | |
| } |
🤖 Prompt for AI Agents
In conf/fluent-bit around lines 32 to 41, the stop_fluent_bit function uses
start-stop-daemon with --name $NAME which can mismatch the start invocation and
also fails if the process is not running; update the call to quote variables and
align the identifier with the start by using --exec with the exact binary path
(e.g. --exec "$DAEMON"), add --retry with a sensible timeout and --oknodo so
non-running services aren't treated as failures, and keep --quiet; ensure any
variable expansions are quoted to avoid word-splitting.
| if [ "$1" = "configure" ] || [ "$1" = "abort-upgrade" ] || [ "$1" = "abort-deconfigure" ] || [ "$1" = "abort-remove" ] ; then | ||
| if [ -x "/etc/init.d/fluent-bit" ]; then | ||
| update-rc.d fluent-bit defaults >/dev/null | ||
| if [ -n "$2" ]; then | ||
| _dh_action=restart | ||
| else | ||
| _dh_action=start | ||
| fi | ||
| invoke-rc.d fluent-bit $_dh_action || exit 1 | ||
| fi | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer systemd on systemd hosts; start only on configure, not on abort-*
Enable/start via systemctl when available; keep SysV fallback. Limit service start to configure phase.
-if [ "$1" = "configure" ] || [ "$1" = "abort-upgrade" ] || [ "$1" = "abort-deconfigure" ] || [ "$1" = "abort-remove" ] ; then
- if [ -x "/etc/init.d/fluent-bit" ]; then
- update-rc.d fluent-bit defaults >/dev/null
- if [ -n "$2" ]; then
- _dh_action=restart
- else:
- _dh_action=start
- fi
- invoke-rc.d fluent-bit $_dh_action || exit 1
- fi
-fi
+if [ "$1" = "configure" ] ; then
+ if command -v systemctl >/dev/null 2>&1; then
+ systemctl daemon-reload || true
+ if [ -n "$2" ]; then
+ systemctl restart fluent-bit.service || systemctl start fluent-bit.service || true
+ else
+ systemctl start fluent-bit.service || true
+ fi
+ systemctl enable fluent-bit.service || true
+ elif [ -x "/etc/init.d/fluent-bit" ]; then
+ update-rc.d fluent-bit defaults >/dev/null || true
+ if [ -n "$2" ]; then _dh_action=restart; else _dh_action=start; fi
+ invoke-rc.d fluent-bit $_dh_action || true
+ fi
+fiCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cpack/debian/postinst around lines 20-30, the script currently enables/starts
the service for both configure and abort-* actions and prefers SysV; change it
to act only on the "configure" phase and prefer systemd when available: detect
systemd (e.g., check for systemctl and /run/systemd/system), if present use
systemctl enable --now fluent-bit (or systemctl start when only starting) and
check its exit status, otherwise fall back to update-rc.d + invoke-rc.d as
before; ensure the outer conditional only matches "$1" = "configure" (not
abort-*), preserve exit codes on failure, and keep the existing logic for
deciding start vs restart based on $2.
| if [ "$1" = "purge" ] ; then | ||
| update-rc.d fluent-bit remove >/dev/null | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Purge should also disable systemd unit when present
Complement update-rc.d with systemd disable/stop to avoid leftover unit state.
-if [ "$1" = "purge" ] ; then
- update-rc.d fluent-bit remove >/dev/null
-fi
+if [ "$1" = "purge" ] ; then
+ if command -v systemctl >/dev/null 2>&1; then
+ systemctl disable --now fluent-bit.service || true
+ fi
+ update-rc.d fluent-bit remove >/dev/null || true
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ "$1" = "purge" ] ; then | |
| update-rc.d fluent-bit remove >/dev/null | |
| fi | |
| if [ "$1" = "purge" ] ; then | |
| if command -v systemctl >/dev/null 2>&1; then | |
| systemctl disable --now fluent-bit.service || true | |
| fi | |
| update-rc.d fluent-bit remove >/dev/null || true | |
| fi |
🤖 Prompt for AI Agents
In cpack/debian/postrm around lines 20-22, the script only calls update-rc.d on
purge and doesn't handle systemd units, which can leave the service enabled or
running; add logic to detect systemd (e.g., test -x /bin/systemctl or command -v
systemctl) and when present run systemctl disable --now fluent-bit.service (or
systemctl --no-reload --now disable --now) redirecting output to /dev/null and
ignoring non-zero exit codes so removal remains idempotent; keep the existing
update-rc.d call for SysV compatibility and ensure the commands run only for the
"purge" case.
| if [ -x "/etc/init.d/fluent-bit" ] && [ "$1" = remove ]; then | ||
| invoke-rc.d fluent-bit stop || exit 1 | ||
| fi |
There was a problem hiding this comment.
Handle upgrades and deconfigure; don’t fail removal if stop errors
prerm must stop the service on upgrade/deconfigure too; failing the whole dpkg operation on stop errors is brittle.
-if [ -x "/etc/init.d/fluent-bit" ] && [ "$1" = remove ]; then
- invoke-rc.d fluent-bit stop || exit 1
-fi
+case "$1" in
+ remove|upgrade|deconfigure)
+ if [ -x "/etc/init.d/fluent-bit" ]; then
+ invoke-rc.d fluent-bit stop || true
+ fi
+ ;;
+esac📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ -x "/etc/init.d/fluent-bit" ] && [ "$1" = remove ]; then | |
| invoke-rc.d fluent-bit stop || exit 1 | |
| fi | |
| case "$1" in | |
| remove|upgrade|deconfigure) | |
| if [ -x "/etc/init.d/fluent-bit" ]; then | |
| invoke-rc.d fluent-bit stop || true | |
| fi | |
| ;; | |
| esac |
🤖 Prompt for AI Agents
In cpack/debian/prerm around lines 4-6, the script only stops the service on
remove and exits on failure; change the condition to also match upgrade and
deconfigure actions (e.g. check for remove, upgrade or deconfigure) and avoid
aborting the entire dpkg operation on stop failure — call invoke-rc.d fluent-bit
stop but swallow errors (replace "|| exit 1" with a no-op like "|| true" or "||
:") so upgrades/deconfigures proceed even if stop fails.
| install(FILES | ||
| "${PROJECT_SOURCE_DIR}/conf/fluent-bit" | ||
| COMPONENT binary | ||
| DESTINATION ${FLB_INSTALL_INITDIR}) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Install as executable and gate SysV init only when systemd/upstart are absent
Avoid installing SysV init when a native systemd unit is present; also ensure the script is installed executable.
-install(FILES
- "${PROJECT_SOURCE_DIR}/conf/fluent-bit"
- COMPONENT binary
- DESTINATION ${FLB_INSTALL_INITDIR})
+if(NOT SYSTEMD_UNITDIR AND NOT IS_DIRECTORY /usr/share/upstart)
+ install(PROGRAMS
+ "${PROJECT_SOURCE_DIR}/conf/fluent-bit"
+ COMPONENT binary
+ DESTINATION ${FLB_INSTALL_INITDIR})
+endif()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| install(FILES | |
| "${PROJECT_SOURCE_DIR}/conf/fluent-bit" | |
| COMPONENT binary | |
| DESTINATION ${FLB_INSTALL_INITDIR}) | |
| if(NOT SYSTEMD_UNITDIR AND NOT IS_DIRECTORY /usr/share/upstart) | |
| install(PROGRAMS | |
| "${PROJECT_SOURCE_DIR}/conf/fluent-bit" | |
| COMPONENT binary | |
| DESTINATION ${FLB_INSTALL_INITDIR}) | |
| endif() |
🤖 Prompt for AI Agents
In src/CMakeLists.txt around lines 664-668, replace the unconditional
install(FILES ...) of the SysV init script with an install(PROGRAMS ...) so the
script is installed executable, and wrap that install in a conditional that only
runs when no native systemd or upstart unit is being installed (e.g. if(NOT
FLB_HAVE_SYSTEMD AND NOT FLB_HAVE_UPSTART) or by checking for the presence of
your systemd/upstart unit variables/files); specifically change the FILES
install to PROGRAMS and move it inside the conditional so the SysV init script
is only installed (and marked executable) when systemd/upstart are absent.
💡 Verification agent
🧩 Analysis chain
Verify FLB_INSTALL_INITDIR definition and Debian path
Confirm FLB_INSTALL_INITDIR resolves to /etc/init.d on Debian derivatives and is unset on Windows to avoid nonsense installs.
🏁 Script executed:
#!/bin/bash
# Find FLB_INSTALL_INITDIR definition and sample values
rg -nP 'FLB_INSTALL_INITDIR' -C2
# Ensure conf/fluent-bit has +x in the repo (or rely on PROGRAMS)
ls -l conf/fluent-bit || trueLength of output: 961
Wrap init script installation under if(UNIX)
Wrap the install(FILES "${PROJECT_SOURCE_DIR}/conf/fluent-bit" COMPONENT binary DESTINATION ${FLB_INSTALL_INITDIR}) block in an if(UNIX) (or clear FLB_INSTALL_INITDIR on Windows) so that on Windows—where it’s currently set to "init.d"—the init script isn’t installed to a meaningless location.
src/CMakeLists.txt:664-668
conf/fluent-bit is already executable.
🤖 Prompt for AI Agents
In src/CMakeLists.txt around lines 664 to 668, the install(FILES ...) call
installs the init script unconditionally which places it into a meaningless
"init.d" path on Windows; wrap that install block in an if(UNIX) ... endif() so
it only runs on Unix-like systems (or alternatively explicitly clear/unset
FLB_INSTALL_INITDIR on Windows before this install), ensuring the
conf/fluent-bit init script is only installed on Unix.
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