-
Notifications
You must be signed in to change notification settings - Fork 1.9k
build: execute daemon-reload and restart postinst in deb #10899
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Initial attempt on |
40165cb to
9c593e6
Compare
|
I've commented anything other debian 11 and debian 12 and it's not working. |
911efe8 to
b26ddd2
Compare
|
This has broken other stuff Currently it is: Also |
b26ddd2 to
e266230
Compare
|
It's correct now. It restarts service when it's already started, it does not start the service if it has not been started already. For testing with to in CMakelists.txt, build package and test it. |
|
With in CMakelists.txt |
|
There is another issue in It's ready for review now. |
ea13cfe to
716357e
Compare
|
I do not know the policies for this repository, but I had Gemini's help for creating this PR, but I've tested final deb file on a debian bullseye as stated in my comments. |
|
Note to myself: Is this the correct approach or should I use |
716357e to
e0d552b
Compare
|
Failing tests are irrelevant to this PR: There is also windows fix which is failing. |
8d80332 to
be18f35
Compare
|
Can you rebase? The awscli issue is related to the arm image but it's resolved in master |
Yep, I saw that and already rebased. |
be18f35 to
a7fd450
Compare
|
Failing tests are irrelevant to this PR. |
85f67d9 to
3c3a743
Compare
📝 WalkthroughWalkthroughIntroduces a templated Debian post-install script and updates CMake packaging flow to generate it via configure_file. Accumulates package control extras using list(APPEND), ensuring generated postinst and existing prerm are included. Replaces inline file writes with a variable-based command list for postinst content. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant CMake as CMake (configure)
participant CFG as configure_file
participant CPack as CPack (package)
participant dpkg as dpkg (install)
participant Post as postinst script
participant systemd as systemd
Dev->>CMake: Configure project
CMake->>CFG: Generate scripts/postinst from cpack/debian/postinst.in
Note right of CFG: Inject LDCONFIG_SCRIPT_CMDS into template
CMake->>CPack: Set CPACK_DEBIAN_RUNTIME_PACKAGE_CONTROL_EXTRA (append postinst, prerm)
Dev->>CPack: Build Debian package
dpkg->>Post: Run postinst (configure/abort-*)
Post->>Post: Execute placeholder post-install commands
alt systemd available
Post->>systemd: daemon-reload (quiet)
Post->>systemd: restart FLB_OUT_NAME (best-effort)
else no systemd
Note right of Post: Skip daemon-reload/restart
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CMakeLists.txt (1)
1488-1501: Broken packaging when FLB_RUN_LDCONFIG=OFF (unconditional prerm reference) + missing scripts dir + wrong var name for template.
- prerm is always appended (Line 1500), but only created when FLB_RUN_LDCONFIG is ON.
- ${PROJECT_BINARY_DIR}/scripts/ may not exist before file(WRITE) and configure_file, causing failures.
- Variable name should match postinst.in’s @LDCONFIG_POSTINST_CMDS@.
Fix all in one go:
Apply this diff:
- set(LDCONFIG_SCRIPT_CMDS "") - if(FLB_RUN_LDCONFIG) - set(LDCONFIG_DIR ${FLB_INSTALL_LIBDIR}) - set(LDCONFIG_SCRIPT_CMDS " + # Ensure scripts directory exists for generated maintainer scripts + file(MAKE_DIRECTORY "${PROJECT_BINARY_DIR}/scripts") + + # Content injected into postinst when ldconfig is enabled + set(LDCONFIG_POSTINST_CMDS "") + if(FLB_RUN_LDCONFIG) + set(LDCONFIG_DIR "${FLB_INSTALL_LIBDIR}") + set(LDCONFIG_POSTINST_CMDS " mkdir -p /etc/ld.so.conf.d echo \"${LDCONFIG_DIR}\" > /etc/ld.so.conf.d/libfluent-bit.conf ldconfig ") - file(WRITE ${PROJECT_BINARY_DIR}/scripts/prerm " + file(WRITE "${PROJECT_BINARY_DIR}/scripts/prerm" " rm -f -- /etc/ld.so.conf.d/libfluent-bit.conf ldconfig ") - list(APPEND CPACK_DEBIAN_RUNTIME_PACKAGE_CONTROL_EXTRA "${PROJECT_BINARY_DIR}/scripts/postinst;${PROJECT_BINARY_DIR}/scripts/prerm") + # Ensure executable bit for dpkg maintainer script + execute_process(COMMAND "${CMAKE_COMMAND}" -E chmod +x "${PROJECT_BINARY_DIR}/scripts/prerm") + list(APPEND CPACK_DEBIAN_RUNTIME_PACKAGE_CONTROL_EXTRA "${PROJECT_BINARY_DIR}/scripts/prerm") endif(FLB_RUN_LDCONFIG)
🧹 Nitpick comments (2)
cpack/debian/postinst.in (2)
4-9: Limit service actions to configure; avoid try-restart on abort- phases.*Running daemon-reload/try-restart during abort-* is unnecessary and unexpected for Debian maintainer scripts. Keep these to the configure phase.
Apply this diff:
-if [ "$1" = "configure" ] || [ "$1" = "abort-upgrade" ] || [ "$1" = "abort-deconfigure" ] || [ "$1" = "abort-remove" ] ; then +if [ "$1" = "configure" ] ; then if [ -d /run/systemd/system ]; then systemctl --system daemon-reload >/dev/null || true systemctl try-restart @[email protected] >/dev/null || true fi fi
5-8: Prefer deb-systemd-invoke to systemctl for maintainer scripts.deb-systemd-invoke handles non-systemd environments and policy nuances better than calling systemctl directly.
Consider:
- systemctl --system daemon-reload >/dev/null || true - systemctl try-restart @[email protected] >/dev/null || true + if command -v deb-systemd-invoke >/dev/null 2>&1; then + deb-systemd-invoke --system daemon-reload >/dev/null || true + deb-systemd-invoke try-restart @[email protected] >/dev/null || true + else + systemctl --system daemon-reload >/dev/null || true + systemctl try-restart @[email protected] >/dev/null || true + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CMakeLists.txt(2 hunks)cpack/debian/postinst.in(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-14T17:23:01.225Z
Learnt from: aminvakil
PR: fluent/fluent-bit#10844
File: src/CMakeLists.txt:664-668
Timestamp: 2025-09-14T17:23:01.225Z
Learning: Many popular Debian packages like nginx and fail2ban install both init.d scripts and systemd services simultaneously, following standard Debian packaging practices where systemd automatically prefers the native service file over the SysV init script when both are present.
Applied to files:
CMakeLists.txt
📚 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:
CMakeLists.txt
📚 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, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Applied to files:
CMakeLists.txt
📚 Learning: 2025-09-14T09:46:09.531Z
Learnt from: aminvakil
PR: fluent/fluent-bit#10844
File: conf/fluent-bit:13-15
Timestamp: 2025-09-14T09:46:09.531Z
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:
CMakeLists.txt
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
3c3a743 to
7e08514
Compare
7e08514 to
23b785f
Compare
Signed-off-by: Amin Vakil <[email protected]> Improve systemd daemon-reload command by adding default Signed-off-by: Amin Vakil <[email protected]> Put debian/postinst.in in separate file Signed-off-by: Amin Vakil <[email protected]> Execute systemd stuff if ran by systemd Signed-off-by: Amin Vakil <[email protected]> Append postinst once, and only append prerm when it exists Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Amin Vakil <[email protected]> Fix variable name Signed-off-by: Amin Vakil <[email protected]> ensure correct perms Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Amin Vakil <[email protected]>
e9cc193 to
b3ef75d
Compare
Follow up to #10844 and #8341 and #8330.
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).Documentation
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