# [RFC PATCH 0/5] maintenance: use packaged systemd units 22 messages from 2024-03-18 to 2024-03-21. Participants: Max Gautier, Eric Sunshine, Junio C Hamano, Patrick Steinhardt. Thread: https://gitlist.dev/t/61145 ## Max Gautier, 2024-03-18 15:31 Subject: [RFC PATCH 0/5] maintenance: use packaged systemd units Message-ID: <20240318153257.27451-1-mg@max.gautier.name> URL: https://gitlist.dev/e/20240318153257.27451-1-mg%40max.gautier.name ``` Please ignore the previous series, I messed up the cover letter and Cc with git-send-email somehow... ----- Hello, This is a proposal to distribute the timers of the systemd scheduler of git-maintenance directly, rather than inlining them into the code and writing them on demand. IMO, this is better than using the user $XDG_CONFIG_HOME, and allows that user to override them if they so wish. We also move away from using the random minute, and instead rely on systemd features to achieve the same goal (see patch 2). This allows us to go back to using unit templating for the timers. (Not that even if we really more specific OnCalendar= settings for each timer, we should still do it that way, but instead distribute override alongside the template, i.e /usr/lib/systemd-user/git-maintenance@daily.timer.d/override.conf: [Timer] OnCalendar= The cleanup code for the units written in $XDG_CONFIG_HOME is adapted, and takes care of not removing legitimate user overrides, by checking the file start. Patch 5 removes the cleanup code, it should not be applied right away, but once enough time has passed and we can be reasonably sure that no one still has the old units laying around. Testing: The simplest way to simulate having the units in /usr/lib is probably to copy them in /etc/systemd/user. Unresolved (reason why it's still an RFC): - Should I implement conditional install of systemd units (if systemd is available) ? I've avoided digging too deep in the Makefile, but that's doable, I think. --- Documentation/git-maintenance.txt | 33 ++-- Makefile | 4 + builtin/gc.c | 279 ++-------------------------------- systemd/user/git-maintenance@.service | 17 +++ systemd/user/git-maintenance@.timer | 12 ++ 5 files changed, 63 insertions(+), 282 deletions(-) ``` ## Max Gautier, 2024-03-18 15:31 Subject: [RFC PATCH 1/5] maintenance: package systemd units Message-ID: <20240318153257.27451-2-mg@max.gautier.name> URL: https://gitlist.dev/e/20240318153257.27451-2-mg%40max.gautier.name In-Reply-To: <20240318153257.27451-1-mg@max.gautier.name> ``` Signed-off-by: Max Gautier --- Makefile | 4 ++++ systemd/user/git-maintenance@.service | 16 ++++++++++++++++ systemd/user/git-maintenance@.timer | 9 +++++++++ 3 files changed, 29 insertions(+) create mode 100644 systemd/user/git-maintenance@.service create mode 100644 systemd/user/git-maintenance@.timer diff --git a/Makefile b/Makefile index 4e255c81f2..276b4373c6 100644 --- a/Makefile +++ b/Makefile @@ -619,6 +619,7 @@ htmldir = $(prefix)/share/doc/git-doc ETC_GITCONFIG = $(sysconfdir)/gitconfig ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes lib = lib +libdir = $(prefix)/lib # DESTDIR = pathsep = : @@ -1328,6 +1329,8 @@ BUILTIN_OBJS += builtin/verify-tag.o BUILTIN_OBJS += builtin/worktree.o BUILTIN_OBJS += builtin/write-tree.o +SYSTEMD_USER_UNITS := $(wildcard systemd/user/*) + # THIRD_PARTY_SOURCES is a list of patterns compatible with the # $(filter) and $(filter-out) family of functions. They specify source # files which are taken from some third-party source where we want to be @@ -3469,6 +3472,7 @@ install: all $(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' $(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)' $(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)' + $(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(libdir)/systemd/user' $(SYSTEMD_USER_UNITS) ifdef MSVC # We DO NOT install the individual foo.o.pdb files because they diff --git a/systemd/user/git-maintenance@.service b/systemd/user/git-maintenance@.service new file mode 100644 index 0000000000..87ac0c86e6 --- /dev/null +++ b/systemd/user/git-maintenance@.service @@ -0,0 +1,16 @@ +[Unit] +Description=Optimize Git repositories data + +[Service] +Type=oneshot +ExecStart=git for-each-repo --config=maintenance.repo \ + maintenance run --schedule=%i +LockPersonality=yes +MemoryDenyWriteExecute=yes +NoNewPrivileges=yes +RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 AF_VSOCK +RestrictNamespaces=yes +RestrictRealtime=yes +RestrictSUIDSGID=yes +SystemCallArchitectures=native +SystemCallFilter=@system-service diff --git a/systemd/user/git-maintenance@.timer b/systemd/user/git-maintenance@.timer new file mode 100644 index 0000000000..40fbc77a62 --- /dev/null +++ b/systemd/user/git-maintenance@.timer @@ -0,0 +1,9 @@ +[Unit] +Description=Optimize Git repositories data + +[Timer] +OnCalendar=%i +Persistent=true + +[Install] +WantedBy=timers.target -- 2.44.0 ``` ## Max Gautier, 2024-03-18 15:31 Subject: [RFC PATCH 2/5] maintenance: add fixed random delay to systemd timers Message-ID: <20240318153257.27451-3-mg@max.gautier.name> URL: https://gitlist.dev/e/20240318153257.27451-3-mg%40max.gautier.name In-Reply-To: <20240318153257.27451-1-mg@max.gautier.name> ``` Ensures that: - git maintenance timers have a fixed time interval between execution. - the three timers are not executed at the same time. This is intended to implement an alternative to the two followings commits: c97ec0378b (maintenance: fix systemd schedule overlaps, 2023-08-10) daa787010c (maintenance: use random minute in systemd scheduler, 2023-08-10) Instead of manually adding a specific minute (which is reset on each invocation of `git maintenance start`), we use systemd timers RandomizedDelaySec and FixedRandomDelay functionalities. From man systemd.timer: >FixedRandomDelay= > Takes a boolean argument. When enabled, the randomized offset > specified by RandomizedDelaySec= is reused for all firings of the > same timer. For a given timer unit, **the offset depends on the > machine ID, user identifier and timer name**, which means that it is > stable between restarts of the manager. This effectively creates a > fixed offset for an individual timer, reducing the jitter in > firings of this timer, while still avoiding firing at the same time > as other similarly configured timers. -> which is exactly the use case for git-maintenance timers. Signed-off-by: Max Gautier --- systemd/user/git-maintenance@.service | 1 + systemd/user/git-maintenance@.timer | 3 +++ 2 files changed, 4 insertions(+) diff --git a/systemd/user/git-maintenance@.service b/systemd/user/git-maintenance@.service index 87ac0c86e6..f949e1a217 100644 --- a/systemd/user/git-maintenance@.service +++ b/systemd/user/git-maintenance@.service @@ -1,5 +1,6 @@ [Unit] Description=Optimize Git repositories data +Documentation=man:git-maintenance(1) [Service] Type=oneshot diff --git a/systemd/user/git-maintenance@.timer b/systemd/user/git-maintenance@.timer index 40fbc77a62..667c5998ba 100644 --- a/systemd/user/git-maintenance@.timer +++ b/systemd/user/git-maintenance@.timer @@ -1,9 +1,12 @@ [Unit] Description=Optimize Git repositories data +Documentation=man:git-maintenance(1) [Timer] OnCalendar=%i Persistent=true +RandomizedDelaySec=1800 +FixedRandomDelay=true [Install] WantedBy=timers.target -- 2.44.0 ``` ## Max Gautier, 2024-03-18 15:31 Subject: [RFC PATCH 4/5] maintenance: update systemd scheduler docs Message-ID: <20240318153257.27451-5-mg@max.gautier.name> URL: https://gitlist.dev/e/20240318153257.27451-5-mg%40max.gautier.name In-Reply-To: <20240318153257.27451-1-mg@max.gautier.name> ``` Signed-off-by: Max Gautier --- Documentation/git-maintenance.txt | 33 +++++++++++++++---------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt index 51d0f7e94b..6511c3f3f1 100644 --- a/Documentation/git-maintenance.txt +++ b/Documentation/git-maintenance.txt @@ -304,10 +304,9 @@ distributions, systemd timers are superseding it. If user systemd timers are available, they will be used as a replacement of `cron`. -In this case, `git maintenance start` will create user systemd timer units -and start the timers. The current list of user-scheduled tasks can be found -by running `systemctl --user list-timers`. The timers written by `git -maintenance start` are similar to this: +In this case, `git maintenance start` will enable user systemd timer units +and start them. The current list of user-scheduled tasks can be found by +running `systemctl --user list-timers`. These timers are similar to this: ----------------------------------------------------------------------- $ systemctl --user list-timers @@ -317,25 +316,25 @@ Fri 2021-04-30 00:00:00 CEST 5h 42min left Thu 2021-04-29 00:00:11 CEST 18h ago Mon 2021-05-03 00:00:00 CEST 3 days left Mon 2021-04-26 00:00:11 CEST 3 days ago git-maintenance@weekly.timer git-maintenance@weekly.service ----------------------------------------------------------------------- -One timer is registered for each `--schedule=` option. +One timer instance is enabled for each `--schedule=` option. -The definition of the systemd units can be inspected in the following files: +The definition of the systemd units can be inspected this way: ----------------------------------------------------------------------- -~/.config/systemd/user/git-maintenance@.timer -~/.config/systemd/user/git-maintenance@.service -~/.config/systemd/user/timers.target.wants/git-maintenance@hourly.timer -~/.config/systemd/user/timers.target.wants/git-maintenance@daily.timer -~/.config/systemd/user/timers.target.wants/git-maintenance@weekly.timer +$ systemctl cat --user git-maintenance@.timer +$ systemctl cat --user git-maintenance@.service ----------------------------------------------------------------------- -`git maintenance start` will overwrite these files and start the timer -again with `systemctl --user`, so any customization should be done by -creating a drop-in file, i.e. a `.conf` suffixed file in the -`~/.config/systemd/user/git-maintenance@.service.d` directory. +Customization of the timer or service can be performed with the usual systemd +tooling: +----------------------------------------------------------------------- +$ systemctl edit --user git-maintenance@.timer # all the timers +$ systemctl edit --user git-maintenance@hourly.timer # the hourly timer +$ systemctl edit --user git-maintenance@.service # all the services +$ systemctl edit --user git-maintenance@hourly.service # the hourly run +----------------------------------------------------------------------- -`git maintenance stop` will stop the user systemd timers and delete -the above mentioned files. +`git maintenance stop` will disable and stop the user systemd timers. For more details, see `systemd.timer(5)`. -- 2.44.0 ``` ## Max Gautier, 2024-03-18 15:31 Subject: [RFC PATCH 3/5] maintenance: use packaged systemd units Message-ID: <20240318153257.27451-4-mg@max.gautier.name> URL: https://gitlist.dev/e/20240318153257.27451-4-mg%40max.gautier.name In-Reply-To: <20240318153257.27451-1-mg@max.gautier.name> ``` - Remove the code writing the units - Simplify calling systemctl - systemctl does not error out when disabling units which aren't enabled, nor when enabling already enabled units - call systemctl only once, with all the units. - Add clean-up code for leftover units in $XDG_CONFIG_HOME created by previous versions of git, taking care of preserving actual user override (by checking the start of the file). Signed-off-by: Max Gautier --- builtin/gc.c | 293 ++++++++------------------------------------------- 1 file changed, 45 insertions(+), 248 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index cb80ced6cb..981db8e297 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -2308,276 +2308,73 @@ static char *xdg_config_home_systemd(const char *filename) return xdg_config_home_for("systemd/user", filename); } -#define SYSTEMD_UNIT_FORMAT "git-maintenance@%s.%s" - -static int systemd_timer_delete_timer_file(enum schedule_priority priority) -{ - int ret = 0; - const char *frequency = get_frequency(priority); - char *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "timer"); - char *filename = xdg_config_home_systemd(local_timer_name); - - if (unlink(filename) && !is_missing_file_error(errno)) - ret = error_errno(_("failed to delete '%s'"), filename); - - free(filename); - free(local_timer_name); - return ret; -} - -static int systemd_timer_delete_service_template(void) -{ - int ret = 0; - char *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "service"); - char *filename = xdg_config_home_systemd(local_service_name); - if (unlink(filename) && !is_missing_file_error(errno)) - ret = error_errno(_("failed to delete '%s'"), filename); - - free(filename); - free(local_service_name); - return ret; -} - -/* - * Write the schedule information into a git-maintenance@.timer - * file using a custom minute. This timer file cannot use the templating - * system, so we generate a specific file for each. - */ -static int systemd_timer_write_timer_file(enum schedule_priority schedule, - int minute) -{ - int res = -1; - char *filename; - FILE *file; - const char *unit; - char *schedule_pattern = NULL; - const char *frequency = get_frequency(schedule); - char *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "timer"); - - filename = xdg_config_home_systemd(local_timer_name); - - if (safe_create_leading_directories(filename)) { - error(_("failed to create directories for '%s'"), filename); - goto error; - } - file = fopen_or_warn(filename, "w"); - if (!file) - goto error; - - switch (schedule) { - case SCHEDULE_HOURLY: - schedule_pattern = xstrfmt("*-*-* 1..23:%02d:00", minute); - break; - - case SCHEDULE_DAILY: - schedule_pattern = xstrfmt("Tue..Sun *-*-* 0:%02d:00", minute); - break; - - case SCHEDULE_WEEKLY: - schedule_pattern = xstrfmt("Mon 0:%02d:00", minute); - break; - - default: - BUG("Unhandled schedule_priority"); - } - - unit = "# This file was created and is maintained by Git.\n" - "# Any edits made in this file might be replaced in the future\n" - "# by a Git command.\n" - "\n" - "[Unit]\n" - "Description=Optimize Git repositories data\n" - "\n" - "[Timer]\n" - "OnCalendar=%s\n" - "Persistent=true\n" - "\n" - "[Install]\n" - "WantedBy=timers.target\n"; - if (fprintf(file, unit, schedule_pattern) < 0) { - error(_("failed to write to '%s'"), filename); - fclose(file); - goto error; - } - if (fclose(file) == EOF) { - error_errno(_("failed to flush '%s'"), filename); - goto error; - } - - res = 0; - -error: - free(schedule_pattern); - free(local_timer_name); - free(filename); - return res; -} - -/* - * No matter the schedule, we use the same service and can make use of the - * templating system. When installing git-maintenance@.timer, - * systemd will notice that git-maintenance@.service exists as a template - * and will use this file and insert the into the template at - * the position of "%i". - */ -static int systemd_timer_write_service_template(const char *exec_path) -{ - int res = -1; - char *filename; - FILE *file; - const char *unit; - char *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "service"); - - filename = xdg_config_home_systemd(local_service_name); - if (safe_create_leading_directories(filename)) { - error(_("failed to create directories for '%s'"), filename); - goto error; - } - file = fopen_or_warn(filename, "w"); - if (!file) - goto error; - - unit = "# This file was created and is maintained by Git.\n" - "# Any edits made in this file might be replaced in the future\n" - "# by a Git command.\n" - "\n" - "[Unit]\n" - "Description=Optimize Git repositories data\n" - "\n" - "[Service]\n" - "Type=oneshot\n" - "ExecStart=\"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n" - "LockPersonality=yes\n" - "MemoryDenyWriteExecute=yes\n" - "NoNewPrivileges=yes\n" - "RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 AF_VSOCK\n" - "RestrictNamespaces=yes\n" - "RestrictRealtime=yes\n" - "RestrictSUIDSGID=yes\n" - "SystemCallArchitectures=native\n" - "SystemCallFilter=@system-service\n"; - if (fprintf(file, unit, exec_path, exec_path) < 0) { - error(_("failed to write to '%s'"), filename); - fclose(file); - goto error; - } - if (fclose(file) == EOF) { - error_errno(_("failed to flush '%s'"), filename); - goto error; - } - - res = 0; - -error: - free(local_service_name); - free(filename); - return res; -} - -static int systemd_timer_enable_unit(int enable, - enum schedule_priority schedule, - int minute) +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; - else if (systemd_timer_write_timer_file(schedule, minute)) - return -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, SYSTEMD_UNIT_FORMAT, frequency, "timer"); + + 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; } -/* - * A previous version of Git wrote the timer units as template files. - * Clean these up, if they exist. - */ -static void systemd_timer_delete_stale_timer_templates(void) +static void systemd_delete_user_unit(char const *unit) { - char *timer_template_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "timer"); - char *filename = xdg_config_home_systemd(timer_template_name); + char const file_start_stale[] = "# This file was created and is" + " maintained by Git."; + char file_start_user[sizeof(file_start_stale)] = {'\0'}; + + char *filename = xdg_config_home_systemd(unit); + int handle = open(filename, O_RDONLY); - if (unlink(filename) && !is_missing_file_error(errno)) + /* + ** Check this is actually our file and we're not removing a legitimate + ** user override. + */ + if (handle == -1 && !is_missing_file_error(errno)) warning(_("failed to delete '%s'"), filename); + else { + read(handle, file_start_user, sizeof(file_start_stale) - 1); + close(handle); + if (strcmp(file_start_stale, file_start_user) == 0) { + if (unlink(filename) == 0) + warning(_("deleted stale unit file '%s'"), filename); + else if (!is_missing_file_error(errno)) + warning(_("failed to delete '%s'"), filename); + } + } free(filename); - free(timer_template_name); -} - -static int systemd_timer_delete_unit_files(void) -{ - systemd_timer_delete_stale_timer_templates(); - - /* Purposefully not short-circuited to make sure all are called. */ - return systemd_timer_delete_timer_file(SCHEDULE_HOURLY) | - systemd_timer_delete_timer_file(SCHEDULE_DAILY) | - systemd_timer_delete_timer_file(SCHEDULE_WEEKLY) | - systemd_timer_delete_service_template(); -} - -static int systemd_timer_delete_units(void) -{ - int minute = get_random_minute(); - /* Purposefully not short-circuited to make sure all are called. */ - return systemd_timer_enable_unit(0, SCHEDULE_HOURLY, minute) | - systemd_timer_enable_unit(0, SCHEDULE_DAILY, minute) | - systemd_timer_enable_unit(0, SCHEDULE_WEEKLY, minute) | - systemd_timer_delete_unit_files(); -} - -static int systemd_timer_setup_units(void) -{ - int minute = get_random_minute(); - const char *exec_path = git_exec_path(); - - int ret = systemd_timer_write_service_template(exec_path) || - systemd_timer_enable_unit(1, SCHEDULE_HOURLY, minute) || - systemd_timer_enable_unit(1, SCHEDULE_DAILY, minute) || - systemd_timer_enable_unit(1, SCHEDULE_WEEKLY, minute); - - if (ret) - systemd_timer_delete_units(); - else - systemd_timer_delete_stale_timer_templates(); - - 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_delete_units(); + /* + * A previous version of Git wrote the units in the user configuration + * directory. Clean these up, if they exist. + */ + systemd_delete_user_unit("git-maintenance@hourly.timer"); + systemd_delete_user_unit("git-maintenance@daily.timer"); + systemd_delete_user_unit("git-maintenance@weekly.timer"); + systemd_delete_user_unit("git-maintenance@.timer"); + systemd_delete_user_unit("git-maintenance@.service"); + return systemd_set_units_state(run_maintenance); } enum scheduler { -- 2.44.0 ``` ## Max Gautier, 2024-03-18 15:31 Subject: [RFC PATCH 5/5] DON'T APPLY YET: maintenance: remove cleanup code Message-ID: <20240318153257.27451-6-mg@max.gautier.name> URL: https://gitlist.dev/e/20240318153257.27451-6-mg%40max.gautier.name In-Reply-To: <20240318153257.27451-1-mg@max.gautier.name> ``` This removes code to remove old git-maintenance systemd timer and service user units which were written in $XDG_CONFIG_HOME by git previous versions. Signed-off-by: Max Gautier --- builtin/gc.c | 54 +++------------------------------------------------- 1 file changed, 3 insertions(+), 51 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index 981db8e297..6ac184eaf5 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -2303,12 +2303,7 @@ static int is_systemd_timer_available(void) return real_is_systemd_timer_available(); } -static char *xdg_config_home_systemd(const char *filename) -{ - return xdg_config_home_for("systemd/user", filename); -} - -static int systemd_set_units_state(int enable) +static int systemd_set_units_state(int run_maintenance, int fd UNUSED) { const char *cmd = "systemctl"; struct child_process child = CHILD_PROCESS_INIT; @@ -2317,7 +2312,7 @@ static int systemd_set_units_state(int enable) strvec_split(&child.args, cmd); strvec_pushl(&child.args, "--user", "--force", "--now", - enable ? "enable" : "disable", + run_maintenance ? "enable" : "disable", "git-maintenance@hourly.timer", "git-maintenance@daily.timer", "git-maintenance@weekly.timer", NULL); @@ -2334,49 +2329,6 @@ static int systemd_set_units_state(int enable) return 0; } -static void systemd_delete_user_unit(char const *unit) -{ - char const file_start_stale[] = "# This file was created and is" - " maintained by Git."; - char file_start_user[sizeof(file_start_stale)] = {'\0'}; - - char *filename = xdg_config_home_systemd(unit); - int handle = open(filename, O_RDONLY); - - /* - ** Check this is actually our file and we're not removing a legitimate - ** user override. - */ - if (handle == -1 && !is_missing_file_error(errno)) - warning(_("failed to delete '%s'"), filename); - else { - read(handle, file_start_user, sizeof(file_start_stale) - 1); - close(handle); - if (strcmp(file_start_stale, file_start_user) == 0) { - if (unlink(filename) == 0) - warning(_("deleted stale unit file '%s'"), filename); - else if (!is_missing_file_error(errno)) - warning(_("failed to delete '%s'"), filename); - } - } - - free(filename); -} - -static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED) -{ - /* - * A previous version of Git wrote the units in the user configuration - * directory. Clean these up, if they exist. - */ - systemd_delete_user_unit("git-maintenance@hourly.timer"); - systemd_delete_user_unit("git-maintenance@daily.timer"); - systemd_delete_user_unit("git-maintenance@weekly.timer"); - systemd_delete_user_unit("git-maintenance@.timer"); - systemd_delete_user_unit("git-maintenance@.service"); - return systemd_set_units_state(run_maintenance); -} - enum scheduler { SCHEDULER_INVALID = -1, SCHEDULER_AUTO, @@ -2399,7 +2351,7 @@ static const struct { [SCHEDULER_SYSTEMD] = { .name = "systemctl", .is_available = is_systemd_timer_available, - .update_schedule = systemd_timer_update_schedule, + .update_schedule = systemd_set_units_state, }, [SCHEDULER_LAUNCHCTL] = { .name = "launchctl", -- 2.44.0 ``` ## Max Gautier, 2024-03-19 12:09 Subject: Re: [RFC PATCH 3/5] maintenance: use packaged systemd units Message-ID: URL: https://gitlist.dev/e/ZfmAfIErHRZVbd49%40framework In-Reply-To: <20240318153257.27451-4-mg@max.gautier.name> ``` I'm working on updating the test in t7900-maintenance.sh, but I might be missing something here: >test_expect_success 'start and stop Linux/systemd maintenance' ' > write_script print-args <<-\EOF && > printf "%s\n" "$*" >>args > EOF > > XDG_CONFIG_HOME="$PWD" && > export XDG_CONFIG_HOME && > rm -f args && > GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args" git maintenance start --scheduler=systemd-timer && Do I understand correctly that this means we're not actually running systemctl here, just printing the arguments to our file ? > # start registers the repo > git config --get --global --fixed-value maintenance.repo "$(pwd)" && > > for schedule in hourly daily weekly > do > test_path_is_file "systemd/user/git-maintenance@$schedule.timer" || return 1 > done && > test_path_is_file "systemd/user/git-maintenance@.service" && > > test_systemd_analyze_verify "systemd/user/git-maintenance@hourly.service" && > test_systemd_analyze_verify "systemd/user/git-maintenance@daily.service" && > test_systemd_analyze_verify "systemd/user/git-maintenance@weekly.service" && > > printf -- "--user enable --now git-maintenance@%s.timer\n" hourly daily weekly >expect && > test_cmp expect args && > > rm -f args && > GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args" git maintenance stop && > > # stop does not unregister the repo > git config --get --global --fixed-value maintenance.repo "$(pwd)" && > > for schedule in hourly daily weekly > do > test_path_is_missing "systemd/user/git-maintenance@$schedule.timer" || return 1 > done && > test_path_is_missing "systemd/user/git-maintenance@.service" && > > printf -- "--user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect && > test_cmp expect args The rest of the systemd tests only check that the service file are in XDG_CONFIG_HOME, which should not be the case anymore. However, the test does not actually check we have enabled and started the timers as it is , right ? Should I add that ? I'm not sure how, because it does not seem like the tests run in a isolated env, so it would mess with the systemd user manager of the developper running the tests... Regarding systemd-analyze verify, do the tests have access to the source directory in a special way, or is using '../..' enough ? Thanks -- Max Gautier ``` ## Eric Sunshine, 2024-03-19 17:17 Subject: Re: [RFC PATCH 3/5] maintenance: use packaged systemd units Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSWLoRdTgrrU2SBswnKr82L_BPCKtaP6atMyZVDAU%3Dhpw%40mail.gmail.com In-Reply-To: ``` On Tue, Mar 19, 2024 at 8:10 AM Max Gautier wrote: > I'm working on updating the test in t7900-maintenance.sh, but I might be > missing something here: > > >test_expect_success 'start and stop Linux/systemd maintenance' ' > > write_script print-args <<-\EOF && > > printf "%s\n" "$*" >>args > > EOF > > > > XDG_CONFIG_HOME="$PWD" && > > export XDG_CONFIG_HOME && > > rm -f args && > > GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args" git maintenance start --scheduler=systemd-timer && > > Do I understand correctly that this means we're not actually running > systemctl here, just printing the arguments to our file ? That's correct. The purpose of GIT_TEST_MAINT_SCHEDULER is twofold. The primary purpose is to test as much as possible without actually mucking with the user's real scheduler-related configuration (whether it be systemd, cron, launchctl, etc.). This means that we want to verify that the expected files are created or removed by git-maintenance, that they are well-formed, and that git-maintenance is invoking the correct platform-specific scheduler-related command with correct arguments (without actually invoking that command and messing up the user's personal configuration). The secondary purpose is to allow these otherwise platform-specific tests to run on any platform. This is possible since, as noted above, we're not actually running the platform-specific scheduler-related command, but instead only capturing the command and arguments that would have been applied had git-maintenace been run "for real" outside of the test framework. > > for schedule in hourly daily weekly > > do > > test_path_is_missing "systemd/user/git-maintenance@$schedule.timer" || return 1 > > done && > > test_path_is_missing "systemd/user/git-maintenance@.service" && > > > > printf -- "--user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect && > > test_cmp expect args > > The rest of the systemd tests only check that the service file are in > XDG_CONFIG_HOME, which should not be the case anymore. > > However, the test does not actually check we have enabled and started > the timers as it is , right ? Correct. As noted above, we don't want to muck with the user's real configuration, and we certainly don't want the system-specific scheduler to actually kick off some command we're testing in the user's account while the test script is running. > Should I add that ? I'm not sure how, because it does not seem like the > tests run in a isolated env, so it would mess with the systemd user > manager of the developper running the tests... No you don't want to add that since, as you state and as stated above, it would muck with the user's own configuration which would be undesirable. > Regarding systemd-analyze verify, do the tests have access to the source > directory in a special way, or is using '../..' enough ? You can't assume that the source directory is available at "../.." since the --root option (see t/README) allows the root of the tests working-tree to reside outside of the project directory. You may be able to use "$TEST_DIRECTORY/.." to reference files in the source tree, though the documentation in t/test-lib.sh doesn't seem to state explicitly that this is intended or supported use. However, a few existing tests (t1021, t3000, t4023) already access files in the source tree in this fashion, so there is precedent. ``` ## Junio C Hamano, 2024-03-19 18:19 Subject: Re: [RFC PATCH 3/5] maintenance: use packaged systemd units Message-ID: URL: https://gitlist.dev/e/xmqq8r2eqe2w.fsf%40gitster.g In-Reply-To: ``` Eric Sunshine writes: > You may be able to use "$TEST_DIRECTORY/.." to reference files in the > source tree, though the documentation in t/test-lib.sh doesn't seem to > state explicitly that this is intended or supported use. However, a > few existing tests (t1021, t3000, t4023) already access files in the > source tree in this fashion, so there is precedent. I think the following from t/test-lib.sh should give us sufficient assurance. # The TEST_DIRECTORY will always be the path to the "t" # directory in the git.git checkout. This is overridden by # e.g. t/lib-subtest.sh, but only because its $(pwd) is # different. Those tests still set "$TEST_DIRECTORY" to the # same path. Everything you said in your review is correct. Thanks. ``` ## Max Gautier, 2024-03-19 19:38 Subject: Re: [RFC PATCH 3/5] maintenance: use packaged systemd units Message-ID: URL: https://gitlist.dev/e/BA7F27C1-EFE2-47CD-B892-A3F4262394CB%40max.gautier.name In-Reply-To: ``` Le 19 mars 2024 18:17:27 GMT+01:00, Eric Sunshine a écrit : >On Tue, Mar 19, 2024 at 8:10 AM Max Gautier wrote: >> I'm working on updating the test in t7900-maintenance.sh, but I might be >> missing something here: >> >> >test_expect_success 'start and stop Linux/systemd maintenance' ' >> > write_script print-args <<-\EOF && >> > printf "%s\n" "$*" >>args >> > EOF >> > >> > XDG_CONFIG_HOME="$PWD" && >> > export XDG_CONFIG_HOME && >> > rm -f args && >> > GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args" git maintenance start --scheduler=systemd-timer && >> >> Do I understand correctly that this means we're not actually running >> systemctl here, just printing the arguments to our file ? > >That's correct. The purpose of GIT_TEST_MAINT_SCHEDULER is twofold. > >The primary purpose is to test as much as possible without actually >mucking with the user's real scheduler-related configuration (whether >it be systemd, cron, launchctl, etc.). This means that we want to >verify that the expected files are created or removed by >git-maintenance, that they are well-formed, and that git-maintenance >is invoking the correct platform-specific scheduler-related command >with correct arguments (without actually invoking that command and >messing up the user's personal configuration). > >The secondary purpose is to allow these otherwise platform-specific >tests to run on any platform. This is possible since, as noted above, >we're not actually running the platform-specific scheduler-related >command, but instead only capturing the command and arguments that >would have been applied had git-maintenace been run "for real" outside >of the test framework. Ok thanks, now I see why it's done this way. > >> > for schedule in hourly daily weekly >> > do >> > test_path_is_missing "systemd/user/git-maintenance@$schedule.timer" || return 1 >> > done && >> > test_path_is_missing "systemd/user/git-maintenance@.service" && >> > >> > printf -- "--user disable --now git-maintenance@%s.timer\n" hourly daily weekly >expect && >> > test_cmp expect args >> >> The rest of the systemd tests only check that the service file are in >> XDG_CONFIG_HOME, which should not be the case anymore. >> >> However, the test does not actually check we have enabled and started >> the timers as it is , right ? > >Correct. As noted above, we don't want to muck with the user's real >configuration, and we certainly don't want the system-specific >scheduler to actually kick off some command we're testing in the >user's account while the test script is running. > >> Should I add that ? I'm not sure how, because it does not seem like the >> tests run in a isolated env, so it would mess with the systemd user >> manager of the developper running the tests... > >No you don't want to add that since, as you state and as stated above, >it would muck with the user's own configuration which would be >undesirable. > That makes things easier then :) >> Regarding systemd-analyze verify, do the tests have access to the source >> directory in a special way, or is using '../..' enough ? > >You can't assume that the source directory is available at "../.." >since the --root option (see t/README) allows the root of the tests >working-tree to reside outside of the project directory. > >You may be able to use "$TEST_DIRECTORY/.." to reference files in the >source tree, though the documentation in t/test-lib.sh doesn't seem to >state explicitly that this is intended or supported use. However, a >few existing tests (t1021, t3000, t4023) already access files in the >source tree in this fashion, so there is precedent. I'll look into those. Thanks for all the info ! -- Max Gautier ``` ## Patrick Steinhardt, 2024-03-21 12:37 Subject: Re: [RFC PATCH 1/5] maintenance: package systemd units Message-ID: URL: https://gitlist.dev/e/ZfwqCv889UdI0mU6%40tanuki In-Reply-To: <20240318153257.27451-2-mg@max.gautier.name> ``` On Mon, Mar 18, 2024 at 04:31:15PM +0100, Max Gautier wrote: It would be great to document _why_ we want to package the systemd units alongside with Git. > Signed-off-by: Max Gautier > --- > Makefile | 4 ++++ > systemd/user/git-maintenance@.service | 16 ++++++++++++++++ > systemd/user/git-maintenance@.timer | 9 +++++++++ > 3 files changed, 29 insertions(+) > create mode 100644 systemd/user/git-maintenance@.service > create mode 100644 systemd/user/git-maintenance@.timer > > diff --git a/Makefile b/Makefile > index 4e255c81f2..276b4373c6 100644 > --- a/Makefile > +++ b/Makefile > @@ -619,6 +619,7 @@ htmldir = $(prefix)/share/doc/git-doc > ETC_GITCONFIG = $(sysconfdir)/gitconfig > ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes > lib = lib > +libdir = $(prefix)/lib > # DESTDIR = > pathsep = : > > @@ -1328,6 +1329,8 @@ BUILTIN_OBJS += builtin/verify-tag.o > BUILTIN_OBJS += builtin/worktree.o > BUILTIN_OBJS += builtin/write-tree.o > > +SYSTEMD_USER_UNITS := $(wildcard systemd/user/*) > + > # THIRD_PARTY_SOURCES is a list of patterns compatible with the > # $(filter) and $(filter-out) family of functions. They specify source > # files which are taken from some third-party source where we want to be > @@ -3469,6 +3472,7 @@ install: all > $(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' > $(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)' > $(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)' > + $(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(libdir)/systemd/user' $(SYSTEMD_USER_UNITS) I wonder whether we want to unconditionally install those units. Many of the platforms that we support don't even have systemd available, so certainly it wouldn't make any sense to install it on those platforms. Assuming that this is something we want in the first place I thus think that we should at least make this conditional and add some platform specific quirk to "config.mak.uname". > ifdef MSVC > # We DO NOT install the individual foo.o.pdb files because they > diff --git a/systemd/user/git-maintenance@.service b/systemd/user/git-maintenance@.service > new file mode 100644 > index 0000000000..87ac0c86e6 > --- /dev/null > +++ b/systemd/user/git-maintenance@.service > @@ -0,0 +1,16 @@ > +[Unit] > +Description=Optimize Git repositories data > + > +[Service] > +Type=oneshot > +ExecStart=git for-each-repo --config=maintenance.repo \ > + maintenance run --schedule=%i > +LockPersonality=yes > +MemoryDenyWriteExecute=yes > +NoNewPrivileges=yes > +RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 AF_VSOCK > +RestrictNamespaces=yes > +RestrictRealtime=yes > +RestrictSUIDSGID=yes > +SystemCallArchitectures=native > +SystemCallFilter=@system-service Curious, but how did you arrive at these particular restrictions for the unit? Might be something to explain in the commit message, as well. Patrick > diff --git a/systemd/user/git-maintenance@.timer b/systemd/user/git-maintenance@.timer > new file mode 100644 > index 0000000000..40fbc77a62 > --- /dev/null > +++ b/systemd/user/git-maintenance@.timer > @@ -0,0 +1,9 @@ > +[Unit] > +Description=Optimize Git repositories data > + > +[Timer] > +OnCalendar=%i > +Persistent=true > + > +[Install] > +WantedBy=timers.target > -- > 2.44.0 > > ``` ## Patrick Steinhardt, 2024-03-21 12:37 Subject: Re: [RFC PATCH 2/5] maintenance: add fixed random delay to systemd timers Message-ID: URL: https://gitlist.dev/e/ZfwqD7z6S8Olc5xa%40tanuki In-Reply-To: <20240318153257.27451-3-mg@max.gautier.name> ``` On Mon, Mar 18, 2024 at 04:31:16PM +0100, Max Gautier wrote: > Ensures that: > - git maintenance timers have a fixed time interval between execution. > - the three timers are not executed at the same time. Commit messages are typically structured so that you first explain what the problem is that you're trying to solve, and then you explain how you solve it. Your commit message is missing the first part. > This is intended to implement an alternative to the two followings > commits: > c97ec0378b (maintenance: fix systemd schedule overlaps, 2023-08-10) > daa787010c (maintenance: use random minute in systemd scheduler, 2023-08-10) > > Instead of manually adding a specific minute (which is reset on each > invocation of `git maintenance start`), we use systemd timers > RandomizedDelaySec and FixedRandomDelay functionalities. I think it would help to not list commits, but put the commit references in a paragraph. Something in the spirit of "In commit c97ec0378b we already tried to address this issue in such and such a way, but that is quite limiting due to that and that. Similarly, in commit daa787010c..". > From man systemd.timer: > >FixedRandomDelay= > > Takes a boolean argument. When enabled, the randomized offset > > specified by RandomizedDelaySec= is reused for all firings of the > > same timer. For a given timer unit, **the offset depends on the > > machine ID, user identifier and timer name**, which means that it is > > stable between restarts of the manager. This effectively creates a > > fixed offset for an individual timer, reducing the jitter in > > firings of this timer, while still avoiding firing at the same time > > as other similarly configured timers. > > -> which is exactly the use case for git-maintenance timers. > > Signed-off-by: Max Gautier > --- > systemd/user/git-maintenance@.service | 1 + > systemd/user/git-maintenance@.timer | 3 +++ > 2 files changed, 4 insertions(+) > > diff --git a/systemd/user/git-maintenance@.service b/systemd/user/git-maintenance@.service > index 87ac0c86e6..f949e1a217 100644 > --- a/systemd/user/git-maintenance@.service > +++ b/systemd/user/git-maintenance@.service > @@ -1,5 +1,6 @@ > [Unit] > Description=Optimize Git repositories data > +Documentation=man:git-maintenance(1) This change feels unrelated and should likely go into the first commit. > > [Service] > Type=oneshot > diff --git a/systemd/user/git-maintenance@.timer b/systemd/user/git-maintenance@.timer > index 40fbc77a62..667c5998ba 100644 > --- a/systemd/user/git-maintenance@.timer > +++ b/systemd/user/git-maintenance@.timer > @@ -1,9 +1,12 @@ > [Unit] > Description=Optimize Git repositories data > +Documentation=man:git-maintenance(1) Same. Patrick > [Timer] > OnCalendar=%i > Persistent=true > +RandomizedDelaySec=1800 > +FixedRandomDelay=true > > [Install] > WantedBy=timers.target > -- > 2.44.0 > > ``` ## Patrick Steinhardt, 2024-03-21 12:37 Subject: Re: [RFC PATCH 3/5] maintenance: use packaged systemd units Message-ID: URL: https://gitlist.dev/e/ZfwqFU_b3MqLCZNR%40tanuki In-Reply-To: <20240318153257.27451-4-mg@max.gautier.name> ``` On Mon, Mar 18, 2024 at 04:31:17PM +0100, Max Gautier wrote: > - Remove the code writing the units > - Simplify calling systemctl > - systemctl does not error out when disabling units which aren't > enabled, nor when enabling already enabled units > - call systemctl only once, with all the units. > - Add clean-up code for leftover units in $XDG_CONFIG_HOME created by > previous versions of git, taking care of preserving actual user > override (by checking the start of the file). We very much try to avoid bulleted-list-style commit messages. Once you have a list of changes it very often indicates that there are multiple semi-related things going on in the same commit. At that point we prefer to split the commit up into multiple commits so that each of the bullet items can be explained separately. Also, as mentioned before, commit messages should explain what the problem is and how that problem is solved. Patrick > Signed-off-by: Max Gautier > --- > builtin/gc.c | 293 ++++++++------------------------------------------- > 1 file changed, 45 insertions(+), 248 deletions(-) > > diff --git a/builtin/gc.c b/builtin/gc.c > index cb80ced6cb..981db8e297 100644 > --- a/builtin/gc.c > +++ b/builtin/gc.c > @@ -2308,276 +2308,73 @@ static char *xdg_config_home_systemd(const char *filename) > return xdg_config_home_for("systemd/user", filename); > } > > -#define SYSTEMD_UNIT_FORMAT "git-maintenance@%s.%s" > - > -static int systemd_timer_delete_timer_file(enum schedule_priority priority) > -{ > - int ret = 0; > - const char *frequency = get_frequency(priority); > - char *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "timer"); > - char *filename = xdg_config_home_systemd(local_timer_name); > - > - if (unlink(filename) && !is_missing_file_error(errno)) > - ret = error_errno(_("failed to delete '%s'"), filename); > - > - free(filename); > - free(local_timer_name); > - return ret; > -} > - > -static int systemd_timer_delete_service_template(void) > -{ > - int ret = 0; > - char *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "service"); > - char *filename = xdg_config_home_systemd(local_service_name); > - if (unlink(filename) && !is_missing_file_error(errno)) > - ret = error_errno(_("failed to delete '%s'"), filename); > - > - free(filename); > - free(local_service_name); > - return ret; > -} > - > -/* > - * Write the schedule information into a git-maintenance@.timer > - * file using a custom minute. This timer file cannot use the templating > - * system, so we generate a specific file for each. > - */ > -static int systemd_timer_write_timer_file(enum schedule_priority schedule, > - int minute) > -{ > - int res = -1; > - char *filename; > - FILE *file; > - const char *unit; > - char *schedule_pattern = NULL; > - const char *frequency = get_frequency(schedule); > - char *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, "timer"); > - > - filename = xdg_config_home_systemd(local_timer_name); > - > - if (safe_create_leading_directories(filename)) { > - error(_("failed to create directories for '%s'"), filename); > - goto error; > - } > - file = fopen_or_warn(filename, "w"); > - if (!file) > - goto error; > - > - switch (schedule) { > - case SCHEDULE_HOURLY: > - schedule_pattern = xstrfmt("*-*-* 1..23:%02d:00", minute); > - break; > - > - case SCHEDULE_DAILY: > - schedule_pattern = xstrfmt("Tue..Sun *-*-* 0:%02d:00", minute); > - break; > - > - case SCHEDULE_WEEKLY: > - schedule_pattern = xstrfmt("Mon 0:%02d:00", minute); > - break; > - > - default: > - BUG("Unhandled schedule_priority"); > - } > - > - unit = "# This file was created and is maintained by Git.\n" > - "# Any edits made in this file might be replaced in the future\n" > - "# by a Git command.\n" > - "\n" > - "[Unit]\n" > - "Description=Optimize Git repositories data\n" > - "\n" > - "[Timer]\n" > - "OnCalendar=%s\n" > - "Persistent=true\n" > - "\n" > - "[Install]\n" > - "WantedBy=timers.target\n"; > - if (fprintf(file, unit, schedule_pattern) < 0) { > - error(_("failed to write to '%s'"), filename); > - fclose(file); > - goto error; > - } > - if (fclose(file) == EOF) { > - error_errno(_("failed to flush '%s'"), filename); > - goto error; > - } > - > - res = 0; > - > -error: > - free(schedule_pattern); > - free(local_timer_name); > - free(filename); > - return res; > -} > - > -/* > - * No matter the schedule, we use the same service and can make use of the > - * templating system. When installing git-maintenance@.timer, > - * systemd will notice that git-maintenance@.service exists as a template > - * and will use this file and insert the into the template at > - * the position of "%i". > - */ > -static int systemd_timer_write_service_template(const char *exec_path) > -{ > - int res = -1; > - char *filename; > - FILE *file; > - const char *unit; > - char *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "service"); > - > - filename = xdg_config_home_systemd(local_service_name); > - if (safe_create_leading_directories(filename)) { > - error(_("failed to create directories for '%s'"), filename); > - goto error; > - } > - file = fopen_or_warn(filename, "w"); > - if (!file) > - goto error; > - > - unit = "# This file was created and is maintained by Git.\n" > - "# Any edits made in this file might be replaced in the future\n" > - "# by a Git command.\n" > - "\n" > - "[Unit]\n" > - "Description=Optimize Git repositories data\n" > - "\n" > - "[Service]\n" > - "Type=oneshot\n" > - "ExecStart=\"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n" > - "LockPersonality=yes\n" > - "MemoryDenyWriteExecute=yes\n" > - "NoNewPrivileges=yes\n" > - "RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 AF_VSOCK\n" > - "RestrictNamespaces=yes\n" > - "RestrictRealtime=yes\n" > - "RestrictSUIDSGID=yes\n" > - "SystemCallArchitectures=native\n" > - "SystemCallFilter=@system-service\n"; > - if (fprintf(file, unit, exec_path, exec_path) < 0) { > - error(_("failed to write to '%s'"), filename); > - fclose(file); > - goto error; > - } > - if (fclose(file) == EOF) { > - error_errno(_("failed to flush '%s'"), filename); > - goto error; > - } > - > - res = 0; > - > -error: > - free(local_service_name); > - free(filename); > - return res; > -} > - > -static int systemd_timer_enable_unit(int enable, > - enum schedule_priority schedule, > - int minute) > +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; > - else if (systemd_timer_write_timer_file(schedule, minute)) > - return -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, SYSTEMD_UNIT_FORMAT, frequency, "timer"); > + > + 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; > } > > -/* > - * A previous version of Git wrote the timer units as template files. > - * Clean these up, if they exist. > - */ > -static void systemd_timer_delete_stale_timer_templates(void) > +static void systemd_delete_user_unit(char const *unit) > { > - char *timer_template_name = xstrfmt(SYSTEMD_UNIT_FORMAT, "", "timer"); > - char *filename = xdg_config_home_systemd(timer_template_name); > + char const file_start_stale[] = "# This file was created and is" > + " maintained by Git."; > + char file_start_user[sizeof(file_start_stale)] = {'\0'}; > + > + char *filename = xdg_config_home_systemd(unit); > + int handle = open(filename, O_RDONLY); > > - if (unlink(filename) && !is_missing_file_error(errno)) > + /* > + ** Check this is actually our file and we're not removing a legitimate > + ** user override. > + */ > + if (handle == -1 && !is_missing_file_error(errno)) > warning(_("failed to delete '%s'"), filename); > + else { > + read(handle, file_start_user, sizeof(file_start_stale) - 1); > + close(handle); > + if (strcmp(file_start_stale, file_start_user) == 0) { > + if (unlink(filename) == 0) > + warning(_("deleted stale unit file '%s'"), filename); > + else if (!is_missing_file_error(errno)) > + warning(_("failed to delete '%s'"), filename); > + } > + } > > free(filename); > - free(timer_template_name); > -} > - > -static int systemd_timer_delete_unit_files(void) > -{ > - systemd_timer_delete_stale_timer_templates(); > - > - /* Purposefully not short-circuited to make sure all are called. */ > - return systemd_timer_delete_timer_file(SCHEDULE_HOURLY) | > - systemd_timer_delete_timer_file(SCHEDULE_DAILY) | > - systemd_timer_delete_timer_file(SCHEDULE_WEEKLY) | > - systemd_timer_delete_service_template(); > -} > - > -static int systemd_timer_delete_units(void) > -{ > - int minute = get_random_minute(); > - /* Purposefully not short-circuited to make sure all are called. */ > - return systemd_timer_enable_unit(0, SCHEDULE_HOURLY, minute) | > - systemd_timer_enable_unit(0, SCHEDULE_DAILY, minute) | > - systemd_timer_enable_unit(0, SCHEDULE_WEEKLY, minute) | > - systemd_timer_delete_unit_files(); > -} > - > -static int systemd_timer_setup_units(void) > -{ > - int minute = get_random_minute(); > - const char *exec_path = git_exec_path(); > - > - int ret = systemd_timer_write_service_template(exec_path) || > - systemd_timer_enable_unit(1, SCHEDULE_HOURLY, minute) || > - systemd_timer_enable_unit(1, SCHEDULE_DAILY, minute) || > - systemd_timer_enable_unit(1, SCHEDULE_WEEKLY, minute); > - > - if (ret) > - systemd_timer_delete_units(); > - else > - systemd_timer_delete_stale_timer_templates(); > - > - 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_delete_units(); > + /* > + * A previous version of Git wrote the units in the user configuration > + * directory. Clean these up, if they exist. > + */ > + systemd_delete_user_unit("git-maintenance@hourly.timer"); > + systemd_delete_user_unit("git-maintenance@daily.timer"); > + systemd_delete_user_unit("git-maintenance@weekly.timer"); > + systemd_delete_user_unit("git-maintenance@.timer"); > + systemd_delete_user_unit("git-maintenance@.service"); > + return systemd_set_units_state(run_maintenance); > } > > enum scheduler { > -- > 2.44.0 > > ``` ## Patrick Steinhardt, 2024-03-21 12:37 Subject: Re: [RFC PATCH 4/5] maintenance: update systemd scheduler docs Message-ID: URL: https://gitlist.dev/e/ZfwqIklp5GoC1RP-%40tanuki In-Reply-To: <20240318153257.27451-5-mg@max.gautier.name> ``` On Mon, Mar 18, 2024 at 04:31:18PM +0100, Max Gautier wrote: This commit is missing an explanation what exactly you are updating. While you mention that you update something, the reader now has to find out by themselves what exactly you update by reading through the whole commit diff. Patrick > Signed-off-by: Max Gautier > --- > Documentation/git-maintenance.txt | 33 +++++++++++++++---------------- > 1 file changed, 16 insertions(+), 17 deletions(-) > > diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt > index 51d0f7e94b..6511c3f3f1 100644 > --- a/Documentation/git-maintenance.txt > +++ b/Documentation/git-maintenance.txt > @@ -304,10 +304,9 @@ distributions, systemd timers are superseding it. > If user systemd timers are available, they will be used as a replacement > of `cron`. > > -In this case, `git maintenance start` will create user systemd timer units > -and start the timers. The current list of user-scheduled tasks can be found > -by running `systemctl --user list-timers`. The timers written by `git > -maintenance start` are similar to this: > +In this case, `git maintenance start` will enable user systemd timer units > +and start them. The current list of user-scheduled tasks can be found by > +running `systemctl --user list-timers`. These timers are similar to this: > > ----------------------------------------------------------------------- > $ systemctl --user list-timers > @@ -317,25 +316,25 @@ Fri 2021-04-30 00:00:00 CEST 5h 42min left Thu 2021-04-29 00:00:11 CEST 18h ago > Mon 2021-05-03 00:00:00 CEST 3 days left Mon 2021-04-26 00:00:11 CEST 3 days ago git-maintenance@weekly.timer git-maintenance@weekly.service > ----------------------------------------------------------------------- > > -One timer is registered for each `--schedule=` option. > +One timer instance is enabled for each `--schedule=` option. > > -The definition of the systemd units can be inspected in the following files: > +The definition of the systemd units can be inspected this way: > > ----------------------------------------------------------------------- > -~/.config/systemd/user/git-maintenance@.timer > -~/.config/systemd/user/git-maintenance@.service > -~/.config/systemd/user/timers.target.wants/git-maintenance@hourly.timer > -~/.config/systemd/user/timers.target.wants/git-maintenance@daily.timer > -~/.config/systemd/user/timers.target.wants/git-maintenance@weekly.timer > +$ systemctl cat --user git-maintenance@.timer > +$ systemctl cat --user git-maintenance@.service > ----------------------------------------------------------------------- > > -`git maintenance start` will overwrite these files and start the timer > -again with `systemctl --user`, so any customization should be done by > -creating a drop-in file, i.e. a `.conf` suffixed file in the > -`~/.config/systemd/user/git-maintenance@.service.d` directory. > +Customization of the timer or service can be performed with the usual systemd > +tooling: > +----------------------------------------------------------------------- > +$ systemctl edit --user git-maintenance@.timer # all the timers > +$ systemctl edit --user git-maintenance@hourly.timer # the hourly timer > +$ systemctl edit --user git-maintenance@.service # all the services > +$ systemctl edit --user git-maintenance@hourly.service # the hourly run > +----------------------------------------------------------------------- > > -`git maintenance stop` will stop the user systemd timers and delete > -the above mentioned files. > +`git maintenance stop` will disable and stop the user systemd timers. > > For more details, see `systemd.timer(5)`. > > -- > 2.44.0 > > ``` ## Max Gautier, 2024-03-21 13:07 Subject: Re: [RFC PATCH 1/5] maintenance: package systemd units Message-ID: URL: https://gitlist.dev/e/Zfww9jI2em6ZY4SL%40framework In-Reply-To: ``` On Thu, Mar 21, 2024 at 01:37:30PM +0100, Patrick Steinhardt wrote: > On Mon, Mar 18, 2024 at 04:31:15PM +0100, Max Gautier wrote: > > It would be great to document _why_ we want to package the systemd units > alongside with Git. > Hum, I wrote that in the cover, but you're right, it should be in the commit itself. Ack. > > ... > > diff --git a/systemd/user/git-maintenance@.service b/systemd/user/git-maintenance@.service > > new file mode 100644 > > index 0000000000..87ac0c86e6 > > --- /dev/null > > +++ b/systemd/user/git-maintenance@.service > > @@ -0,0 +1,16 @@ > > +[Unit] > > +Description=Optimize Git repositories data > > + > > +[Service] > > +Type=oneshot > > +ExecStart=git for-each-repo --config=maintenance.repo \ > > + maintenance run --schedule=%i > > +LockPersonality=yes > > +MemoryDenyWriteExecute=yes > > +NoNewPrivileges=yes > > +RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 AF_VSOCK > > +RestrictNamespaces=yes > > +RestrictRealtime=yes > > +RestrictSUIDSGID=yes > > +SystemCallArchitectures=native > > +SystemCallFilter=@system-service > > Curious, but how did you arrive at these particular restrictions for the > unit? Might be something to explain in the commit message, as well. > > Patrick I copied the unit file which is defined in strings in builtin/gc.c, which I delete in patch 3. Should the moving be inside one commit, in order to explicit the fact that it's only moving things around ? -- Max Gautier ``` ## Max Gautier, 2024-03-21 13:13 Subject: Re: [RFC PATCH 2/5] maintenance: add fixed random delay to systemd timers Message-ID: URL: https://gitlist.dev/e/ZfwyhSgvhDASusFf%40framework In-Reply-To: ``` On Thu, Mar 21, 2024 at 01:37:35PM +0100, Patrick Steinhardt wrote: > On Mon, Mar 18, 2024 at 04:31:16PM +0100, Max Gautier wrote: > > Ensures that: > > - git maintenance timers have a fixed time interval between execution. > > - the three timers are not executed at the same time. > > Commit messages are typically structured so that you first explain what > the problem is that you're trying to solve, and then you explain how you > solve it. Your commit message is missing the first part. > > > This is intended to implement an alternative to the two followings > > commits: > > c97ec0378b (maintenance: fix systemd schedule overlaps, 2023-08-10) > > daa787010c (maintenance: use random minute in systemd scheduler, 2023-08-10) > > > > Instead of manually adding a specific minute (which is reset on each > > invocation of `git maintenance start`), we use systemd timers > > RandomizedDelaySec and FixedRandomDelay functionalities. > > I think it would help to not list commits, but put the commit references > in a paragraph. Something in the spirit of "In commit c97ec0378b we > already tried to address this issue in such and such a way, but that is > quite limiting due to that and that. Similarly, in commit daa787010c..". > Ok I see how that would work. Stating the limitations of the implemenation added by those commits would be the problem we're trying to solve, then the proposed solution. > > From man systemd.timer: > > >FixedRandomDelay= > > > Takes a boolean argument. When enabled, the randomized offset > > > specified by RandomizedDelaySec= is reused for all firings of the > > > same timer. For a given timer unit, **the offset depends on the > > > machine ID, user identifier and timer name**, which means that it is > > > stable between restarts of the manager. This effectively creates a > > > fixed offset for an individual timer, reducing the jitter in > > > firings of this timer, while still avoiding firing at the same time > > > as other similarly configured timers. > > > > -> which is exactly the use case for git-maintenance timers. > > > > Signed-off-by: Max Gautier > > --- > > systemd/user/git-maintenance@.service | 1 + > > systemd/user/git-maintenance@.timer | 3 +++ > > 2 files changed, 4 insertions(+) > > > > diff --git a/systemd/user/git-maintenance@.service b/systemd/user/git-maintenance@.service > > index 87ac0c86e6..f949e1a217 100644 > > --- a/systemd/user/git-maintenance@.service > > +++ b/systemd/user/git-maintenance@.service > > @@ -1,5 +1,6 @@ > > [Unit] > > Description=Optimize Git repositories data > > +Documentation=man:git-maintenance(1) > > This change feels unrelated and should likely go into the first commit. > > > > > [Service] > > Type=oneshot > > diff --git a/systemd/user/git-maintenance@.timer b/systemd/user/git-maintenance@.timer > > index 40fbc77a62..667c5998ba 100644 > > --- a/systemd/user/git-maintenance@.timer > > +++ b/systemd/user/git-maintenance@.timer > > @@ -1,9 +1,12 @@ > > [Unit] > > Description=Optimize Git repositories data > > +Documentation=man:git-maintenance(1) > > Same. > > Patrick Ack, will do. > > > [Timer] > > OnCalendar=%i > > Persistent=true > > +RandomizedDelaySec=1800 > > +FixedRandomDelay=true > > > > [Install] > > WantedBy=timers.target > > -- > > 2.44.0 > > > > -- Max Gautier ``` ## Max Gautier, 2024-03-21 13:19 Subject: Re: [RFC PATCH 3/5] maintenance: use packaged systemd units Message-ID: URL: https://gitlist.dev/e/Zfwz9zs3odKcU9Dm%40framework In-Reply-To: ``` On Thu, Mar 21, 2024 at 01:37:41PM +0100, Patrick Steinhardt wrote: > On Mon, Mar 18, 2024 at 04:31:17PM +0100, Max Gautier wrote: > > - Remove the code writing the units > > - Simplify calling systemctl > > - systemctl does not error out when disabling units which aren't > > enabled, nor when enabling already enabled units > > - call systemctl only once, with all the units. > > - Add clean-up code for leftover units in $XDG_CONFIG_HOME created by > > previous versions of git, taking care of preserving actual user > > override (by checking the start of the file). > > We very much try to avoid bulleted-list-style commit messages. Once you > have a list of changes it very often indicates that there are multiple > semi-related things going on in the same commit. At that point we prefer > to split the commit up into multiple commits so that each of the bullet > items can be explained separately. > > Also, as mentioned before, commit messages should explain what the > problem is and how that problem is solved. > > Patrick > I need to think a bit about patch ordering here because changing from the "random minute" implementation to the systemd (RandomDelaySec/FixedRandomDelay) is required to the other changes of putting the unit out of the code and in the package. I think I have an idea how, I'll work on that, thanks. -- Max Gautier ``` ## Patrick Steinhardt, 2024-03-21 13:22 Subject: Re: [RFC PATCH 1/5] maintenance: package systemd units Message-ID: URL: https://gitlist.dev/e/Zfw0rf7ez7Im1rIx%40tanuki In-Reply-To: ``` On Thu, Mar 21, 2024 at 02:07:02PM +0100, Max Gautier wrote: > On Thu, Mar 21, 2024 at 01:37:30PM +0100, Patrick Steinhardt wrote: > > On Mon, Mar 18, 2024 at 04:31:15PM +0100, Max Gautier wrote: > > > > It would be great to document _why_ we want to package the systemd units > > alongside with Git. > > > > Hum, I wrote that in the cover, but you're right, it should be in the > commit itself. > Ack. > > > > ... > > > diff --git a/systemd/user/git-maintenance@.service b/systemd/user/git-maintenance@.service > > > new file mode 100644 > > > index 0000000000..87ac0c86e6 > > > --- /dev/null > > > +++ b/systemd/user/git-maintenance@.service > > > @@ -0,0 +1,16 @@ > > > +[Unit] > > > +Description=Optimize Git repositories data > > > + > > > +[Service] > > > +Type=oneshot > > > +ExecStart=git for-each-repo --config=maintenance.repo \ > > > + maintenance run --schedule=%i > > > +LockPersonality=yes > > > +MemoryDenyWriteExecute=yes > > > +NoNewPrivileges=yes > > > +RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 AF_VSOCK > > > +RestrictNamespaces=yes > > > +RestrictRealtime=yes > > > +RestrictSUIDSGID=yes > > > +SystemCallArchitectures=native > > > +SystemCallFilter=@system-service > > > > Curious, but how did you arrive at these particular restrictions for the > > unit? Might be something to explain in the commit message, as well. > > > > Patrick > > I copied the unit file which is defined in strings in builtin/gc.c, > which I delete in patch 3. > Should the moving be inside one commit, in order to explicit the fact > that it's only moving things around ? I don't really think it's necessary. But pointing that out in the commit message would go a long way to explain where exactly these definitions come from. Patrick ``` ## Max Gautier, 2024-03-21 13:38 Subject: Re: [RFC PATCH 1/5] maintenance: package systemd units Message-ID: URL: https://gitlist.dev/e/Zfw4XNJdZqgZhvOv%40framework In-Reply-To: ``` On Thu, Mar 21, 2024 at 01:37:30PM +0100, Patrick Steinhardt wrote: > On Mon, Mar 18, 2024 at 04:31:15PM +0100, Max Gautier wrote: > > It would be great to document _why_ we want to package the systemd units > alongside with Git. > > > Signed-off-by: Max Gautier > > --- > > Makefile | 4 ++++ > > systemd/user/git-maintenance@.service | 16 ++++++++++++++++ > > systemd/user/git-maintenance@.timer | 9 +++++++++ > > 3 files changed, 29 insertions(+) > > create mode 100644 systemd/user/git-maintenance@.service > > create mode 100644 systemd/user/git-maintenance@.timer > > > > diff --git a/Makefile b/Makefile > > index 4e255c81f2..276b4373c6 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -619,6 +619,7 @@ htmldir = $(prefix)/share/doc/git-doc > > ETC_GITCONFIG = $(sysconfdir)/gitconfig > > ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes > > lib = lib > > +libdir = $(prefix)/lib > > # DESTDIR = > > pathsep = : > > > > @@ -1328,6 +1329,8 @@ BUILTIN_OBJS += builtin/verify-tag.o > > BUILTIN_OBJS += builtin/worktree.o > > BUILTIN_OBJS += builtin/write-tree.o > > > > +SYSTEMD_USER_UNITS := $(wildcard systemd/user/*) > > + > > # THIRD_PARTY_SOURCES is a list of patterns compatible with the > > # $(filter) and $(filter-out) family of functions. They specify source > > # files which are taken from some third-party source where we want to be > > @@ -3469,6 +3472,7 @@ install: all > > $(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' > > $(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)' > > $(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)' > > + $(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(libdir)/systemd/user' $(SYSTEMD_USER_UNITS) > > I wonder whether we want to unconditionally install those units. Many of > the platforms that we support don't even have systemd available, so > certainly it wouldn't make any sense to install it on those platforms. > > Assuming that this is something we want in the first place I thus think > that we should at least make this conditional and add some platform > specific quirk to "config.mak.uname". > We probably want that (conditional install) but I'm not sure where that should go ; I'm not super familiar with autoconf. I just noticed than man 7 daemon (shipped by systemd) propose the following snippet for installing systemd system services (should be easy enough to adapt for user ervices, I think): PKG_PROG_PKG_CONFIG() AC_ARG_WITH([systemdsystemunitdir], [AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd service files])],, [with_systemdsystemunitdir=auto]) AS_IF([test "x$with_systemdsystemunitdir" = "xyes" -o "x$with_systemdsystemunitdir" = "xauto"], [ def_systemdsystemunitdir=$($PKG_CONFIG --variable=systemdsystemunitdir systemd) AS_IF([test "x$def_systemdsystemunitdir" = "x"], [AS_IF([test "x$with_systemdsystemunitdir" = "xyes"], [AC_MSG_ERROR([systemd support requested but pkg-config unable to query systemd package])]) with_systemdsystemunitdir=no], [with_systemdsystemunitdir="$def_systemdsystemunitdir"])]) AS_IF([test "x$with_systemdsystemunitdir" != "xno"], [AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])]) AM_CONDITIONAL([HAVE_SYSTEMD], [test "x$with_systemdsystemunitdir" != "xno"]) Would something like that work ? -- Max Gautier ``` ## Patrick Steinhardt, 2024-03-21 14:44 Subject: Re: [RFC PATCH 1/5] maintenance: package systemd units Message-ID: URL: https://gitlist.dev/e/ZfxHtetjUIUByqeF%40tanuki In-Reply-To: ``` On Thu, Mar 21, 2024 at 02:38:36PM +0100, Max Gautier wrote: > On Thu, Mar 21, 2024 at 01:37:30PM +0100, Patrick Steinhardt wrote: > > On Mon, Mar 18, 2024 at 04:31:15PM +0100, Max Gautier wrote: > > > > It would be great to document _why_ we want to package the systemd units > > alongside with Git. > > > > > Signed-off-by: Max Gautier > > > --- > > > Makefile | 4 ++++ > > > systemd/user/git-maintenance@.service | 16 ++++++++++++++++ > > > systemd/user/git-maintenance@.timer | 9 +++++++++ > > > 3 files changed, 29 insertions(+) > > > create mode 100644 systemd/user/git-maintenance@.service > > > create mode 100644 systemd/user/git-maintenance@.timer > > > > > > diff --git a/Makefile b/Makefile > > > index 4e255c81f2..276b4373c6 100644 > > > --- a/Makefile > > > +++ b/Makefile > > > @@ -619,6 +619,7 @@ htmldir = $(prefix)/share/doc/git-doc > > > ETC_GITCONFIG = $(sysconfdir)/gitconfig > > > ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes > > > lib = lib > > > +libdir = $(prefix)/lib > > > # DESTDIR = > > > pathsep = : > > > > > > @@ -1328,6 +1329,8 @@ BUILTIN_OBJS += builtin/verify-tag.o > > > BUILTIN_OBJS += builtin/worktree.o > > > BUILTIN_OBJS += builtin/write-tree.o > > > > > > +SYSTEMD_USER_UNITS := $(wildcard systemd/user/*) > > > + > > > # THIRD_PARTY_SOURCES is a list of patterns compatible with the > > > # $(filter) and $(filter-out) family of functions. They specify source > > > # files which are taken from some third-party source where we want to be > > > @@ -3469,6 +3472,7 @@ install: all > > > $(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' > > > $(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)' > > > $(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)' > > > + $(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(libdir)/systemd/user' $(SYSTEMD_USER_UNITS) > > > > I wonder whether we want to unconditionally install those units. Many of > > the platforms that we support don't even have systemd available, so > > certainly it wouldn't make any sense to install it on those platforms. > > > > Assuming that this is something we want in the first place I thus think > > that we should at least make this conditional and add some platform > > specific quirk to "config.mak.uname". > > > > We probably want that (conditional install) but I'm not sure where that > should go ; I'm not super familiar with autoconf. > > I just noticed than man 7 daemon (shipped by systemd) propose the > following snippet for installing systemd system services (should be easy > enough to adapt for user ervices, I think): > > PKG_PROG_PKG_CONFIG() > AC_ARG_WITH([systemdsystemunitdir], > [AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd service files])],, > [with_systemdsystemunitdir=auto]) > AS_IF([test "x$with_systemdsystemunitdir" = "xyes" -o "x$with_systemdsystemunitdir" = "xauto"], [ > def_systemdsystemunitdir=$($PKG_CONFIG --variable=systemdsystemunitdir systemd) > > AS_IF([test "x$def_systemdsystemunitdir" = "x"], > [AS_IF([test "x$with_systemdsystemunitdir" = "xyes"], > [AC_MSG_ERROR([systemd support requested but pkg-config unable to query systemd package])]) > with_systemdsystemunitdir=no], > [with_systemdsystemunitdir="$def_systemdsystemunitdir"])]) > AS_IF([test "x$with_systemdsystemunitdir" != "xno"], > [AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])]) > AM_CONDITIONAL([HAVE_SYSTEMD], [test "x$with_systemdsystemunitdir" != "xno"]) > > Would something like that work ? Probably. But while we do have autoconf wired up, the primary way of building Git does not use it, but rather uses `config.mak.uname`. I'd recommend to have a look at it to figure out how we handle other build-time options there. Patrick ``` ## Max Gautier, 2024-03-21 14:48 Subject: Re: [RFC PATCH 1/5] maintenance: package systemd units Message-ID: URL: https://gitlist.dev/e/ZfxIungwqHdGCZlv%40framework In-Reply-To: ``` On Thu, Mar 21, 2024 at 02:38:36PM +0100, Max Gautier wrote: > On Thu, Mar 21, 2024 at 01:37:30PM +0100, Patrick Steinhardt wrote: > > On Mon, Mar 18, 2024 at 04:31:15PM +0100, Max Gautier wrote: > > > > It would be great to document _why_ we want to package the systemd units > > alongside with Git. > > > > > Signed-off-by: Max Gautier > > > --- > > > Makefile | 4 ++++ > > > systemd/user/git-maintenance@.service | 16 ++++++++++++++++ > > > systemd/user/git-maintenance@.timer | 9 +++++++++ > > > 3 files changed, 29 insertions(+) > > > create mode 100644 systemd/user/git-maintenance@.service > > > create mode 100644 systemd/user/git-maintenance@.timer > > > > > > diff --git a/Makefile b/Makefile > > > index 4e255c81f2..276b4373c6 100644 > > > --- a/Makefile > > > +++ b/Makefile > > > @@ -619,6 +619,7 @@ htmldir = $(prefix)/share/doc/git-doc > > > ETC_GITCONFIG = $(sysconfdir)/gitconfig > > > ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes > > > lib = lib > > > +libdir = $(prefix)/lib > > > # DESTDIR = > > > pathsep = : > > > > > > @@ -1328,6 +1329,8 @@ BUILTIN_OBJS += builtin/verify-tag.o > > > BUILTIN_OBJS += builtin/worktree.o > > > BUILTIN_OBJS += builtin/write-tree.o > > > > > > +SYSTEMD_USER_UNITS := $(wildcard systemd/user/*) > > > + > > > # THIRD_PARTY_SOURCES is a list of patterns compatible with the > > > # $(filter) and $(filter-out) family of functions. They specify source > > > # files which are taken from some third-party source where we want to be > > > @@ -3469,6 +3472,7 @@ install: all > > > $(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' > > > $(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)' > > > $(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)' > > > + $(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(libdir)/systemd/user' $(SYSTEMD_USER_UNITS) > > > > I wonder whether we want to unconditionally install those units. Many of > > the platforms that we support don't even have systemd available, so > > certainly it wouldn't make any sense to install it on those platforms. > > > > Assuming that this is something we want in the first place I thus think > > that we should at least make this conditional and add some platform > > specific quirk to "config.mak.uname". > > > > We probably want that (conditional install) but I'm not sure where that > should go ; I'm not super familiar with autoconf. > > I just noticed than man 7 daemon (shipped by systemd) propose the > following snippet for installing systemd system services (should be easy > enough to adapt for user ervices, I think): > > PKG_PROG_PKG_CONFIG() > AC_ARG_WITH([systemdsystemunitdir], > [AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd service files])],, > [with_systemdsystemunitdir=auto]) > AS_IF([test "x$with_systemdsystemunitdir" = "xyes" -o "x$with_systemdsystemunitdir" = "xauto"], [ > def_systemdsystemunitdir=$($PKG_CONFIG --variable=systemdsystemunitdir systemd) > > AS_IF([test "x$def_systemdsystemunitdir" = "x"], > [AS_IF([test "x$with_systemdsystemunitdir" = "xyes"], > [AC_MSG_ERROR([systemd support requested but pkg-config unable to query systemd package])]) > with_systemdsystemunitdir=no], > [with_systemdsystemunitdir="$def_systemdsystemunitdir"])]) > AS_IF([test "x$with_systemdsystemunitdir" != "xno"], > [AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])]) > AM_CONDITIONAL([HAVE_SYSTEMD], [test "x$with_systemdsystemunitdir" != "xno"]) > > Would something like that work ? Ugh, that snippet apparently requires that I delete aclocal.m4, execute `autoreconf --install`, then concat together the previous aclocal.m4 and the newly generated one. I'm adding the last authors on aclocal.m4 to the discussion, maybe they have some lights ? (Though they don't have commits in the last ten years, so it might be a very long shot ...) -- Max Gautier ``` ## Max Gautier, 2024-03-21 14:49 Subject: Re: [RFC PATCH 1/5] maintenance: package systemd units Message-ID: URL: https://gitlist.dev/e/ZfxI5et2h6Bx1bym%40framework In-Reply-To: ``` On Thu, Mar 21, 2024 at 03:44:05PM +0100, Patrick Steinhardt wrote: > On Thu, Mar 21, 2024 at 02:38:36PM +0100, Max Gautier wrote: > > On Thu, Mar 21, 2024 at 01:37:30PM +0100, Patrick Steinhardt wrote: > > > On Mon, Mar 18, 2024 at 04:31:15PM +0100, Max Gautier wrote: > > > > > > It would be great to document _why_ we want to package the systemd units > > > alongside with Git. > > > > > > > Signed-off-by: Max Gautier > > > > --- > > > > Makefile | 4 ++++ > > > > systemd/user/git-maintenance@.service | 16 ++++++++++++++++ > > > > systemd/user/git-maintenance@.timer | 9 +++++++++ > > > > 3 files changed, 29 insertions(+) > > > > create mode 100644 systemd/user/git-maintenance@.service > > > > create mode 100644 systemd/user/git-maintenance@.timer > > > > > > > > diff --git a/Makefile b/Makefile > > > > index 4e255c81f2..276b4373c6 100644 > > > > --- a/Makefile > > > > +++ b/Makefile > > > > @@ -619,6 +619,7 @@ htmldir = $(prefix)/share/doc/git-doc > > > > ETC_GITCONFIG = $(sysconfdir)/gitconfig > > > > ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes > > > > lib = lib > > > > +libdir = $(prefix)/lib > > > > # DESTDIR = > > > > pathsep = : > > > > > > > > @@ -1328,6 +1329,8 @@ BUILTIN_OBJS += builtin/verify-tag.o > > > > BUILTIN_OBJS += builtin/worktree.o > > > > BUILTIN_OBJS += builtin/write-tree.o > > > > > > > > +SYSTEMD_USER_UNITS := $(wildcard systemd/user/*) > > > > + > > > > # THIRD_PARTY_SOURCES is a list of patterns compatible with the > > > > # $(filter) and $(filter-out) family of functions. They specify source > > > > # files which are taken from some third-party source where we want to be > > > > @@ -3469,6 +3472,7 @@ install: all > > > > $(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' > > > > $(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)' > > > > $(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)' > > > > + $(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(libdir)/systemd/user' $(SYSTEMD_USER_UNITS) > > > > > > I wonder whether we want to unconditionally install those units. Many of > > > the platforms that we support don't even have systemd available, so > > > certainly it wouldn't make any sense to install it on those platforms. > > > > > > Assuming that this is something we want in the first place I thus think > > > that we should at least make this conditional and add some platform > > > specific quirk to "config.mak.uname". > > > > > > > We probably want that (conditional install) but I'm not sure where that > > should go ; I'm not super familiar with autoconf. > > > > I just noticed than man 7 daemon (shipped by systemd) propose the > > following snippet for installing systemd system services (should be easy > > enough to adapt for user ervices, I think): > > > > PKG_PROG_PKG_CONFIG() > > AC_ARG_WITH([systemdsystemunitdir], > > [AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd service files])],, > > [with_systemdsystemunitdir=auto]) > > AS_IF([test "x$with_systemdsystemunitdir" = "xyes" -o "x$with_systemdsystemunitdir" = "xauto"], [ > > def_systemdsystemunitdir=$($PKG_CONFIG --variable=systemdsystemunitdir systemd) > > > > AS_IF([test "x$def_systemdsystemunitdir" = "x"], > > [AS_IF([test "x$with_systemdsystemunitdir" = "xyes"], > > [AC_MSG_ERROR([systemd support requested but pkg-config unable to query systemd package])]) > > with_systemdsystemunitdir=no], > > [with_systemdsystemunitdir="$def_systemdsystemunitdir"])]) > > AS_IF([test "x$with_systemdsystemunitdir" != "xno"], > > [AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])]) > > AM_CONDITIONAL([HAVE_SYSTEMD], [test "x$with_systemdsystemunitdir" != "xno"]) > > > > Would something like that work ? > > Probably. But while we do have autoconf wired up, the primary way of > building Git does not use it, but rather uses `config.mak.uname`. I'd > recommend to have a look at it to figure out how we handle other > build-time options there. > > Patrick I'll take a look, thanks. -- Max Gautier ```