-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Remove duplicate leading slash on $SYSCONFDIR #11364
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
📝 WalkthroughWalkthroughRemoved a leading slash before the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ 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. Comment |
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>
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: 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 whenCMAKE_INSTALL_SYSCONFDIRis 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@). TheFULLvariants are guaranteed to be absolute paths. Without it, the behavior ofCMAKE_INSTALL_SYSCONFDIRdepends 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. UsingCMAKE_INSTALL_FULL_SYSCONFDIRwould ensure consistent behavior across all build configurations.
|
The coderabbitai suggestion of switching |
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:
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:
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
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.