Enable ntp if autodetect ntpd or timesyncd#597
Enable ntp if autodetect ntpd or timesyncd#597ColinMcInnes wants to merge 6 commits intoNetworkConfiguration:masterfrom
Conversation
If --enable-ntpd or --enable-timesyncd, assume --enable-ntp and choose the appropriate hook file. Will complain if --enable-ntpd or --enable-timesyncd are both set, allows override of autodetect. Autodetect will enable ntp functionality unless explicitly told not to via --disable-ntp. If --disable-ntp is used, no time hooks are installed. Addresses NetworkConfiguration#574
WalkthroughAdd explicit time-service flags ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
configure (2)
1711-1728: Autodetection runs even when service is explicitly enabled.When
--enable-ntpdis passed,NTPDis alreadyyes, but the script still runs ntpd/chronyd detection. This is redundant and produces confusing output (e.g., "not found" messages even though the service was explicitly enabled).Consider skipping autodetection when the service flag is already set:
Proposed refactor
- printf "Checking for ntpd ... " - NTPD_PATH=$(_which ntpd) - if [ -n "$NTPD_PATH" ]; then - NTPD=yes - NTP=yes - echo "$NTPD_PATH (Selecting 50-ntp.conf)" - else - echo "not found" - fi + if [ "$NTPD" != "yes" ]; then + printf "Checking for ntpd ... " + NTPD_PATH=$(_which ntpd) + if [ -n "$NTPD_PATH" ]; then + NTPD=yes + NTP=yes + echo "$NTPD_PATH (Selecting 50-ntp.conf)" + else + echo "not found" + fi + fiApply similar logic to chronyd and timesyncd detection blocks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configure` around lines 1711 - 1728, The ntpd/chronyd autodetection runs unconditionally even when the service is explicitly enabled, causing confusing "not found" output; modify the detection blocks that set NTPD_PATH and CHRONYD_PATH to first check the NTPD flag (e.g., test if NTPD is empty or not "yes") and skip calling _which and the echo "not found" branch when NTPD is already set, so only run the detection logic when NTPD is unset and preserve setting NTPD=yes, NTP=yes and the "(Selecting 50-ntp.conf)" message only when a binary is actually found.
42-44:CHRONYDvariable is initialized but never used.The script uses
CHRONYD_PATHto store the detected path and setsNTPD=yeswhen chronyd is found, making theCHRONYDvariable dead code. Consider removing it or using it consistently withNTPDandTIMESYNCD.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configure` around lines 42 - 44, The CHRONYD variable is declared but never used; either remove CHRONYD entirely or make it consistent with the other toggles by setting and using it when chronyd is detected. Update the detection block that currently sets CHRONYD_PATH and NTPD=yes to also set CHRONYD=yes (or remove the CHRONYD declaration at the top and any references) and ensure any later logic that checks NTPD or TIMESYNCD uses the correct variable(s) (CHRONYD, NTPD, TIMESYNCD) consistently so there is no dead/unreferenced variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@configure`:
- Around line 1731-1733: The script currently echoes a warning only when both
NTPD_PATH and CHRONYD_PATH are set; add a similar warning when either NTPD_PATH
or CHRONYD_PATH is set together with TIMESYNCD_PATH so users know 50-ntp.conf
will be chosen over 50-timesyncd.conf. Locate the block referencing NTPD_PATH
and CHRONYD_PATH and insert a check like "if [ -n \"$TIMESYNCD_PATH\" ] && ([ -n
\"$NTPD_PATH\" ] || [ -n \"$CHRONYD_PATH\" ])" and echo a clear warning
indicating both an ntp implementation and timesyncd were detected and that
50-ntp.conf will be preferred over 50-timesyncd.conf.
- Around line 1706-1710: The mutual-exclusivity check for NTPD vs TIMESYNCD is
currently nested inside the HOOKSET conditional and gets skipped when
HOOKSET/--with-hooks is set; relocate the check that tests NTP, NTPD and
TIMESYNCD (the if ! [ "$NTP" = "no" ]; then ... if [ "$NTPD" = "yes" ] && [
"$TIMESYNCD" = "yes" ]; then ... exit 1 fi fi block) so it runs unconditionally
after option parsing (or immediately after the code that sets HOOKSET), ensuring
that the conflict between --enable-ntpd and --enable-timesyncd is detected even
when HOOKSET/--with-hooks is used.
---
Nitpick comments:
In `@configure`:
- Around line 1711-1728: The ntpd/chronyd autodetection runs unconditionally
even when the service is explicitly enabled, causing confusing "not found"
output; modify the detection blocks that set NTPD_PATH and CHRONYD_PATH to first
check the NTPD flag (e.g., test if NTPD is empty or not "yes") and skip calling
_which and the echo "not found" branch when NTPD is already set, so only run the
detection logic when NTPD is unset and preserve setting NTPD=yes, NTP=yes and
the "(Selecting 50-ntp.conf)" message only when a binary is actually found.
- Around line 42-44: The CHRONYD variable is declared but never used; either
remove CHRONYD entirely or make it consistent with the other toggles by setting
and using it when chronyd is detected. Update the detection block that currently
sets CHRONYD_PATH and NTPD=yes to also set CHRONYD=yes (or remove the CHRONYD
declaration at the top and any references) and ensure any later logic that
checks NTPD or TIMESYNCD uses the correct variable(s) (CHRONYD, NTPD, TIMESYNCD)
consistently so there is no dead/unreferenced variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Addressing AI review comments.
Skips autodetection.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@configure`:
- Around line 1713-1734: The autodetect block currently sets NTPD=yes when NTPD
is unset, which lets ntpd/chronyd detection override an explicit TIMESYNCD=yes;
modify the logic in the detection around the _which checks (ntpd and chronyd) so
they only set NTPD and NTP when TIMESYNCD is not explicitly set to yes (i.e.
guard the assignments to NTPD/NTP behind a check of TIMESYNCD and/or NTPD being
empty), and ensure the hook selection later still respects explicit TIMESYNCD by
not changing NTPD when TIMESYNCD=yes or when NTPD was explicitly set by the
user. Ensure you reference and update the detection code that uses _which, the
NTPD/NTP assignments, and the branch that chooses 50-ntp.conf so explicit
TIMESYNCD is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
enabled ntpd/chrony, timesyncd, or other HOOKSCRIPT method.
Service selection should disable autodetect of the other service.
|
Why would it complain if both --enable-ntpd and --enable-timesyncd are set? A distro might ship both hooks. |
I was focusing on fixing the autodetect, but that's a good point, you might not know at compile time which one you'd end up with at the end point, so if --enable is used it should not complain and allow both. @perkelix if it's configured for both, I see that timesyncd.conf will exit if it's not present at runtime, but I don't see ntpd.conf doing the same, is that ok? that might result in it complaining loudly if timesyncd is installed on the target system. |
|
Obviously ntp.conf should also exit if it doesn't find the right daemon on the system. |
It looks like all it really does is configure/deconfigure the ntp.conf file for an ntp daemon and quietly requests the system restart the service, throwing errors to dev/null. So I think we're ok there. |
exit hooks determine at runtime if the required feature is present on the target system, so allow both ntpd and timesyncd to be enabled by configure so it packages both sets of exit hooks (and likewise enables ntp support in dhcpcd)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@configure`:
- Around line 1742-1750: The configure script only checks
/usr/lib/systemd/systemd-timesyncd when setting TIMESYNCD/TIMESYNCD_PATH/NTP,
but the runtime hook hooks/50-timesyncd.conf expects
/lib/systemd/systemd-timesyncd; update the autodetect loop in the configure
logic that sets TIMESYNCD and TIMESYNCD_PATH to probe both paths (at least
/usr/lib/systemd/systemd-timesyncd and /lib/systemd/systemd-timesyncd) and set
NTP=yes when one is executable so the hook installation decision matches the
runtime hook path check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| if [ -z "$TIMESYNCD" ]; then | ||
| TIMESYNCD_PATH= | ||
| for x in /usr/lib/systemd/systemd-timesyncd; do | ||
| if [ -x "$x" ]; then | ||
| TIMESYNCD=yes | ||
| TIMESYNCD_PATH=$x | ||
| NTP=yes | ||
| break | ||
| fi |
There was a problem hiding this comment.
Check the same timesyncd path that the shipped hook uses.
The autodetect loop only probes /usr/lib/systemd/systemd-timesyncd, but hooks/50-timesyncd.conf checks /lib/systemd/systemd-timesyncd at runtime. On systems that only expose the /lib/... path, configure will miss timesyncd and skip installing the hook even though the runtime hook would work.
Proposed fix
- for x in /usr/lib/systemd/systemd-timesyncd; do
+ for x in /lib/systemd/systemd-timesyncd \
+ /usr/lib/systemd/systemd-timesyncd; do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@configure` around lines 1742 - 1750, The configure script only checks
/usr/lib/systemd/systemd-timesyncd when setting TIMESYNCD/TIMESYNCD_PATH/NTP,
but the runtime hook hooks/50-timesyncd.conf expects
/lib/systemd/systemd-timesyncd; update the autodetect loop in the configure
logic that sets TIMESYNCD and TIMESYNCD_PATH to probe both paths (at least
/usr/lib/systemd/systemd-timesyncd and /lib/systemd/systemd-timesyncd) and set
NTP=yes when one is executable so the hook installation decision matches the
runtime hook path check.
There was a problem hiding this comment.
Should update the timesyncd.conf as well, have it check both paths.
There was a problem hiding this comment.
🧩 Analysis chain
Good idea! Let me check the current content of 50-timesyncd.conf to propose the right fix.
🏁 Script executed:
#!/bin/bash
fd -i "timesyncd" --type f
fd -i "timesyncd" --type f | xargs cat
Length of output: 1505
@ColinMcInnes — agreed, both sides need updating. Here are the two changes:
1. configure — add /lib/systemd/systemd-timesyncd to the autodetect loop
- for x in /usr/lib/systemd/systemd-timesyncd; do
+ for x in /lib/systemd/systemd-timesyncd \
+ /usr/lib/systemd/systemd-timesyncd; do2. hooks/50-timesyncd.conf — guard against both paths being absent
-if [ ! -x /lib/systemd/systemd-timesyncd ]; then
- return
-fi
+if [ ! -x /lib/systemd/systemd-timesyncd ] && \
+ [ ! -x /usr/lib/systemd/systemd-timesyncd ]; then
+ return
+fiThis keeps configure's detection and the hook's runtime guard in sync — both will accept either installation path. On systems that merge /lib into /usr/lib (e.g. via a symlink), the loop in configure will naturally find it under whichever path exists first, and the hook will do the same.
2 participants
If --enable-ntpd or --enable-timesyncd, assume --enable-ntp and
choose the appropriate hook file.
Will complain if --enable-ntpd or --enable-timesyncd are both set,
allows override of autodetect.
Autodetect will enable ntp functionality unless explicitly told not
to via --disable-ntp. If --disable-ntp is used, no time hooks
are installed.
Addresses #574