Skip to content

Conversation

@Elwell
Copy link

@Elwell Elwell commented Jan 13, 2026

There's quite a bit of history on this edit - it's been added and removed several times over the years. Current CMake policy (CMP0192) says it's an absolute rather than relative path.

Closes #6619


Testing
Before we can approve your change; please submit the following in a comment:

  • [N/A] Example configuration file for the change
  • [N/A] Debug log output from testing the change
  • [N/A] Attached Valgrind output that shows no leaks or memory corruption was found

This is a packaging change for the generation of a systemd unit file and does not alter any internals of fluent-bit.

Typical configuration on a fresh install of 4.2.2 looks like this before the change:

[root@poddy ~]# rpm -ql fluent-bit | grep serv
/usr/lib/systemd/system/fluent-bit.service
[root@poddy ~]# grep ExecStart /usr/lib/systemd/system/fluent-bit.service
ExecStart=/opt/fluent-bit/bin/fluent-bit -c //etc/fluent-bit/fluent-bit.conf
[root@poddy ~]#

Documentation

  • Documentation required for this feature

Backporting

  • Backport to latest stable release.

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

Release Notes

  • Chores
    • Updated service configuration path format to a relative-style layout, improving startup and configuration loading.
    • No changes to environment file handling or restart behavior; runtime behavior remains unchanged.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Jan 13, 2026

📝 Walkthrough

Walkthrough

Removed a leading slash before the CMAKE_INSTALL_SYSCONFDIR variable in the ExecStart line of the systemd unit template to avoid producing a double slash in the configuration file path.

Changes

Cohort / File(s) Summary
Systemd service template
init/systemd.in
Removed the leading / before @CMAKE_INSTALL_SYSCONFDIR@ in the ExecStart command so the -c option no longer yields //... when expanded.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Suggested reviewers

  • edsiper

Poem

A rabbit hopped in, with whiskers bright,
Found a slash that doubled in sight,
Nibbled one off, neat and spry,
Now the config path runs tidy and shy 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: removing a duplicate leading slash in the systemd unit file path configuration.
Linked Issues check ✅ Passed The PR directly addresses issue #6619 by fixing the double slash in ExecStart path (//etc to /etc) as required.
Out of Scope Changes check ✅ Passed The change is narrowly scoped to init/systemd.in only, directly fixing the reported double-slash issue without unrelated modifications.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

There's quite a bit of history on this edit - it's been added and removed
several times over the years. Current CMake policy (CMP0192) says it's an
absolute rather than relative path.

Closes fluent#6619

Signed-off-by: Andrew Elwell <Andrew.Elwell@gmail.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
init/systemd.in (1)

11-11: Fix correctly addresses the double-slash issue.

Removing the leading slash before @CMAKE_INSTALL_SYSCONFDIR@ properly resolves the //etc/... path problem when CMAKE_INSTALL_SYSCONFDIR is set to an absolute path.

However, consider using @CMAKE_INSTALL_FULL_SYSCONFDIR@ instead to match the pattern used for the binary path (@CMAKE_INSTALL_FULL_BINDIR@). The FULL variants are guaranteed to be absolute paths. Without it, the behavior of CMAKE_INSTALL_SYSCONFDIR depends on the CMake policy CMP0192 and the CMAKE_INSTALL_PREFIX setting—it will only be absolute when CMP0192 is NEW and CMAKE_INSTALL_PREFIX is a special prefix like / or /usr. Using CMAKE_INSTALL_FULL_SYSCONFDIR would ensure consistent behavior across all build configurations.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5bb2b6c and 7c317b1.

📒 Files selected for processing (1)
  • init/systemd.in

@Elwell
Copy link
Author

Elwell commented Jan 13, 2026

The coderabbitai suggestion of switching @CMAKE_INSTALL_SYSCONFDIR@ to @CMAKE_INSTALL_FULL_SYSCONFDIR@ sounds like it would be the ideal fix to ensure that there's always a fully qualified path for the config file. The potential issue is that the binary packaging presently uses /opt/fluent-bit/ as the destinaton, and /opt is apparently handled as a special case and uses /etc/opt/ for sysconfig as per FHS.
I suspect this would therefore need testing by people with access to the build farm to ensure it didn't break all the variants.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inconsistency in CMAKE_INSTALL_SYSCONFDIR

2 participants