| Message ID | 20240322221327.12204-4-mg@max.gautier.name (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | maintenance: use packaged systemd units | expand |
On Fri, Mar 22, 2024 at 6:13 PM Max Gautier <mg@max.gautier.name> wrote: > The systemctl invocation to enable or disable the git maintenance timers > is needlessly complicated: > - systemctl does not mind at all enabling already enabled units, nor > disabling already disabled units. Has systemctl behavior changed... > Use only one systemctl invocation per `git maintenance start/stop`. > Transparently pass its status and output. > Add the --force option to override conflicting symlinks to previous > instances of our units files which lived in $XDG_CONFIG_HOME. > > Signed-off-by: Max Gautier <mg@max.gautier.name> > --- > diff --git a/builtin/gc.c b/builtin/gc.c > @@ -2303,70 +2303,35 @@ static int is_systemd_timer_available(void) > - * Disabling the systemd unit while it is already disabled makes > - * systemctl print an error. > - * Let's ignore it since it means we already are in the expected state: > - * the unit is disabled. ... since this and... > - * Disabling an already disabled systemd unit makes > - * systemctl fail. > - * Let's ignore this failure. ... this were written? If so, do we still need to worry about older systems in which systemctl prints errors and/or fails outright?
On Fri, Mar 22, 2024 at 07:09:18PM -0400, Eric Sunshine wrote: > On Fri, Mar 22, 2024 at 6:13 PM Max Gautier <mg@max.gautier.name> wrote: > > The systemctl invocation to enable or disable the git maintenance timers > > is needlessly complicated: > > - systemctl does not mind at all enabling already enabled units, nor > > disabling already disabled units. > > Has systemctl behavior changed... > > > Use only one systemctl invocation per `git maintenance start/stop`. > > Transparently pass its status and output. > > Add the --force option to override conflicting symlinks to previous > > instances of our units files which lived in $XDG_CONFIG_HOME. > > > > Signed-off-by: Max Gautier <mg@max.gautier.name> > > --- > > diff --git a/builtin/gc.c b/builtin/gc.c > > @@ -2303,70 +2303,35 @@ static int is_systemd_timer_available(void) > > - * Disabling the systemd unit while it is already disabled makes > > - * systemctl print an error. > > - * Let's ignore it since it means we already are in the expected state: > > - * the unit is disabled. > > ... since this and... > > > - * Disabling an already disabled systemd unit makes > > - * systemctl fail. > > - * Let's ignore this failure. > > ... this were written? > > If so, do we still need to worry about older systems in which > systemctl prints errors and/or fails outright? I tried the following on systemd source $ git log -L :do_unit_file_disable:src/shared/install.c $ git log -L :unit_file_disable:src/shared/install.c` $ git log --grep 'already disabled' $ git log --grep 're disable' $ git log --grep 'disabled unit' $ git log --grep 'systemctl disable' That yields nothing indicating a change (in the commit messages, I didn't look at the code in detail) when disabling already disabled units, and testing on systemd 255 disable already disabled units (normal and templated, same thing) without complaining or an error status. Nothing relevant I could find on github in the issues or PRs either. systemctl does error out or print warnings in other conditions, like missing [Install] section, but that's not something we should ignore. Philip Wood asked that question here in the original thread: > What is the exit code of systemctl if a unit is already enabled and we > try to enbale it again (and the same for disabling a disabled unit)? https://lore.kernel.org/git/3fd17223-8667-24be-2e65-f1970d411bdf@gmail.com/ But I can't find a follow-up email from Lénaïc, and searching for "disabled" in the whole thread only yield the comment I'm removing. Not sure if Lénaïc still follows the list, but maybe he can comment on that ? To me it looks like it was not necessary to begin with, but I might have just missed the discussion about it. Adding Phillip to the discussion as well.
diff --git a/builtin/gc.c b/builtin/gc.c index 199c8e6240..aaee91451a 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -2303,70 +2303,35 @@ static int is_systemd_timer_available(void) return real_is_systemd_timer_available(); } -static int systemd_timer_enable_unit(int enable, - enum schedule_priority schedule) +static int systemd_set_units_state(int enable) { const char *cmd = "systemctl"; struct child_process child = CHILD_PROCESS_INIT; - const char *frequency = get_frequency(schedule); - - /* - * Disabling the systemd unit while it is already disabled makes - * systemctl print an error. - * Let's ignore it since it means we already are in the expected state: - * the unit is disabled. - * - * On the other hand, enabling a systemd unit which is already enabled - * produces no error. - */ - if (!enable) - child.no_stderr = 1; get_schedule_cmd(&cmd, NULL); strvec_split(&child.args, cmd); - strvec_pushl(&child.args, "--user", enable ? "enable" : "disable", - "--now", NULL); - strvec_pushf(&child.args, "git-maintenance@%s.timer", frequency); + + strvec_pushl(&child.args, "--user", "--force", "--now", + enable ? "enable" : "disable", + "git-maintenance@hourly.timer", + "git-maintenance@daily.timer", + "git-maintenance@weekly.timer", NULL); + /* + * --force override existing conflicting symlinks + * We need it because the units have changed location (~/.config -> + * /usr/lib) + */ if (start_command(&child)) return error(_("failed to start systemctl")); if (finish_command(&child)) - /* - * Disabling an already disabled systemd unit makes - * systemctl fail. - * Let's ignore this failure. - * - * Enabling an enabled systemd unit doesn't fail. - */ - if (enable) - return error(_("failed to run systemctl")); + return error(_("failed to run systemctl")); return 0; } -static int systemd_timer_disable_units(void) -{ - return systemd_timer_enable_unit(0, SCHEDULE_HOURLY) || - systemd_timer_enable_unit(0, SCHEDULE_DAILY) || - systemd_timer_enable_unit(0, SCHEDULE_WEEKLY); -} - -static int systemd_timer_setup_units(void) -{ - - int ret = systemd_timer_enable_unit(1, SCHEDULE_HOURLY) || - systemd_timer_enable_unit(1, SCHEDULE_DAILY) || - systemd_timer_enable_unit(1, SCHEDULE_WEEKLY); - if (ret) - systemd_timer_disable_units(); - return ret; -} - static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED) { - if (run_maintenance) - return systemd_timer_setup_units(); - else - return systemd_timer_disable_units(); + return systemd_set_units_state(run_maintenance); } enum scheduler {
The systemctl invocation to enable or disable the git maintenance timers is needlessly complicated: - systemctl does not mind at all enabling already enabled units, nor disabling already disabled units. - it can act on several units at once instead of only one. Use only one systemctl invocation per `git maintenance start/stop`. Transparently pass its status and output. Add the --force option to override conflicting symlinks to previous instances of our units files which lived in $XDG_CONFIG_HOME. Signed-off-by: Max Gautier <mg@max.gautier.name> --- builtin/gc.c | 63 ++++++++++++---------------------------------------- 1 file changed, 14 insertions(+), 49 deletions(-)