{"thread":{"id":"61145","subject":"[RFC PATCH 0/5] maintenance: use packaged systemd units","startedAt":"2024-03-18T15:33:17Z","lastAt":"2024-03-21T14:49:11Z","messageCount":22,"participants":["Max Gautier","Eric Sunshine","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"490869","messageId":"20240318153257.27451-1-mg@max.gautier.name","threadId":"61145","inReplyTo":null,"subject":"[RFC PATCH 0/5] maintenance: use packaged systemd units","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-18T15:31:14Z","receivedAt":"2024-03-18T15:33:17Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"Please ignore the previous series, I messed up the cover letter and Cc\nwith git-send-email somehow...\n\n-----\n\nHello,\n\nThis is a proposal to distribute the timers of the systemd scheduler of\ngit-maintenance directly, rather than inlining them into the code and\nwriting them on demand.\nIMO, this is better than using the user $XDG_CONFIG_HOME, and allows\nthat user to override them if they so wish.\n\nWe also move away from using the random minute, and instead rely on\nsystemd features to achieve the same goal (see patch 2). This allows us\nto go back to using unit templating for the timers.\n(Not that even if we really more specific OnCalendar= settings for each\ntimer, we should still do it that way, but instead distribute override\nalongside the template, i.e\n\n/usr/lib/systemd-user/git-maintenance@daily.timer.d/override.conf:\n[Timer]\nOnCalendar=\n\nThe cleanup code for the units written in $XDG_CONFIG_HOME is adapted,\nand takes care of not removing legitimate user overrides, by checking\nthe file start.\nPatch 5 removes the cleanup code, it should not be applied right away,\nbut once enough time has passed and we can be reasonably sure that no\none still has the old units laying around.\n\nTesting:\nThe simplest way to simulate having the units in /usr/lib is probably to\ncopy them in /etc/systemd/user.\n\nUnresolved (reason why it's still an RFC):\n- Should I implement conditional install of systemd units (if systemd is\n available) ? I've avoided digging too deep in the Makefile, but that's\n doable, I think.\n\n\n---\n Documentation/git-maintenance.txt | 33 ++--\n Makefile | 4 +\n builtin/gc.c | 279 ++--------------------------------\n systemd/user/git-maintenance@.service | 17 +++\n systemd/user/git-maintenance@.timer | 12 ++\n 5 files changed, 63 insertions(+), 282 deletions(-)\n\n"},{"id":"490870","messageId":"20240318153257.27451-2-mg@max.gautier.name","threadId":"61145","inReplyTo":"20240318153257.27451-1-mg@max.gautier.name","subject":"[RFC PATCH 1/5] maintenance: package systemd units","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-18T15:31:15Z","receivedAt":"2024-03-18T15:33:17Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"Signed-off-by: Max Gautier \n---\n Makefile | 4 ++++\n systemd/user/git-maintenance@.service | 16 ++++++++++++++++\n systemd/user/git-maintenance@.timer | 9 +++++++++\n 3 files changed, 29 insertions(+)\n create mode 100644 systemd/user/git-maintenance@.service\n create mode 100644 systemd/user/git-maintenance@.timer\n\ndiff --git a/Makefile b/Makefile\nindex 4e255c81f2..276b4373c6 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -619,6 +619,7 @@ htmldir = $(prefix)/share/doc/git-doc\n ETC_GITCONFIG = $(sysconfdir)/gitconfig\n ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes\n lib = lib\n+libdir = $(prefix)/lib\n # DESTDIR =\n pathsep = :\n \n@@ -1328,6 +1329,8 @@ BUILTIN_OBJS += builtin/verify-tag.o\n BUILTIN_OBJS += builtin/worktree.o\n BUILTIN_OBJS += builtin/write-tree.o\n \n+SYSTEMD_USER_UNITS := $(wildcard systemd/user/*)\n+\n # THIRD_PARTY_SOURCES is a list of patterns compatible with the\n # $(filter) and $(filter-out) family of functions. They specify source\n # files which are taken from some third-party source where we want to be\n@@ -3469,6 +3472,7 @@ install: all\n \t$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'\n \t$(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)'\n \t$(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)'\n+\t$(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(libdir)/systemd/user' $(SYSTEMD_USER_UNITS)\n \n ifdef MSVC\n \t# We DO NOT install the individual foo.o.pdb files because they\ndiff --git a/systemd/user/git-maintenance@.service b/systemd/user/git-maintenance@.service\nnew file mode 100644\nindex 0000000000..87ac0c86e6\n--- /dev/null\n+++ b/systemd/user/git-maintenance@.service\n@@ -0,0 +1,16 @@\n+[Unit]\n+Description=Optimize Git repositories data\n+\n+[Service]\n+Type=oneshot\n+ExecStart=git for-each-repo --config=maintenance.repo \\\n+ 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\ndiff --git a/systemd/user/git-maintenance@.timer b/systemd/user/git-maintenance@.timer\nnew file mode 100644\nindex 0000000000..40fbc77a62\n--- /dev/null\n+++ b/systemd/user/git-maintenance@.timer\n@@ -0,0 +1,9 @@\n+[Unit]\n+Description=Optimize Git repositories data\n+\n+[Timer]\n+OnCalendar=%i\n+Persistent=true\n+\n+[Install]\n+WantedBy=timers.target\n-- \n2.44.0\n\n"},{"id":"490872","messageId":"20240318153257.27451-3-mg@max.gautier.name","threadId":"61145","inReplyTo":"20240318153257.27451-1-mg@max.gautier.name","subject":"[RFC PATCH 2/5] maintenance: add fixed random delay to systemd timers","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-18T15:31:16Z","receivedAt":"2024-03-18T15:33:17Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"Ensures that:\n- git maintenance timers have a fixed time interval between execution.\n- the three timers are not executed at the same time.\n\nThis is intended to implement an alternative to the two followings\ncommits:\nc97ec0378b (maintenance: fix systemd schedule overlaps, 2023-08-10)\ndaa787010c (maintenance: use random minute in systemd scheduler, 2023-08-10)\n\nInstead of manually adding a specific minute (which is reset on each\ninvocation of `git maintenance start`), we use systemd timers\nRandomizedDelaySec and FixedRandomDelay functionalities.\n\nFrom man systemd.timer:\n>FixedRandomDelay=\n> Takes a boolean argument. When enabled, the randomized offset\n> specified by RandomizedDelaySec= is reused for all firings of the\n> same timer. For a given timer unit, **the offset depends on the\n> machine ID, user identifier and timer name**, which means that it is\n> stable between restarts of the manager. This effectively creates a\n> fixed offset for an individual timer, reducing the jitter in\n> firings of this timer, while still avoiding firing at the same time\n> as other similarly configured timers.\n\n-> which is exactly the use case for git-maintenance timers.\n\nSigned-off-by: Max Gautier \n---\n systemd/user/git-maintenance@.service | 1 +\n systemd/user/git-maintenance@.timer | 3 +++\n 2 files changed, 4 insertions(+)\n\ndiff --git a/systemd/user/git-maintenance@.service b/systemd/user/git-maintenance@.service\nindex 87ac0c86e6..f949e1a217 100644\n--- a/systemd/user/git-maintenance@.service\n+++ b/systemd/user/git-maintenance@.service\n@@ -1,5 +1,6 @@\n [Unit]\n Description=Optimize Git repositories data\n+Documentation=man:git-maintenance(1)\n \n [Service]\n Type=oneshot\ndiff --git a/systemd/user/git-maintenance@.timer b/systemd/user/git-maintenance@.timer\nindex 40fbc77a62..667c5998ba 100644\n--- a/systemd/user/git-maintenance@.timer\n+++ b/systemd/user/git-maintenance@.timer\n@@ -1,9 +1,12 @@\n [Unit]\n Description=Optimize Git repositories data\n+Documentation=man:git-maintenance(1)\n \n [Timer]\n OnCalendar=%i\n Persistent=true\n+RandomizedDelaySec=1800\n+FixedRandomDelay=true\n \n [Install]\n WantedBy=timers.target\n-- \n2.44.0\n\n"},{"id":"490871","messageId":"20240318153257.27451-5-mg@max.gautier.name","threadId":"61145","inReplyTo":"20240318153257.27451-1-mg@max.gautier.name","subject":"[RFC PATCH 4/5] maintenance: update systemd scheduler docs","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-18T15:31:18Z","receivedAt":"2024-03-18T15:33:18Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"Signed-off-by: Max Gautier \n---\n Documentation/git-maintenance.txt | 33 +++++++++++++++----------------\n 1 file changed, 16 insertions(+), 17 deletions(-)\n\ndiff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt\nindex 51d0f7e94b..6511c3f3f1 100644\n--- a/Documentation/git-maintenance.txt\n+++ b/Documentation/git-maintenance.txt\n@@ -304,10 +304,9 @@ distributions, systemd timers are superseding it.\n If user systemd timers are available, they will be used as a replacement\n of `cron`.\n \n-In this case, `git maintenance start` will create user systemd timer units\n-and start the timers. The current list of user-scheduled tasks can be found\n-by running `systemctl --user list-timers`. The timers written by `git\n-maintenance start` are similar to this:\n+In this case, `git maintenance start` will enable user systemd timer units\n+and start them. The current list of user-scheduled tasks can be found by\n+running `systemctl --user list-timers`. These timers are similar to this:\n \n -----------------------------------------------------------------------\n $ systemctl --user list-timers\n@@ -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\n 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\n -----------------------------------------------------------------------\n \n-One timer is registered for each `--schedule=` option.\n+One timer instance is enabled for each `--schedule=` option.\n \n-The definition of the systemd units can be inspected in the following files:\n+The definition of the systemd units can be inspected this way:\n \n -----------------------------------------------------------------------\n-~/.config/systemd/user/git-maintenance@.timer\n-~/.config/systemd/user/git-maintenance@.service\n-~/.config/systemd/user/timers.target.wants/git-maintenance@hourly.timer\n-~/.config/systemd/user/timers.target.wants/git-maintenance@daily.timer\n-~/.config/systemd/user/timers.target.wants/git-maintenance@weekly.timer\n+$ systemctl cat --user git-maintenance@.timer\n+$ systemctl cat --user git-maintenance@.service\n -----------------------------------------------------------------------\n \n-`git maintenance start` will overwrite these files and start the timer\n-again with `systemctl --user`, so any customization should be done by\n-creating a drop-in file, i.e. a `.conf` suffixed file in the\n-`~/.config/systemd/user/git-maintenance@.service.d` directory.\n+Customization of the timer or service can be performed with the usual systemd\n+tooling:\n+-----------------------------------------------------------------------\n+$ systemctl edit --user git-maintenance@.timer # all the timers\n+$ systemctl edit --user git-maintenance@hourly.timer # the hourly timer\n+$ systemctl edit --user git-maintenance@.service # all the services\n+$ systemctl edit --user git-maintenance@hourly.service # the hourly run\n+-----------------------------------------------------------------------\n \n-`git maintenance stop` will stop the user systemd timers and delete\n-the above mentioned files.\n+`git maintenance stop` will disable and stop the user systemd timers.\n \n For more details, see `systemd.timer(5)`.\n \n-- \n2.44.0\n\n"},{"id":"490873","messageId":"20240318153257.27451-4-mg@max.gautier.name","threadId":"61145","inReplyTo":"20240318153257.27451-1-mg@max.gautier.name","subject":"[RFC PATCH 3/5] maintenance: use packaged systemd units","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-18T15:31:17Z","receivedAt":"2024-03-18T15:33:18Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"- Remove the code writing the units\n- Simplify calling systemctl\n - systemctl does not error out when disabling units which aren't\n enabled, nor when enabling already enabled units\n - call systemctl only once, with all the units.\n- Add clean-up code for leftover units in $XDG_CONFIG_HOME created by\n previous versions of git, taking care of preserving actual user\n override (by checking the start of the file).\n\nSigned-off-by: Max Gautier \n---\n builtin/gc.c | 293 ++++++++-------------------------------------------\n 1 file changed, 45 insertions(+), 248 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex cb80ced6cb..981db8e297 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -2308,276 +2308,73 @@ static char *xdg_config_home_systemd(const char *filename)\n \treturn xdg_config_home_for(\"systemd/user\", filename);\n }\n \n-#define SYSTEMD_UNIT_FORMAT \"git-maintenance@%s.%s\"\n-\n-static int systemd_timer_delete_timer_file(enum schedule_priority priority)\n-{\n-\tint ret = 0;\n-\tconst char *frequency = get_frequency(priority);\n-\tchar *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, \"timer\");\n-\tchar *filename = xdg_config_home_systemd(local_timer_name);\n-\n-\tif (unlink(filename) && !is_missing_file_error(errno))\n-\t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n-\n-\tfree(filename);\n-\tfree(local_timer_name);\n-\treturn ret;\n-}\n-\n-static int systemd_timer_delete_service_template(void)\n-{\n-\tint ret = 0;\n-\tchar *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, \"\", \"service\");\n-\tchar *filename = xdg_config_home_systemd(local_service_name);\n-\tif (unlink(filename) && !is_missing_file_error(errno))\n-\t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n-\n-\tfree(filename);\n-\tfree(local_service_name);\n-\treturn ret;\n-}\n-\n-/*\n- * Write the schedule information into a git-maintenance@.timer\n- * file using a custom minute. This timer file cannot use the templating\n- * system, so we generate a specific file for each.\n- */\n-static int systemd_timer_write_timer_file(enum schedule_priority schedule,\n-\t\t\t\t\t int minute)\n-{\n-\tint res = -1;\n-\tchar *filename;\n-\tFILE *file;\n-\tconst char *unit;\n-\tchar *schedule_pattern = NULL;\n-\tconst char *frequency = get_frequency(schedule);\n-\tchar *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, \"timer\");\n-\n-\tfilename = xdg_config_home_systemd(local_timer_name);\n-\n-\tif (safe_create_leading_directories(filename)) {\n-\t\terror(_(\"failed to create directories for '%s'\"), filename);\n-\t\tgoto error;\n-\t}\n-\tfile = fopen_or_warn(filename, \"w\");\n-\tif (!file)\n-\t\tgoto error;\n-\n-\tswitch (schedule) {\n-\tcase SCHEDULE_HOURLY:\n-\t\tschedule_pattern = xstrfmt(\"*-*-* 1..23:%02d:00\", minute);\n-\t\tbreak;\n-\n-\tcase SCHEDULE_DAILY:\n-\t\tschedule_pattern = xstrfmt(\"Tue..Sun *-*-* 0:%02d:00\", minute);\n-\t\tbreak;\n-\n-\tcase SCHEDULE_WEEKLY:\n-\t\tschedule_pattern = xstrfmt(\"Mon 0:%02d:00\", minute);\n-\t\tbreak;\n-\n-\tdefault:\n-\t\tBUG(\"Unhandled schedule_priority\");\n-\t}\n-\n-\tunit = \"# This file was created and is maintained by Git.\\n\"\n-\t \"# Any edits made in this file might be replaced in the future\\n\"\n-\t \"# by a Git command.\\n\"\n-\t \"\\n\"\n-\t \"[Unit]\\n\"\n-\t \"Description=Optimize Git repositories data\\n\"\n-\t \"\\n\"\n-\t \"[Timer]\\n\"\n-\t \"OnCalendar=%s\\n\"\n-\t \"Persistent=true\\n\"\n-\t \"\\n\"\n-\t \"[Install]\\n\"\n-\t \"WantedBy=timers.target\\n\";\n-\tif (fprintf(file, unit, schedule_pattern) < 0) {\n-\t\terror(_(\"failed to write to '%s'\"), filename);\n-\t\tfclose(file);\n-\t\tgoto error;\n-\t}\n-\tif (fclose(file) == EOF) {\n-\t\terror_errno(_(\"failed to flush '%s'\"), filename);\n-\t\tgoto error;\n-\t}\n-\n-\tres = 0;\n-\n-error:\n-\tfree(schedule_pattern);\n-\tfree(local_timer_name);\n-\tfree(filename);\n-\treturn res;\n-}\n-\n-/*\n- * No matter the schedule, we use the same service and can make use of the\n- * templating system. When installing git-maintenance@.timer,\n- * systemd will notice that git-maintenance@.service exists as a template\n- * and will use this file and insert the into the template at\n- * the position of \"%i\".\n- */\n-static int systemd_timer_write_service_template(const char *exec_path)\n-{\n-\tint res = -1;\n-\tchar *filename;\n-\tFILE *file;\n-\tconst char *unit;\n-\tchar *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, \"\", \"service\");\n-\n-\tfilename = xdg_config_home_systemd(local_service_name);\n-\tif (safe_create_leading_directories(filename)) {\n-\t\terror(_(\"failed to create directories for '%s'\"), filename);\n-\t\tgoto error;\n-\t}\n-\tfile = fopen_or_warn(filename, \"w\");\n-\tif (!file)\n-\t\tgoto error;\n-\n-\tunit = \"# This file was created and is maintained by Git.\\n\"\n-\t \"# Any edits made in this file might be replaced in the future\\n\"\n-\t \"# by a Git command.\\n\"\n-\t \"\\n\"\n-\t \"[Unit]\\n\"\n-\t \"Description=Optimize Git repositories data\\n\"\n-\t \"\\n\"\n-\t \"[Service]\\n\"\n-\t \"Type=oneshot\\n\"\n-\t \"ExecStart=\\\"%s/git\\\" --exec-path=\\\"%s\\\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\\n\"\n-\t \"LockPersonality=yes\\n\"\n-\t \"MemoryDenyWriteExecute=yes\\n\"\n-\t \"NoNewPrivileges=yes\\n\"\n-\t \"RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 AF_VSOCK\\n\"\n-\t \"RestrictNamespaces=yes\\n\"\n-\t \"RestrictRealtime=yes\\n\"\n-\t \"RestrictSUIDSGID=yes\\n\"\n-\t \"SystemCallArchitectures=native\\n\"\n-\t \"SystemCallFilter=@system-service\\n\";\n-\tif (fprintf(file, unit, exec_path, exec_path) < 0) {\n-\t\terror(_(\"failed to write to '%s'\"), filename);\n-\t\tfclose(file);\n-\t\tgoto error;\n-\t}\n-\tif (fclose(file) == EOF) {\n-\t\terror_errno(_(\"failed to flush '%s'\"), filename);\n-\t\tgoto error;\n-\t}\n-\n-\tres = 0;\n-\n-error:\n-\tfree(local_service_name);\n-\tfree(filename);\n-\treturn res;\n-}\n-\n-static int systemd_timer_enable_unit(int enable,\n-\t\t\t\t enum schedule_priority schedule,\n-\t\t\t\t int minute)\n+static int systemd_set_units_state(int enable)\n {\n \tconst char *cmd = \"systemctl\";\n \tstruct child_process child = CHILD_PROCESS_INIT;\n-\tconst char *frequency = get_frequency(schedule);\n-\n-\t/*\n-\t * Disabling the systemd unit while it is already disabled makes\n-\t * systemctl print an error.\n-\t * Let's ignore it since it means we already are in the expected state:\n-\t * the unit is disabled.\n-\t *\n-\t * On the other hand, enabling a systemd unit which is already enabled\n-\t * produces no error.\n-\t */\n-\tif (!enable)\n-\t\tchild.no_stderr = 1;\n-\telse if (systemd_timer_write_timer_file(schedule, minute))\n-\t\treturn -1;\n \n \tget_schedule_cmd(&cmd, NULL);\n \tstrvec_split(&child.args, cmd);\n-\tstrvec_pushl(&child.args, \"--user\", enable ? \"enable\" : \"disable\",\n-\t\t \"--now\", NULL);\n-\tstrvec_pushf(&child.args, SYSTEMD_UNIT_FORMAT, frequency, \"timer\");\n+\n+\tstrvec_pushl(&child.args, \"--user\", \"--force\", \"--now\",\n+\t\t\tenable ? \"enable\" : \"disable\",\n+\t\t\t\"git-maintenance@hourly.timer\",\n+\t\t\t\"git-maintenance@daily.timer\",\n+\t\t\t\"git-maintenance@weekly.timer\", NULL);\n+\t/*\n+\t** --force override existing conflicting symlinks\n+\t** We need it because the units have changed location (~/.config ->\n+\t** /usr/lib)\n+\t*/\n \n \tif (start_command(&child))\n \t\treturn error(_(\"failed to start systemctl\"));\n \tif (finish_command(&child))\n-\t\t/*\n-\t\t * Disabling an already disabled systemd unit makes\n-\t\t * systemctl fail.\n-\t\t * Let's ignore this failure.\n-\t\t *\n-\t\t * Enabling an enabled systemd unit doesn't fail.\n-\t\t */\n-\t\tif (enable)\n-\t\t\treturn error(_(\"failed to run systemctl\"));\n+\t\treturn error(_(\"failed to run systemctl\"));\n \treturn 0;\n }\n \n-/*\n- * A previous version of Git wrote the timer units as template files.\n- * Clean these up, if they exist.\n- */\n-static void systemd_timer_delete_stale_timer_templates(void)\n+static void systemd_delete_user_unit(char const *unit)\n {\n-\tchar *timer_template_name = xstrfmt(SYSTEMD_UNIT_FORMAT, \"\", \"timer\");\n-\tchar *filename = xdg_config_home_systemd(timer_template_name);\n+\tchar const\tfile_start_stale[] =\t\"# This file was created and is\"\n+\t\t\t\t\t\t\" maintained by Git.\";\n+\tchar\t\tfile_start_user[sizeof(file_start_stale)] = {'\\0'};\n+\n+\tchar *filename = xdg_config_home_systemd(unit);\n+\tint handle = open(filename, O_RDONLY);\n \n-\tif (unlink(filename) && !is_missing_file_error(errno))\n+\t/*\n+\t** Check this is actually our file and we're not removing a legitimate\n+\t** user override.\n+\t*/\n+\tif (handle == -1 && !is_missing_file_error(errno))\n \t\twarning(_(\"failed to delete '%s'\"), filename);\n+\telse {\n+\t\tread(handle, file_start_user, sizeof(file_start_stale) - 1);\n+\t\tclose(handle);\n+\t\tif (strcmp(file_start_stale, file_start_user) == 0) {\n+\t\t\tif (unlink(filename) == 0)\n+\t\t\t\twarning(_(\"deleted stale unit file '%s'\"), filename);\n+\t\t\telse if (!is_missing_file_error(errno))\n+\t\t\t\twarning(_(\"failed to delete '%s'\"), filename);\n+\t\t}\n+\t}\n \n \tfree(filename);\n-\tfree(timer_template_name);\n-}\n-\n-static int systemd_timer_delete_unit_files(void)\n-{\n-\tsystemd_timer_delete_stale_timer_templates();\n-\n-\t/* Purposefully not short-circuited to make sure all are called. */\n-\treturn systemd_timer_delete_timer_file(SCHEDULE_HOURLY) |\n-\t systemd_timer_delete_timer_file(SCHEDULE_DAILY) |\n-\t systemd_timer_delete_timer_file(SCHEDULE_WEEKLY) |\n-\t systemd_timer_delete_service_template();\n-}\n-\n-static int systemd_timer_delete_units(void)\n-{\n-\tint minute = get_random_minute();\n-\t/* Purposefully not short-circuited to make sure all are called. */\n-\treturn systemd_timer_enable_unit(0, SCHEDULE_HOURLY, minute) |\n-\t systemd_timer_enable_unit(0, SCHEDULE_DAILY, minute) |\n-\t systemd_timer_enable_unit(0, SCHEDULE_WEEKLY, minute) |\n-\t systemd_timer_delete_unit_files();\n-}\n-\n-static int systemd_timer_setup_units(void)\n-{\n-\tint minute = get_random_minute();\n-\tconst char *exec_path = git_exec_path();\n-\n-\tint ret = systemd_timer_write_service_template(exec_path) ||\n-\t\t systemd_timer_enable_unit(1, SCHEDULE_HOURLY, minute) ||\n-\t\t systemd_timer_enable_unit(1, SCHEDULE_DAILY, minute) ||\n-\t\t systemd_timer_enable_unit(1, SCHEDULE_WEEKLY, minute);\n-\n-\tif (ret)\n-\t\tsystemd_timer_delete_units();\n-\telse\n-\t\tsystemd_timer_delete_stale_timer_templates();\n-\n-\treturn ret;\n }\n \n static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED)\n {\n-\tif (run_maintenance)\n-\t\treturn systemd_timer_setup_units();\n-\telse\n-\t\treturn systemd_timer_delete_units();\n+\t/*\n+\t * A previous version of Git wrote the units in the user configuration\n+\t * directory. Clean these up, if they exist.\n+\t */\n+\tsystemd_delete_user_unit(\"git-maintenance@hourly.timer\");\n+\tsystemd_delete_user_unit(\"git-maintenance@daily.timer\");\n+\tsystemd_delete_user_unit(\"git-maintenance@weekly.timer\");\n+\tsystemd_delete_user_unit(\"git-maintenance@.timer\");\n+\tsystemd_delete_user_unit(\"git-maintenance@.service\");\n+\treturn systemd_set_units_state(run_maintenance);\n }\n \n enum scheduler {\n-- \n2.44.0\n\n"},{"id":"490874","messageId":"20240318153257.27451-6-mg@max.gautier.name","threadId":"61145","inReplyTo":"20240318153257.27451-1-mg@max.gautier.name","subject":"[RFC PATCH 5/5] DON'T APPLY YET: maintenance: remove cleanup code","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-18T15:31:19Z","receivedAt":"2024-03-18T15:33:20Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"This removes code to remove old git-maintenance systemd timer and\nservice user units which were written in $XDG_CONFIG_HOME by git\nprevious versions.\n\nSigned-off-by: Max Gautier \n---\n builtin/gc.c | 54 +++-------------------------------------------------\n 1 file changed, 3 insertions(+), 51 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 981db8e297..6ac184eaf5 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -2303,12 +2303,7 @@ static int is_systemd_timer_available(void)\n \treturn real_is_systemd_timer_available();\n }\n \n-static char *xdg_config_home_systemd(const char *filename)\n-{\n-\treturn xdg_config_home_for(\"systemd/user\", filename);\n-}\n-\n-static int systemd_set_units_state(int enable)\n+static int systemd_set_units_state(int run_maintenance, int fd UNUSED)\n {\n \tconst char *cmd = \"systemctl\";\n \tstruct child_process child = CHILD_PROCESS_INIT;\n@@ -2317,7 +2312,7 @@ static int systemd_set_units_state(int enable)\n \tstrvec_split(&child.args, cmd);\n \n \tstrvec_pushl(&child.args, \"--user\", \"--force\", \"--now\",\n-\t\t\tenable ? \"enable\" : \"disable\",\n+\t\t\trun_maintenance ? \"enable\" : \"disable\",\n \t\t\t\"git-maintenance@hourly.timer\",\n \t\t\t\"git-maintenance@daily.timer\",\n \t\t\t\"git-maintenance@weekly.timer\", NULL);\n@@ -2334,49 +2329,6 @@ static int systemd_set_units_state(int enable)\n \treturn 0;\n }\n \n-static void systemd_delete_user_unit(char const *unit)\n-{\n-\tchar const\tfile_start_stale[] =\t\"# This file was created and is\"\n-\t\t\t\t\t\t\" maintained by Git.\";\n-\tchar\t\tfile_start_user[sizeof(file_start_stale)] = {'\\0'};\n-\n-\tchar *filename = xdg_config_home_systemd(unit);\n-\tint handle = open(filename, O_RDONLY);\n-\n-\t/*\n-\t** Check this is actually our file and we're not removing a legitimate\n-\t** user override.\n-\t*/\n-\tif (handle == -1 && !is_missing_file_error(errno))\n-\t\twarning(_(\"failed to delete '%s'\"), filename);\n-\telse {\n-\t\tread(handle, file_start_user, sizeof(file_start_stale) - 1);\n-\t\tclose(handle);\n-\t\tif (strcmp(file_start_stale, file_start_user) == 0) {\n-\t\t\tif (unlink(filename) == 0)\n-\t\t\t\twarning(_(\"deleted stale unit file '%s'\"), filename);\n-\t\t\telse if (!is_missing_file_error(errno))\n-\t\t\t\twarning(_(\"failed to delete '%s'\"), filename);\n-\t\t}\n-\t}\n-\n-\tfree(filename);\n-}\n-\n-static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED)\n-{\n-\t/*\n-\t * A previous version of Git wrote the units in the user configuration\n-\t * directory. Clean these up, if they exist.\n-\t */\n-\tsystemd_delete_user_unit(\"git-maintenance@hourly.timer\");\n-\tsystemd_delete_user_unit(\"git-maintenance@daily.timer\");\n-\tsystemd_delete_user_unit(\"git-maintenance@weekly.timer\");\n-\tsystemd_delete_user_unit(\"git-maintenance@.timer\");\n-\tsystemd_delete_user_unit(\"git-maintenance@.service\");\n-\treturn systemd_set_units_state(run_maintenance);\n-}\n-\n enum scheduler {\n \tSCHEDULER_INVALID = -1,\n \tSCHEDULER_AUTO,\n@@ -2399,7 +2351,7 @@ static const struct {\n \t[SCHEDULER_SYSTEMD] = {\n \t\t.name = \"systemctl\",\n \t\t.is_available = is_systemd_timer_available,\n-\t\t.update_schedule = systemd_timer_update_schedule,\n+\t\t.update_schedule = systemd_set_units_state,\n \t},\n \t[SCHEDULER_LAUNCHCTL] = {\n \t\t.name = \"launchctl\",\n-- \n2.44.0\n\n"},{"id":"490930","messageId":"ZfmAfIErHRZVbd49@framework","threadId":"61145","inReplyTo":"20240318153257.27451-4-mg@max.gautier.name","subject":"Re: [RFC PATCH 3/5] maintenance: use packaged systemd units","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-19T12:09:32Z","receivedAt":"2024-03-19T12:09:52Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"I'm working on updating the test in t7900-maintenance.sh, but I might be\nmissing something here:\n\n>test_expect_success 'start and stop Linux/systemd maintenance' '\n> write_script print-args <<-\\EOF &&\n> printf \"%s\\n\" \"$*\" >>args\n> EOF\n> \n> XDG_CONFIG_HOME=\"$PWD\" &&\n> export XDG_CONFIG_HOME &&\n> rm -f args &&\n> GIT_TEST_MAINT_SCHEDULER=\"systemctl:./print-args\" git maintenance start --scheduler=systemd-timer &&\n\nDo I understand correctly that this means we're not actually running\nsystemctl here, just printing the arguments to our file ?\n\n>\t# start registers the repo\n>\tgit config --get --global --fixed-value maintenance.repo \"$(pwd)\" &&\n>\n>\tfor schedule in hourly daily weekly\n>\tdo\n>\t\ttest_path_is_file \"systemd/user/git-maintenance@$schedule.timer\" || return 1\n>\tdone &&\n>\ttest_path_is_file \"systemd/user/git-maintenance@.service\" &&\n>\n>\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@hourly.service\" &&\n>\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@daily.service\" &&\n>\ttest_systemd_analyze_verify \"systemd/user/git-maintenance@weekly.service\" &&\n>\n>\tprintf -- \"--user enable --now git-maintenance@%s.timer\\n\" hourly daily weekly >expect &&\n>\ttest_cmp expect args &&\n>\n>\trm -f args &&\n>\tGIT_TEST_MAINT_SCHEDULER=\"systemctl:./print-args\" git maintenance stop &&\n>\n>\t# stop does not unregister the repo\n>\tgit config --get --global --fixed-value maintenance.repo \"$(pwd)\" &&\n>\n>\tfor schedule in hourly daily weekly\n>\tdo\n>\t\ttest_path_is_missing \"systemd/user/git-maintenance@$schedule.timer\" || return 1\n>\tdone &&\n>\ttest_path_is_missing \"systemd/user/git-maintenance@.service\" &&\n>\n>\tprintf -- \"--user disable --now git-maintenance@%s.timer\\n\" hourly daily weekly >expect &&\n>\ttest_cmp expect args\n\nThe rest of the systemd tests only check that the service file are in\nXDG_CONFIG_HOME, which should not be the case anymore.\n\nHowever, the test does not actually check we have enabled and started\nthe timers as it is , right ?\n\nShould I add that ? I'm not sure how, because it does not seem like the\ntests run in a isolated env, so it would mess with the systemd user\nmanager of the developper running the tests...\n\nRegarding systemd-analyze verify, do the tests have access to the source\ndirectory in a special way, or is using '../..' enough ?\n\nThanks\n\n-- \nMax Gautier\n"},{"id":"490937","messageId":"CAPig+cSWLoRdTgrrU2SBswnKr82L_BPCKtaP6atMyZVDAU=hpw@mail.gmail.com","threadId":"61145","inReplyTo":"ZfmAfIErHRZVbd49@framework","subject":"Re: [RFC PATCH 3/5] maintenance: use packaged systemd units","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-19T17:17:27Z","receivedAt":"2024-03-19T17:17:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 19, 2024 at 8:10 AM Max Gautier wrote:\n> I'm working on updating the test in t7900-maintenance.sh, but I might be\n> missing something here:\n>\n> >test_expect_success 'start and stop Linux/systemd maintenance' '\n> > write_script print-args <<-\\EOF &&\n> > printf \"%s\\n\" \"$*\" >>args\n> > EOF\n> >\n> > XDG_CONFIG_HOME=\"$PWD\" &&\n> > export XDG_CONFIG_HOME &&\n> > rm -f args &&\n> > GIT_TEST_MAINT_SCHEDULER=\"systemctl:./print-args\" git maintenance start --scheduler=systemd-timer &&\n>\n> Do I understand correctly that this means we're not actually running\n> systemctl here, just printing the arguments to our file ?\n\nThat's correct. The purpose of GIT_TEST_MAINT_SCHEDULER is twofold.\n\nThe primary purpose is to test as much as possible without actually\nmucking with the user's real scheduler-related configuration (whether\nit be systemd, cron, launchctl, etc.). This means that we want to\nverify that the expected files are created or removed by\ngit-maintenance, that they are well-formed, and that git-maintenance\nis invoking the correct platform-specific scheduler-related command\nwith correct arguments (without actually invoking that command and\nmessing up the user's personal configuration).\n\nThe secondary purpose is to allow these otherwise platform-specific\ntests to run on any platform. This is possible since, as noted above,\nwe're not actually running the platform-specific scheduler-related\ncommand, but instead only capturing the command and arguments that\nwould have been applied had git-maintenace been run \"for real\" outside\nof the test framework.\n\n> > for schedule in hourly daily weekly\n> > do\n> > test_path_is_missing \"systemd/user/git-maintenance@$schedule.timer\" || return 1\n> > done &&\n> > test_path_is_missing \"systemd/user/git-maintenance@.service\" &&\n> >\n> > printf -- \"--user disable --now git-maintenance@%s.timer\\n\" hourly daily weekly >expect &&\n> > test_cmp expect args\n>\n> The rest of the systemd tests only check that the service file are in\n> XDG_CONFIG_HOME, which should not be the case anymore.\n>\n> However, the test does not actually check we have enabled and started\n> the timers as it is , right ?\n\nCorrect. As noted above, we don't want to muck with the user's real\nconfiguration, and we certainly don't want the system-specific\nscheduler to actually kick off some command we're testing in the\nuser's account while the test script is running.\n\n> Should I add that ? I'm not sure how, because it does not seem like the\n> tests run in a isolated env, so it would mess with the systemd user\n> manager of the developper running the tests...\n\nNo you don't want to add that since, as you state and as stated above,\nit would muck with the user's own configuration which would be\nundesirable.\n\n> Regarding systemd-analyze verify, do the tests have access to the source\n> directory in a special way, or is using '../..' enough ?\n\nYou can't assume that the source directory is available at \"../..\"\nsince the --root option (see t/README) allows the root of the tests\nworking-tree to reside outside of the project directory.\n\nYou may be able to use \"$TEST_DIRECTORY/..\" to reference files in the\nsource tree, though the documentation in t/test-lib.sh doesn't seem to\nstate explicitly that this is intended or supported use. However, a\nfew existing tests (t1021, t3000, t4023) already access files in the\nsource tree in this fashion, so there is precedent.\n"},{"id":"490940","messageId":"xmqq8r2eqe2w.fsf@gitster.g","threadId":"61145","inReplyTo":"CAPig+cSWLoRdTgrrU2SBswnKr82L_BPCKtaP6atMyZVDAU=hpw@mail.gmail.com","subject":"Re: [RFC PATCH 3/5] maintenance: use packaged systemd units","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-19T18:19:35Z","receivedAt":"2024-03-19T18:19:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> You may be able to use \"$TEST_DIRECTORY/..\" to reference files in the\n> source tree, though the documentation in t/test-lib.sh doesn't seem to\n> state explicitly that this is intended or supported use. However, a\n> few existing tests (t1021, t3000, t4023) already access files in the\n> source tree in this fashion, so there is precedent.\n\nI think the following from t/test-lib.sh should give us sufficient\nassurance.\n\n\t# The TEST_DIRECTORY will always be the path to the \"t\"\n\t# directory in the git.git checkout. This is overridden by\n\t# e.g. t/lib-subtest.sh, but only because its $(pwd) is\n\t# different. Those tests still set \"$TEST_DIRECTORY\" to the\n\t# same path.\n\nEverything you said in your review is correct. Thanks.\n"},{"id":"490950","messageId":"BA7F27C1-EFE2-47CD-B892-A3F4262394CB@max.gautier.name","threadId":"61145","inReplyTo":"CAPig+cSWLoRdTgrrU2SBswnKr82L_BPCKtaP6atMyZVDAU=hpw@mail.gmail.com","subject":"Re: [RFC PATCH 3/5] maintenance: use packaged systemd units","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-19T19:38:49Z","receivedAt":"2024-03-19T19:38:55Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"\nLe 19 mars 2024 18:17:27 GMT+01:00, Eric Sunshine a écrit :\n>On Tue, Mar 19, 2024 at 8:10 AM Max Gautier wrote:\n>> I'm working on updating the test in t7900-maintenance.sh, but I might be\n>> missing something here:\n>>\n>> >test_expect_success 'start and stop Linux/systemd maintenance' '\n>> > write_script print-args <<-\\EOF &&\n>> > printf \"%s\\n\" \"$*\" >>args\n>> > EOF\n>> >\n>> > XDG_CONFIG_HOME=\"$PWD\" &&\n>> > export XDG_CONFIG_HOME &&\n>> > rm -f args &&\n>> > GIT_TEST_MAINT_SCHEDULER=\"systemctl:./print-args\" git maintenance start --scheduler=systemd-timer &&\n>>\n>> Do I understand correctly that this means we're not actually running\n>> systemctl here, just printing the arguments to our file ?\n>\n>That's correct. The purpose of GIT_TEST_MAINT_SCHEDULER is twofold.\n>\n>The primary purpose is to test as much as possible without actually\n>mucking with the user's real scheduler-related configuration (whether\n>it be systemd, cron, launchctl, etc.). This means that we want to\n>verify that the expected files are created or removed by\n>git-maintenance, that they are well-formed, and that git-maintenance\n>is invoking the correct platform-specific scheduler-related command\n>with correct arguments (without actually invoking that command and\n>messing up the user's personal configuration).\n>\n>The secondary purpose is to allow these otherwise platform-specific\n>tests to run on any platform. This is possible since, as noted above,\n>we're not actually running the platform-specific scheduler-related\n>command, but instead only capturing the command and arguments that\n>would have been applied had git-maintenace been run \"for real\" outside\n>of the test framework.\n\nOk thanks, now I see why it's done this way.\n\n>\n>> > for schedule in hourly daily weekly\n>> > do\n>> > test_path_is_missing \"systemd/user/git-maintenance@$schedule.timer\" || return 1\n>> > done &&\n>> > test_path_is_missing \"systemd/user/git-maintenance@.service\" &&\n>> >\n>> > printf -- \"--user disable --now git-maintenance@%s.timer\\n\" hourly daily weekly >expect &&\n>> > test_cmp expect args\n>>\n>> The rest of the systemd tests only check that the service file are in\n>> XDG_CONFIG_HOME, which should not be the case anymore.\n>>\n>> However, the test does not actually check we have enabled and started\n>> the timers as it is , right ?\n>\n>Correct. As noted above, we don't want to muck with the user's real\n>configuration, and we certainly don't want the system-specific\n>scheduler to actually kick off some command we're testing in the\n>user's account while the test script is running.\n>\n>> Should I add that ? I'm not sure how, because it does not seem like the\n>> tests run in a isolated env, so it would mess with the systemd user\n>> manager of the developper running the tests...\n>\n>No you don't want to add that since, as you state and as stated above,\n>it would muck with the user's own configuration which would be\n>undesirable.\n>\n\nThat makes things easier then :)\n\n>> Regarding systemd-analyze verify, do the tests have access to the source\n>> directory in a special way, or is using '../..' enough ?\n>\n>You can't assume that the source directory is available at \"../..\"\n>since the --root option (see t/README) allows the root of the tests\n>working-tree to reside outside of the project directory.\n>\n>You may be able to use \"$TEST_DIRECTORY/..\" to reference files in the\n>source tree, though the documentation in t/test-lib.sh doesn't seem to\n>state explicitly that this is intended or supported use. However, a\n>few existing tests (t1021, t3000, t4023) already access files in the\n>source tree in this fashion, so there is precedent.\n\n\nI'll look into those. Thanks for all the info !\n\n-- \nMax Gautier\n"},{"id":"491123","messageId":"ZfwqCv889UdI0mU6@tanuki","threadId":"61145","inReplyTo":"20240318153257.27451-2-mg@max.gautier.name","subject":"Re: [RFC PATCH 1/5] maintenance: package systemd units","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-21T12:37:30Z","receivedAt":"2024-03-21T12:37:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Mar 18, 2024 at 04:31:15PM +0100, Max Gautier wrote:\n\nIt would be great to document _why_ we want to package the systemd units\nalongside with Git.\n\n> Signed-off-by: Max Gautier \n> ---\n> Makefile | 4 ++++\n> systemd/user/git-maintenance@.service | 16 ++++++++++++++++\n> systemd/user/git-maintenance@.timer | 9 +++++++++\n> 3 files changed, 29 insertions(+)\n> create mode 100644 systemd/user/git-maintenance@.service\n> create mode 100644 systemd/user/git-maintenance@.timer\n> \n> diff --git a/Makefile b/Makefile\n> index 4e255c81f2..276b4373c6 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -619,6 +619,7 @@ htmldir = $(prefix)/share/doc/git-doc\n> ETC_GITCONFIG = $(sysconfdir)/gitconfig\n> ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes\n> lib = lib\n> +libdir = $(prefix)/lib\n> # DESTDIR =\n> pathsep = :\n> \n> @@ -1328,6 +1329,8 @@ BUILTIN_OBJS += builtin/verify-tag.o\n> BUILTIN_OBJS += builtin/worktree.o\n> BUILTIN_OBJS += builtin/write-tree.o\n> \n> +SYSTEMD_USER_UNITS := $(wildcard systemd/user/*)\n> +\n> # THIRD_PARTY_SOURCES is a list of patterns compatible with the\n> # $(filter) and $(filter-out) family of functions. They specify source\n> # files which are taken from some third-party source where we want to be\n> @@ -3469,6 +3472,7 @@ install: all\n> \t$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'\n> \t$(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)'\n> \t$(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)'\n> +\t$(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(libdir)/systemd/user' $(SYSTEMD_USER_UNITS)\n\nI wonder whether we want to unconditionally install those units. Many of\nthe platforms that we support don't even have systemd available, so\ncertainly it wouldn't make any sense to install it on those platforms.\n\nAssuming that this is something we want in the first place I thus think\nthat we should at least make this conditional and add some platform\nspecific quirk to \"config.mak.uname\".\n\n> ifdef MSVC\n> \t# We DO NOT install the individual foo.o.pdb files because they\n> diff --git a/systemd/user/git-maintenance@.service b/systemd/user/git-maintenance@.service\n> new file mode 100644\n> index 0000000000..87ac0c86e6\n> --- /dev/null\n> +++ b/systemd/user/git-maintenance@.service\n> @@ -0,0 +1,16 @@\n> +[Unit]\n> +Description=Optimize Git repositories data\n> +\n> +[Service]\n> +Type=oneshot\n> +ExecStart=git for-each-repo --config=maintenance.repo \\\n> + 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\nCurious, but how did you arrive at these particular restrictions for the\nunit? Might be something to explain in the commit message, as well.\n\nPatrick\n\n> diff --git a/systemd/user/git-maintenance@.timer b/systemd/user/git-maintenance@.timer\n> new file mode 100644\n> index 0000000000..40fbc77a62\n> --- /dev/null\n> +++ b/systemd/user/git-maintenance@.timer\n> @@ -0,0 +1,9 @@\n> +[Unit]\n> +Description=Optimize Git repositories data\n> +\n> +[Timer]\n> +OnCalendar=%i\n> +Persistent=true\n> +\n> +[Install]\n> +WantedBy=timers.target\n> -- \n> 2.44.0\n> \n> \n"},{"id":"491124","messageId":"ZfwqD7z6S8Olc5xa@tanuki","threadId":"61145","inReplyTo":"20240318153257.27451-3-mg@max.gautier.name","subject":"Re: [RFC PATCH 2/5] maintenance: add fixed random delay to systemd timers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-21T12:37:35Z","receivedAt":"2024-03-21T12:37:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Mar 18, 2024 at 04:31:16PM +0100, Max Gautier wrote:\n> Ensures that:\n> - git maintenance timers have a fixed time interval between execution.\n> - the three timers are not executed at the same time.\n\nCommit messages are typically structured so that you first explain what\nthe problem is that you're trying to solve, and then you explain how you\nsolve it. Your commit message is missing the first part.\n\n> This is intended to implement an alternative to the two followings\n> commits:\n> c97ec0378b (maintenance: fix systemd schedule overlaps, 2023-08-10)\n> daa787010c (maintenance: use random minute in systemd scheduler, 2023-08-10)\n> \n> Instead of manually adding a specific minute (which is reset on each\n> invocation of `git maintenance start`), we use systemd timers\n> RandomizedDelaySec and FixedRandomDelay functionalities.\n\nI think it would help to not list commits, but put the commit references\nin a paragraph. Something in the spirit of \"In commit c97ec0378b we\nalready tried to address this issue in such and such a way, but that is\nquite limiting due to that and that. Similarly, in commit daa787010c..\".\n\n> From man systemd.timer:\n> >FixedRandomDelay=\n> > Takes a boolean argument. When enabled, the randomized offset\n> > specified by RandomizedDelaySec= is reused for all firings of the\n> > same timer. For a given timer unit, **the offset depends on the\n> > machine ID, user identifier and timer name**, which means that it is\n> > stable between restarts of the manager. This effectively creates a\n> > fixed offset for an individual timer, reducing the jitter in\n> > firings of this timer, while still avoiding firing at the same time\n> > as other similarly configured timers.\n> \n> -> which is exactly the use case for git-maintenance timers.\n> \n> Signed-off-by: Max Gautier \n> ---\n> systemd/user/git-maintenance@.service | 1 +\n> systemd/user/git-maintenance@.timer | 3 +++\n> 2 files changed, 4 insertions(+)\n> \n> diff --git a/systemd/user/git-maintenance@.service b/systemd/user/git-maintenance@.service\n> index 87ac0c86e6..f949e1a217 100644\n> --- a/systemd/user/git-maintenance@.service\n> +++ b/systemd/user/git-maintenance@.service\n> @@ -1,5 +1,6 @@\n> [Unit]\n> Description=Optimize Git repositories data\n> +Documentation=man:git-maintenance(1)\n\nThis change feels unrelated and should likely go into the first commit.\n\n> \n> [Service]\n> Type=oneshot\n> diff --git a/systemd/user/git-maintenance@.timer b/systemd/user/git-maintenance@.timer\n> index 40fbc77a62..667c5998ba 100644\n> --- a/systemd/user/git-maintenance@.timer\n> +++ b/systemd/user/git-maintenance@.timer\n> @@ -1,9 +1,12 @@\n> [Unit]\n> Description=Optimize Git repositories data\n> +Documentation=man:git-maintenance(1)\n\nSame.\n\nPatrick\n\n> [Timer]\n> OnCalendar=%i\n> Persistent=true\n> +RandomizedDelaySec=1800\n> +FixedRandomDelay=true\n> \n> [Install]\n> WantedBy=timers.target\n> -- \n> 2.44.0\n> \n> \n"},{"id":"491125","messageId":"ZfwqFU_b3MqLCZNR@tanuki","threadId":"61145","inReplyTo":"20240318153257.27451-4-mg@max.gautier.name","subject":"Re: [RFC PATCH 3/5] maintenance: use packaged systemd units","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-21T12:37:41Z","receivedAt":"2024-03-21T12:37:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Mar 18, 2024 at 04:31:17PM +0100, Max Gautier wrote:\n> - Remove the code writing the units\n> - Simplify calling systemctl\n> - systemctl does not error out when disabling units which aren't\n> enabled, nor when enabling already enabled units\n> - call systemctl only once, with all the units.\n> - Add clean-up code for leftover units in $XDG_CONFIG_HOME created by\n> previous versions of git, taking care of preserving actual user\n> override (by checking the start of the file).\n\nWe very much try to avoid bulleted-list-style commit messages. Once you\nhave a list of changes it very often indicates that there are multiple\nsemi-related things going on in the same commit. At that point we prefer\nto split the commit up into multiple commits so that each of the bullet\nitems can be explained separately.\n\nAlso, as mentioned before, commit messages should explain what the\nproblem is and how that problem is solved. \n\nPatrick\n\n> Signed-off-by: Max Gautier \n> ---\n> builtin/gc.c | 293 ++++++++-------------------------------------------\n> 1 file changed, 45 insertions(+), 248 deletions(-)\n> \n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index cb80ced6cb..981db8e297 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -2308,276 +2308,73 @@ static char *xdg_config_home_systemd(const char *filename)\n> \treturn xdg_config_home_for(\"systemd/user\", filename);\n> }\n> \n> -#define SYSTEMD_UNIT_FORMAT \"git-maintenance@%s.%s\"\n> -\n> -static int systemd_timer_delete_timer_file(enum schedule_priority priority)\n> -{\n> -\tint ret = 0;\n> -\tconst char *frequency = get_frequency(priority);\n> -\tchar *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, \"timer\");\n> -\tchar *filename = xdg_config_home_systemd(local_timer_name);\n> -\n> -\tif (unlink(filename) && !is_missing_file_error(errno))\n> -\t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n> -\n> -\tfree(filename);\n> -\tfree(local_timer_name);\n> -\treturn ret;\n> -}\n> -\n> -static int systemd_timer_delete_service_template(void)\n> -{\n> -\tint ret = 0;\n> -\tchar *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, \"\", \"service\");\n> -\tchar *filename = xdg_config_home_systemd(local_service_name);\n> -\tif (unlink(filename) && !is_missing_file_error(errno))\n> -\t\tret = error_errno(_(\"failed to delete '%s'\"), filename);\n> -\n> -\tfree(filename);\n> -\tfree(local_service_name);\n> -\treturn ret;\n> -}\n> -\n> -/*\n> - * Write the schedule information into a git-maintenance@.timer\n> - * file using a custom minute. This timer file cannot use the templating\n> - * system, so we generate a specific file for each.\n> - */\n> -static int systemd_timer_write_timer_file(enum schedule_priority schedule,\n> -\t\t\t\t\t int minute)\n> -{\n> -\tint res = -1;\n> -\tchar *filename;\n> -\tFILE *file;\n> -\tconst char *unit;\n> -\tchar *schedule_pattern = NULL;\n> -\tconst char *frequency = get_frequency(schedule);\n> -\tchar *local_timer_name = xstrfmt(SYSTEMD_UNIT_FORMAT, frequency, \"timer\");\n> -\n> -\tfilename = xdg_config_home_systemd(local_timer_name);\n> -\n> -\tif (safe_create_leading_directories(filename)) {\n> -\t\terror(_(\"failed to create directories for '%s'\"), filename);\n> -\t\tgoto error;\n> -\t}\n> -\tfile = fopen_or_warn(filename, \"w\");\n> -\tif (!file)\n> -\t\tgoto error;\n> -\n> -\tswitch (schedule) {\n> -\tcase SCHEDULE_HOURLY:\n> -\t\tschedule_pattern = xstrfmt(\"*-*-* 1..23:%02d:00\", minute);\n> -\t\tbreak;\n> -\n> -\tcase SCHEDULE_DAILY:\n> -\t\tschedule_pattern = xstrfmt(\"Tue..Sun *-*-* 0:%02d:00\", minute);\n> -\t\tbreak;\n> -\n> -\tcase SCHEDULE_WEEKLY:\n> -\t\tschedule_pattern = xstrfmt(\"Mon 0:%02d:00\", minute);\n> -\t\tbreak;\n> -\n> -\tdefault:\n> -\t\tBUG(\"Unhandled schedule_priority\");\n> -\t}\n> -\n> -\tunit = \"# This file was created and is maintained by Git.\\n\"\n> -\t \"# Any edits made in this file might be replaced in the future\\n\"\n> -\t \"# by a Git command.\\n\"\n> -\t \"\\n\"\n> -\t \"[Unit]\\n\"\n> -\t \"Description=Optimize Git repositories data\\n\"\n> -\t \"\\n\"\n> -\t \"[Timer]\\n\"\n> -\t \"OnCalendar=%s\\n\"\n> -\t \"Persistent=true\\n\"\n> -\t \"\\n\"\n> -\t \"[Install]\\n\"\n> -\t \"WantedBy=timers.target\\n\";\n> -\tif (fprintf(file, unit, schedule_pattern) < 0) {\n> -\t\terror(_(\"failed to write to '%s'\"), filename);\n> -\t\tfclose(file);\n> -\t\tgoto error;\n> -\t}\n> -\tif (fclose(file) == EOF) {\n> -\t\terror_errno(_(\"failed to flush '%s'\"), filename);\n> -\t\tgoto error;\n> -\t}\n> -\n> -\tres = 0;\n> -\n> -error:\n> -\tfree(schedule_pattern);\n> -\tfree(local_timer_name);\n> -\tfree(filename);\n> -\treturn res;\n> -}\n> -\n> -/*\n> - * No matter the schedule, we use the same service and can make use of the\n> - * templating system. When installing git-maintenance@.timer,\n> - * systemd will notice that git-maintenance@.service exists as a template\n> - * and will use this file and insert the into the template at\n> - * the position of \"%i\".\n> - */\n> -static int systemd_timer_write_service_template(const char *exec_path)\n> -{\n> -\tint res = -1;\n> -\tchar *filename;\n> -\tFILE *file;\n> -\tconst char *unit;\n> -\tchar *local_service_name = xstrfmt(SYSTEMD_UNIT_FORMAT, \"\", \"service\");\n> -\n> -\tfilename = xdg_config_home_systemd(local_service_name);\n> -\tif (safe_create_leading_directories(filename)) {\n> -\t\terror(_(\"failed to create directories for '%s'\"), filename);\n> -\t\tgoto error;\n> -\t}\n> -\tfile = fopen_or_warn(filename, \"w\");\n> -\tif (!file)\n> -\t\tgoto error;\n> -\n> -\tunit = \"# This file was created and is maintained by Git.\\n\"\n> -\t \"# Any edits made in this file might be replaced in the future\\n\"\n> -\t \"# by a Git command.\\n\"\n> -\t \"\\n\"\n> -\t \"[Unit]\\n\"\n> -\t \"Description=Optimize Git repositories data\\n\"\n> -\t \"\\n\"\n> -\t \"[Service]\\n\"\n> -\t \"Type=oneshot\\n\"\n> -\t \"ExecStart=\\\"%s/git\\\" --exec-path=\\\"%s\\\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\\n\"\n> -\t \"LockPersonality=yes\\n\"\n> -\t \"MemoryDenyWriteExecute=yes\\n\"\n> -\t \"NoNewPrivileges=yes\\n\"\n> -\t \"RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 AF_VSOCK\\n\"\n> -\t \"RestrictNamespaces=yes\\n\"\n> -\t \"RestrictRealtime=yes\\n\"\n> -\t \"RestrictSUIDSGID=yes\\n\"\n> -\t \"SystemCallArchitectures=native\\n\"\n> -\t \"SystemCallFilter=@system-service\\n\";\n> -\tif (fprintf(file, unit, exec_path, exec_path) < 0) {\n> -\t\terror(_(\"failed to write to '%s'\"), filename);\n> -\t\tfclose(file);\n> -\t\tgoto error;\n> -\t}\n> -\tif (fclose(file) == EOF) {\n> -\t\terror_errno(_(\"failed to flush '%s'\"), filename);\n> -\t\tgoto error;\n> -\t}\n> -\n> -\tres = 0;\n> -\n> -error:\n> -\tfree(local_service_name);\n> -\tfree(filename);\n> -\treturn res;\n> -}\n> -\n> -static int systemd_timer_enable_unit(int enable,\n> -\t\t\t\t enum schedule_priority schedule,\n> -\t\t\t\t int minute)\n> +static int systemd_set_units_state(int enable)\n> {\n> \tconst char *cmd = \"systemctl\";\n> \tstruct child_process child = CHILD_PROCESS_INIT;\n> -\tconst char *frequency = get_frequency(schedule);\n> -\n> -\t/*\n> -\t * Disabling the systemd unit while it is already disabled makes\n> -\t * systemctl print an error.\n> -\t * Let's ignore it since it means we already are in the expected state:\n> -\t * the unit is disabled.\n> -\t *\n> -\t * On the other hand, enabling a systemd unit which is already enabled\n> -\t * produces no error.\n> -\t */\n> -\tif (!enable)\n> -\t\tchild.no_stderr = 1;\n> -\telse if (systemd_timer_write_timer_file(schedule, minute))\n> -\t\treturn -1;\n> \n> \tget_schedule_cmd(&cmd, NULL);\n> \tstrvec_split(&child.args, cmd);\n> -\tstrvec_pushl(&child.args, \"--user\", enable ? \"enable\" : \"disable\",\n> -\t\t \"--now\", NULL);\n> -\tstrvec_pushf(&child.args, SYSTEMD_UNIT_FORMAT, frequency, \"timer\");\n> +\n> +\tstrvec_pushl(&child.args, \"--user\", \"--force\", \"--now\",\n> +\t\t\tenable ? \"enable\" : \"disable\",\n> +\t\t\t\"git-maintenance@hourly.timer\",\n> +\t\t\t\"git-maintenance@daily.timer\",\n> +\t\t\t\"git-maintenance@weekly.timer\", NULL);\n> +\t/*\n> +\t** --force override existing conflicting symlinks\n> +\t** We need it because the units have changed location (~/.config ->\n> +\t** /usr/lib)\n> +\t*/\n> \n> \tif (start_command(&child))\n> \t\treturn error(_(\"failed to start systemctl\"));\n> \tif (finish_command(&child))\n> -\t\t/*\n> -\t\t * Disabling an already disabled systemd unit makes\n> -\t\t * systemctl fail.\n> -\t\t * Let's ignore this failure.\n> -\t\t *\n> -\t\t * Enabling an enabled systemd unit doesn't fail.\n> -\t\t */\n> -\t\tif (enable)\n> -\t\t\treturn error(_(\"failed to run systemctl\"));\n> +\t\treturn error(_(\"failed to run systemctl\"));\n> \treturn 0;\n> }\n> \n> -/*\n> - * A previous version of Git wrote the timer units as template files.\n> - * Clean these up, if they exist.\n> - */\n> -static void systemd_timer_delete_stale_timer_templates(void)\n> +static void systemd_delete_user_unit(char const *unit)\n> {\n> -\tchar *timer_template_name = xstrfmt(SYSTEMD_UNIT_FORMAT, \"\", \"timer\");\n> -\tchar *filename = xdg_config_home_systemd(timer_template_name);\n> +\tchar const\tfile_start_stale[] =\t\"# This file was created and is\"\n> +\t\t\t\t\t\t\" maintained by Git.\";\n> +\tchar\t\tfile_start_user[sizeof(file_start_stale)] = {'\\0'};\n> +\n> +\tchar *filename = xdg_config_home_systemd(unit);\n> +\tint handle = open(filename, O_RDONLY);\n> \n> -\tif (unlink(filename) && !is_missing_file_error(errno))\n> +\t/*\n> +\t** Check this is actually our file and we're not removing a legitimate\n> +\t** user override.\n> +\t*/\n> +\tif (handle == -1 && !is_missing_file_error(errno))\n> \t\twarning(_(\"failed to delete '%s'\"), filename);\n> +\telse {\n> +\t\tread(handle, file_start_user, sizeof(file_start_stale) - 1);\n> +\t\tclose(handle);\n> +\t\tif (strcmp(file_start_stale, file_start_user) == 0) {\n> +\t\t\tif (unlink(filename) == 0)\n> +\t\t\t\twarning(_(\"deleted stale unit file '%s'\"), filename);\n> +\t\t\telse if (!is_missing_file_error(errno))\n> +\t\t\t\twarning(_(\"failed to delete '%s'\"), filename);\n> +\t\t}\n> +\t}\n> \n> \tfree(filename);\n> -\tfree(timer_template_name);\n> -}\n> -\n> -static int systemd_timer_delete_unit_files(void)\n> -{\n> -\tsystemd_timer_delete_stale_timer_templates();\n> -\n> -\t/* Purposefully not short-circuited to make sure all are called. */\n> -\treturn systemd_timer_delete_timer_file(SCHEDULE_HOURLY) |\n> -\t systemd_timer_delete_timer_file(SCHEDULE_DAILY) |\n> -\t systemd_timer_delete_timer_file(SCHEDULE_WEEKLY) |\n> -\t systemd_timer_delete_service_template();\n> -}\n> -\n> -static int systemd_timer_delete_units(void)\n> -{\n> -\tint minute = get_random_minute();\n> -\t/* Purposefully not short-circuited to make sure all are called. */\n> -\treturn systemd_timer_enable_unit(0, SCHEDULE_HOURLY, minute) |\n> -\t systemd_timer_enable_unit(0, SCHEDULE_DAILY, minute) |\n> -\t systemd_timer_enable_unit(0, SCHEDULE_WEEKLY, minute) |\n> -\t systemd_timer_delete_unit_files();\n> -}\n> -\n> -static int systemd_timer_setup_units(void)\n> -{\n> -\tint minute = get_random_minute();\n> -\tconst char *exec_path = git_exec_path();\n> -\n> -\tint ret = systemd_timer_write_service_template(exec_path) ||\n> -\t\t systemd_timer_enable_unit(1, SCHEDULE_HOURLY, minute) ||\n> -\t\t systemd_timer_enable_unit(1, SCHEDULE_DAILY, minute) ||\n> -\t\t systemd_timer_enable_unit(1, SCHEDULE_WEEKLY, minute);\n> -\n> -\tif (ret)\n> -\t\tsystemd_timer_delete_units();\n> -\telse\n> -\t\tsystemd_timer_delete_stale_timer_templates();\n> -\n> -\treturn ret;\n> }\n> \n> static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED)\n> {\n> -\tif (run_maintenance)\n> -\t\treturn systemd_timer_setup_units();\n> -\telse\n> -\t\treturn systemd_timer_delete_units();\n> +\t/*\n> +\t * A previous version of Git wrote the units in the user configuration\n> +\t * directory. Clean these up, if they exist.\n> +\t */\n> +\tsystemd_delete_user_unit(\"git-maintenance@hourly.timer\");\n> +\tsystemd_delete_user_unit(\"git-maintenance@daily.timer\");\n> +\tsystemd_delete_user_unit(\"git-maintenance@weekly.timer\");\n> +\tsystemd_delete_user_unit(\"git-maintenance@.timer\");\n> +\tsystemd_delete_user_unit(\"git-maintenance@.service\");\n> +\treturn systemd_set_units_state(run_maintenance);\n> }\n> \n> enum scheduler {\n> -- \n> 2.44.0\n> \n> \n"},{"id":"491126","messageId":"ZfwqIklp5GoC1RP-@tanuki","threadId":"61145","inReplyTo":"20240318153257.27451-5-mg@max.gautier.name","subject":"Re: [RFC PATCH 4/5] maintenance: update systemd scheduler docs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-21T12:37:54Z","receivedAt":"2024-03-21T12:38:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Mar 18, 2024 at 04:31:18PM +0100, Max Gautier wrote:\n\nThis commit is missing an explanation what exactly you are updating.\nWhile you mention that you update something, the reader now has to\nfind out by themselves what exactly you update by reading through the\nwhole commit diff.\n\nPatrick\n\n> Signed-off-by: Max Gautier \n> ---\n> Documentation/git-maintenance.txt | 33 +++++++++++++++----------------\n> 1 file changed, 16 insertions(+), 17 deletions(-)\n> \n> diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt\n> index 51d0f7e94b..6511c3f3f1 100644\n> --- a/Documentation/git-maintenance.txt\n> +++ b/Documentation/git-maintenance.txt\n> @@ -304,10 +304,9 @@ distributions, systemd timers are superseding it.\n> If user systemd timers are available, they will be used as a replacement\n> of `cron`.\n> \n> -In this case, `git maintenance start` will create user systemd timer units\n> -and start the timers. The current list of user-scheduled tasks can be found\n> -by running `systemctl --user list-timers`. The timers written by `git\n> -maintenance start` are similar to this:\n> +In this case, `git maintenance start` will enable user systemd timer units\n> +and start them. The current list of user-scheduled tasks can be found by\n> +running `systemctl --user list-timers`. These timers are similar to this:\n> \n> -----------------------------------------------------------------------\n> $ systemctl --user list-timers\n> @@ -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\n> 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\n> -----------------------------------------------------------------------\n> \n> -One timer is registered for each `--schedule=` option.\n> +One timer instance is enabled for each `--schedule=` option.\n> \n> -The definition of the systemd units can be inspected in the following files:\n> +The definition of the systemd units can be inspected this way:\n> \n> -----------------------------------------------------------------------\n> -~/.config/systemd/user/git-maintenance@.timer\n> -~/.config/systemd/user/git-maintenance@.service\n> -~/.config/systemd/user/timers.target.wants/git-maintenance@hourly.timer\n> -~/.config/systemd/user/timers.target.wants/git-maintenance@daily.timer\n> -~/.config/systemd/user/timers.target.wants/git-maintenance@weekly.timer\n> +$ systemctl cat --user git-maintenance@.timer\n> +$ systemctl cat --user git-maintenance@.service\n> -----------------------------------------------------------------------\n> \n> -`git maintenance start` will overwrite these files and start the timer\n> -again with `systemctl --user`, so any customization should be done by\n> -creating a drop-in file, i.e. a `.conf` suffixed file in the\n> -`~/.config/systemd/user/git-maintenance@.service.d` directory.\n> +Customization of the timer or service can be performed with the usual systemd\n> +tooling:\n> +-----------------------------------------------------------------------\n> +$ systemctl edit --user git-maintenance@.timer # all the timers\n> +$ systemctl edit --user git-maintenance@hourly.timer # the hourly timer\n> +$ systemctl edit --user git-maintenance@.service # all the services\n> +$ systemctl edit --user git-maintenance@hourly.service # the hourly run\n> +-----------------------------------------------------------------------\n> \n> -`git maintenance stop` will stop the user systemd timers and delete\n> -the above mentioned files.\n> +`git maintenance stop` will disable and stop the user systemd timers.\n> \n> For more details, see `systemd.timer(5)`.\n> \n> -- \n> 2.44.0\n> \n> \n"},{"id":"491131","messageId":"Zfww9jI2em6ZY4SL@framework","threadId":"61145","inReplyTo":"ZfwqCv889UdI0mU6@tanuki","subject":"Re: [RFC PATCH 1/5] maintenance: package systemd units","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-21T13:07:02Z","receivedAt":"2024-03-21T13:07:10Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"On Thu, Mar 21, 2024 at 01:37:30PM +0100, Patrick Steinhardt wrote:\n> On Mon, Mar 18, 2024 at 04:31:15PM +0100, Max Gautier wrote:\n> \n> It would be great to document _why_ we want to package the systemd units\n> alongside with Git.\n> \n\nHum, I wrote that in the cover, but you're right, it should be in the\ncommit itself.\nAck.\n\n> > ...\n> > diff --git a/systemd/user/git-maintenance@.service b/systemd/user/git-maintenance@.service\n> > new file mode 100644\n> > index 0000000000..87ac0c86e6\n> > --- /dev/null\n> > +++ b/systemd/user/git-maintenance@.service\n> > @@ -0,0 +1,16 @@\n> > +[Unit]\n> > +Description=Optimize Git repositories data\n> > +\n> > +[Service]\n> > +Type=oneshot\n> > +ExecStart=git for-each-repo --config=maintenance.repo \\\n> > + 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> \n> Curious, but how did you arrive at these particular restrictions for the\n> unit? Might be something to explain in the commit message, as well.\n> \n> Patrick\n\nI copied the unit file which is defined in strings in builtin/gc.c,\nwhich I delete in patch 3.\nShould the moving be inside one commit, in order to explicit the fact\nthat it's only moving things around ?\n\n-- \nMax Gautier\n"},{"id":"491135","messageId":"ZfwyhSgvhDASusFf@framework","threadId":"61145","inReplyTo":"ZfwqD7z6S8Olc5xa@tanuki","subject":"Re: [RFC PATCH 2/5] maintenance: add fixed random delay to systemd timers","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-21T13:13:41Z","receivedAt":"2024-03-21T13:13:44Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"On Thu, Mar 21, 2024 at 01:37:35PM +0100, Patrick Steinhardt wrote:\n> On Mon, Mar 18, 2024 at 04:31:16PM +0100, Max Gautier wrote:\n> > Ensures that:\n> > - git maintenance timers have a fixed time interval between execution.\n> > - the three timers are not executed at the same time.\n> \n> Commit messages are typically structured so that you first explain what\n> the problem is that you're trying to solve, and then you explain how you\n> solve it. Your commit message is missing the first part.\n> \n> > This is intended to implement an alternative to the two followings\n> > commits:\n> > c97ec0378b (maintenance: fix systemd schedule overlaps, 2023-08-10)\n> > daa787010c (maintenance: use random minute in systemd scheduler, 2023-08-10)\n> > \n> > Instead of manually adding a specific minute (which is reset on each\n> > invocation of `git maintenance start`), we use systemd timers\n> > RandomizedDelaySec and FixedRandomDelay functionalities.\n> \n> I think it would help to not list commits, but put the commit references\n> in a paragraph. Something in the spirit of \"In commit c97ec0378b we\n> already tried to address this issue in such and such a way, but that is\n> quite limiting due to that and that. Similarly, in commit daa787010c..\".\n> \n\nOk I see how that would work.\nStating the limitations of the implemenation added by those commits\nwould be the problem we're trying to solve, then the proposed solution.\n\n> > From man systemd.timer:\n> > >FixedRandomDelay=\n> > > Takes a boolean argument. When enabled, the randomized offset\n> > > specified by RandomizedDelaySec= is reused for all firings of the\n> > > same timer. For a given timer unit, **the offset depends on the\n> > > machine ID, user identifier and timer name**, which means that it is\n> > > stable between restarts of the manager. This effectively creates a\n> > > fixed offset for an individual timer, reducing the jitter in\n> > > firings of this timer, while still avoiding firing at the same time\n> > > as other similarly configured timers.\n> > \n> > -> which is exactly the use case for git-maintenance timers.\n> > \n> > Signed-off-by: Max Gautier \n> > ---\n> > systemd/user/git-maintenance@.service | 1 +\n> > systemd/user/git-maintenance@.timer | 3 +++\n> > 2 files changed, 4 insertions(+)\n> > \n> > diff --git a/systemd/user/git-maintenance@.service b/systemd/user/git-maintenance@.service\n> > index 87ac0c86e6..f949e1a217 100644\n> > --- a/systemd/user/git-maintenance@.service\n> > +++ b/systemd/user/git-maintenance@.service\n> > @@ -1,5 +1,6 @@\n> > [Unit]\n> > Description=Optimize Git repositories data\n> > +Documentation=man:git-maintenance(1)\n> \n> This change feels unrelated and should likely go into the first commit.\n> \n> > \n> > [Service]\n> > Type=oneshot\n> > diff --git a/systemd/user/git-maintenance@.timer b/systemd/user/git-maintenance@.timer\n> > index 40fbc77a62..667c5998ba 100644\n> > --- a/systemd/user/git-maintenance@.timer\n> > +++ b/systemd/user/git-maintenance@.timer\n> > @@ -1,9 +1,12 @@\n> > [Unit]\n> > Description=Optimize Git repositories data\n> > +Documentation=man:git-maintenance(1)\n> \n> Same.\n> \n> Patrick\n\nAck, will do.\n\n> \n> > [Timer]\n> > OnCalendar=%i\n> > Persistent=true\n> > +RandomizedDelaySec=1800\n> > +FixedRandomDelay=true\n> > \n> > [Install]\n> > WantedBy=timers.target\n> > -- \n> > 2.44.0\n> > \n> > \n\n\n\n-- \nMax Gautier\n"},{"id":"491136","messageId":"Zfwz9zs3odKcU9Dm@framework","threadId":"61145","inReplyTo":"ZfwqFU_b3MqLCZNR@tanuki","subject":"Re: [RFC PATCH 3/5] maintenance: use packaged systemd units","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-21T13:19:51Z","receivedAt":"2024-03-21T13:19:53Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"On Thu, Mar 21, 2024 at 01:37:41PM +0100, Patrick Steinhardt wrote:\n> On Mon, Mar 18, 2024 at 04:31:17PM +0100, Max Gautier wrote:\n> > - Remove the code writing the units\n> > - Simplify calling systemctl\n> > - systemctl does not error out when disabling units which aren't\n> > enabled, nor when enabling already enabled units\n> > - call systemctl only once, with all the units.\n> > - Add clean-up code for leftover units in $XDG_CONFIG_HOME created by\n> > previous versions of git, taking care of preserving actual user\n> > override (by checking the start of the file).\n> \n> We very much try to avoid bulleted-list-style commit messages. Once you\n> have a list of changes it very often indicates that there are multiple\n> semi-related things going on in the same commit. At that point we prefer\n> to split the commit up into multiple commits so that each of the bullet\n> items can be explained separately.\n> \n> Also, as mentioned before, commit messages should explain what the\n> problem is and how that problem is solved. \n> \n> Patrick\n> \n\nI need to think a bit about patch ordering here because changing from\nthe \"random minute\" implementation to the systemd\n(RandomDelaySec/FixedRandomDelay) is required to the other changes of\nputting the unit out of the code and in the package.\n\nI think I have an idea how, I'll work on that, thanks.\n\n-- \nMax Gautier\n"},{"id":"491138","messageId":"Zfw0rf7ez7Im1rIx@tanuki","threadId":"61145","inReplyTo":"Zfww9jI2em6ZY4SL@framework","subject":"Re: [RFC PATCH 1/5] maintenance: package systemd units","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-21T13:22:53Z","receivedAt":"2024-03-21T13:23:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Mar 21, 2024 at 02:07:02PM +0100, Max Gautier wrote:\n> On Thu, Mar 21, 2024 at 01:37:30PM +0100, Patrick Steinhardt wrote:\n> > On Mon, Mar 18, 2024 at 04:31:15PM +0100, Max Gautier wrote:\n> > \n> > It would be great to document _why_ we want to package the systemd units\n> > alongside with Git.\n> > \n> \n> Hum, I wrote that in the cover, but you're right, it should be in the\n> commit itself.\n> Ack.\n> \n> > > ...\n> > > diff --git a/systemd/user/git-maintenance@.service b/systemd/user/git-maintenance@.service\n> > > new file mode 100644\n> > > index 0000000000..87ac0c86e6\n> > > --- /dev/null\n> > > +++ b/systemd/user/git-maintenance@.service\n> > > @@ -0,0 +1,16 @@\n> > > +[Unit]\n> > > +Description=Optimize Git repositories data\n> > > +\n> > > +[Service]\n> > > +Type=oneshot\n> > > +ExecStart=git for-each-repo --config=maintenance.repo \\\n> > > + 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> > \n> > Curious, but how did you arrive at these particular restrictions for the\n> > unit? Might be something to explain in the commit message, as well.\n> > \n> > Patrick\n> \n> I copied the unit file which is defined in strings in builtin/gc.c,\n> which I delete in patch 3.\n> Should the moving be inside one commit, in order to explicit the fact\n> that it's only moving things around ?\n\nI don't really think it's necessary. But pointing that out in the commit\nmessage would go a long way to explain where exactly these definitions\ncome from.\n\nPatrick\n"},{"id":"491140","messageId":"Zfw4XNJdZqgZhvOv@framework","threadId":"61145","inReplyTo":"ZfwqCv889UdI0mU6@tanuki","subject":"Re: [RFC PATCH 1/5] maintenance: package systemd units","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-21T13:38:36Z","receivedAt":"2024-03-21T13:38:38Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"On Thu, Mar 21, 2024 at 01:37:30PM +0100, Patrick Steinhardt wrote:\n> On Mon, Mar 18, 2024 at 04:31:15PM +0100, Max Gautier wrote:\n> \n> It would be great to document _why_ we want to package the systemd units\n> alongside with Git.\n> \n> > Signed-off-by: Max Gautier \n> > ---\n> > Makefile | 4 ++++\n> > systemd/user/git-maintenance@.service | 16 ++++++++++++++++\n> > systemd/user/git-maintenance@.timer | 9 +++++++++\n> > 3 files changed, 29 insertions(+)\n> > create mode 100644 systemd/user/git-maintenance@.service\n> > create mode 100644 systemd/user/git-maintenance@.timer\n> > \n> > diff --git a/Makefile b/Makefile\n> > index 4e255c81f2..276b4373c6 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -619,6 +619,7 @@ htmldir = $(prefix)/share/doc/git-doc\n> > ETC_GITCONFIG = $(sysconfdir)/gitconfig\n> > ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes\n> > lib = lib\n> > +libdir = $(prefix)/lib\n> > # DESTDIR =\n> > pathsep = :\n> > \n> > @@ -1328,6 +1329,8 @@ BUILTIN_OBJS += builtin/verify-tag.o\n> > BUILTIN_OBJS += builtin/worktree.o\n> > BUILTIN_OBJS += builtin/write-tree.o\n> > \n> > +SYSTEMD_USER_UNITS := $(wildcard systemd/user/*)\n> > +\n> > # THIRD_PARTY_SOURCES is a list of patterns compatible with the\n> > # $(filter) and $(filter-out) family of functions. They specify source\n> > # files which are taken from some third-party source where we want to be\n> > @@ -3469,6 +3472,7 @@ install: all\n> > \t$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'\n> > \t$(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)'\n> > \t$(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)'\n> > +\t$(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(libdir)/systemd/user' $(SYSTEMD_USER_UNITS)\n> \n> I wonder whether we want to unconditionally install those units. Many of\n> the platforms that we support don't even have systemd available, so\n> certainly it wouldn't make any sense to install it on those platforms.\n> \n> Assuming that this is something we want in the first place I thus think\n> that we should at least make this conditional and add some platform\n> specific quirk to \"config.mak.uname\".\n> \n\nWe probably want that (conditional install) but I'm not sure where that\nshould go ; I'm not super familiar with autoconf. \n\nI just noticed than man 7 daemon (shipped by systemd) propose the\nfollowing snippet for installing systemd system services (should be easy\nenough to adapt for user ervices, I think):\n\nPKG_PROG_PKG_CONFIG()\nAC_ARG_WITH([systemdsystemunitdir],\n [AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd service files])],,\n [with_systemdsystemunitdir=auto])\nAS_IF([test \"x$with_systemdsystemunitdir\" = \"xyes\" -o \"x$with_systemdsystemunitdir\" = \"xauto\"], [\n def_systemdsystemunitdir=$($PKG_CONFIG --variable=systemdsystemunitdir systemd)\n\n AS_IF([test \"x$def_systemdsystemunitdir\" = \"x\"],\n [AS_IF([test \"x$with_systemdsystemunitdir\" = \"xyes\"],\n [AC_MSG_ERROR([systemd support requested but pkg-config unable to query systemd package])])\n with_systemdsystemunitdir=no],\n [with_systemdsystemunitdir=\"$def_systemdsystemunitdir\"])])\nAS_IF([test \"x$with_systemdsystemunitdir\" != \"xno\"],\n [AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])])\nAM_CONDITIONAL([HAVE_SYSTEMD], [test \"x$with_systemdsystemunitdir\" != \"xno\"])\n\nWould something like that work ?\n\n-- \nMax Gautier\n"},{"id":"491148","messageId":"ZfxHtetjUIUByqeF@tanuki","threadId":"61145","inReplyTo":"Zfw4XNJdZqgZhvOv@framework","subject":"Re: [RFC PATCH 1/5] maintenance: package systemd units","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-21T14:44:05Z","receivedAt":"2024-03-21T14:44:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Mar 21, 2024 at 02:38:36PM +0100, Max Gautier wrote:\n> On Thu, Mar 21, 2024 at 01:37:30PM +0100, Patrick Steinhardt wrote:\n> > On Mon, Mar 18, 2024 at 04:31:15PM +0100, Max Gautier wrote:\n> > \n> > It would be great to document _why_ we want to package the systemd units\n> > alongside with Git.\n> > \n> > > Signed-off-by: Max Gautier \n> > > ---\n> > > Makefile | 4 ++++\n> > > systemd/user/git-maintenance@.service | 16 ++++++++++++++++\n> > > systemd/user/git-maintenance@.timer | 9 +++++++++\n> > > 3 files changed, 29 insertions(+)\n> > > create mode 100644 systemd/user/git-maintenance@.service\n> > > create mode 100644 systemd/user/git-maintenance@.timer\n> > > \n> > > diff --git a/Makefile b/Makefile\n> > > index 4e255c81f2..276b4373c6 100644\n> > > --- a/Makefile\n> > > +++ b/Makefile\n> > > @@ -619,6 +619,7 @@ htmldir = $(prefix)/share/doc/git-doc\n> > > ETC_GITCONFIG = $(sysconfdir)/gitconfig\n> > > ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes\n> > > lib = lib\n> > > +libdir = $(prefix)/lib\n> > > # DESTDIR =\n> > > pathsep = :\n> > > \n> > > @@ -1328,6 +1329,8 @@ BUILTIN_OBJS += builtin/verify-tag.o\n> > > BUILTIN_OBJS += builtin/worktree.o\n> > > BUILTIN_OBJS += builtin/write-tree.o\n> > > \n> > > +SYSTEMD_USER_UNITS := $(wildcard systemd/user/*)\n> > > +\n> > > # THIRD_PARTY_SOURCES is a list of patterns compatible with the\n> > > # $(filter) and $(filter-out) family of functions. They specify source\n> > > # files which are taken from some third-party source where we want to be\n> > > @@ -3469,6 +3472,7 @@ install: all\n> > > \t$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'\n> > > \t$(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)'\n> > > \t$(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)'\n> > > +\t$(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(libdir)/systemd/user' $(SYSTEMD_USER_UNITS)\n> > \n> > I wonder whether we want to unconditionally install those units. Many of\n> > the platforms that we support don't even have systemd available, so\n> > certainly it wouldn't make any sense to install it on those platforms.\n> > \n> > Assuming that this is something we want in the first place I thus think\n> > that we should at least make this conditional and add some platform\n> > specific quirk to \"config.mak.uname\".\n> > \n> \n> We probably want that (conditional install) but I'm not sure where that\n> should go ; I'm not super familiar with autoconf. \n> \n> I just noticed than man 7 daemon (shipped by systemd) propose the\n> following snippet for installing systemd system services (should be easy\n> enough to adapt for user ervices, I think):\n> \n> PKG_PROG_PKG_CONFIG()\n> AC_ARG_WITH([systemdsystemunitdir],\n> [AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd service files])],,\n> [with_systemdsystemunitdir=auto])\n> AS_IF([test \"x$with_systemdsystemunitdir\" = \"xyes\" -o \"x$with_systemdsystemunitdir\" = \"xauto\"], [\n> def_systemdsystemunitdir=$($PKG_CONFIG --variable=systemdsystemunitdir systemd)\n> \n> AS_IF([test \"x$def_systemdsystemunitdir\" = \"x\"],\n> [AS_IF([test \"x$with_systemdsystemunitdir\" = \"xyes\"],\n> [AC_MSG_ERROR([systemd support requested but pkg-config unable to query systemd package])])\n> with_systemdsystemunitdir=no],\n> [with_systemdsystemunitdir=\"$def_systemdsystemunitdir\"])])\n> AS_IF([test \"x$with_systemdsystemunitdir\" != \"xno\"],\n> [AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])])\n> AM_CONDITIONAL([HAVE_SYSTEMD], [test \"x$with_systemdsystemunitdir\" != \"xno\"])\n> \n> Would something like that work ?\n\nProbably. But while we do have autoconf wired up, the primary way of\nbuilding Git does not use it, but rather uses `config.mak.uname`. I'd\nrecommend to have a look at it to figure out how we handle other\nbuild-time options there.\n\nPatrick\n"},{"id":"491149","messageId":"ZfxIungwqHdGCZlv@framework","threadId":"61145","inReplyTo":"Zfw4XNJdZqgZhvOv@framework","subject":"Re: [RFC PATCH 1/5] maintenance: package systemd units","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-21T14:48:26Z","receivedAt":"2024-03-21T14:48:29Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"On Thu, Mar 21, 2024 at 02:38:36PM +0100, Max Gautier wrote:\n> On Thu, Mar 21, 2024 at 01:37:30PM +0100, Patrick Steinhardt wrote:\n> > On Mon, Mar 18, 2024 at 04:31:15PM +0100, Max Gautier wrote:\n> > \n> > It would be great to document _why_ we want to package the systemd units\n> > alongside with Git.\n> > \n> > > Signed-off-by: Max Gautier \n> > > ---\n> > > Makefile | 4 ++++\n> > > systemd/user/git-maintenance@.service | 16 ++++++++++++++++\n> > > systemd/user/git-maintenance@.timer | 9 +++++++++\n> > > 3 files changed, 29 insertions(+)\n> > > create mode 100644 systemd/user/git-maintenance@.service\n> > > create mode 100644 systemd/user/git-maintenance@.timer\n> > > \n> > > diff --git a/Makefile b/Makefile\n> > > index 4e255c81f2..276b4373c6 100644\n> > > --- a/Makefile\n> > > +++ b/Makefile\n> > > @@ -619,6 +619,7 @@ htmldir = $(prefix)/share/doc/git-doc\n> > > ETC_GITCONFIG = $(sysconfdir)/gitconfig\n> > > ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes\n> > > lib = lib\n> > > +libdir = $(prefix)/lib\n> > > # DESTDIR =\n> > > pathsep = :\n> > > \n> > > @@ -1328,6 +1329,8 @@ BUILTIN_OBJS += builtin/verify-tag.o\n> > > BUILTIN_OBJS += builtin/worktree.o\n> > > BUILTIN_OBJS += builtin/write-tree.o\n> > > \n> > > +SYSTEMD_USER_UNITS := $(wildcard systemd/user/*)\n> > > +\n> > > # THIRD_PARTY_SOURCES is a list of patterns compatible with the\n> > > # $(filter) and $(filter-out) family of functions. They specify source\n> > > # files which are taken from some third-party source where we want to be\n> > > @@ -3469,6 +3472,7 @@ install: all\n> > > \t$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'\n> > > \t$(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)'\n> > > \t$(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)'\n> > > +\t$(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(libdir)/systemd/user' $(SYSTEMD_USER_UNITS)\n> > \n> > I wonder whether we want to unconditionally install those units. Many of\n> > the platforms that we support don't even have systemd available, so\n> > certainly it wouldn't make any sense to install it on those platforms.\n> > \n> > Assuming that this is something we want in the first place I thus think\n> > that we should at least make this conditional and add some platform\n> > specific quirk to \"config.mak.uname\".\n> > \n> \n> We probably want that (conditional install) but I'm not sure where that\n> should go ; I'm not super familiar with autoconf. \n> \n> I just noticed than man 7 daemon (shipped by systemd) propose the\n> following snippet for installing systemd system services (should be easy\n> enough to adapt for user ervices, I think):\n> \n> PKG_PROG_PKG_CONFIG()\n> AC_ARG_WITH([systemdsystemunitdir],\n> [AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd service files])],,\n> [with_systemdsystemunitdir=auto])\n> AS_IF([test \"x$with_systemdsystemunitdir\" = \"xyes\" -o \"x$with_systemdsystemunitdir\" = \"xauto\"], [\n> def_systemdsystemunitdir=$($PKG_CONFIG --variable=systemdsystemunitdir systemd)\n> \n> AS_IF([test \"x$def_systemdsystemunitdir\" = \"x\"],\n> [AS_IF([test \"x$with_systemdsystemunitdir\" = \"xyes\"],\n> [AC_MSG_ERROR([systemd support requested but pkg-config unable to query systemd package])])\n> with_systemdsystemunitdir=no],\n> [with_systemdsystemunitdir=\"$def_systemdsystemunitdir\"])])\n> AS_IF([test \"x$with_systemdsystemunitdir\" != \"xno\"],\n> [AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])])\n> AM_CONDITIONAL([HAVE_SYSTEMD], [test \"x$with_systemdsystemunitdir\" != \"xno\"])\n> \n> Would something like that work ?\n\nUgh, that snippet apparently requires that I delete aclocal.m4, execute\n`autoreconf --install`, then concat together the previous aclocal.m4 and\nthe newly generated one.\n\nI'm adding the last authors on aclocal.m4 to the discussion, maybe they\nhave some lights ? (Though they don't have commits in the last ten\nyears, so it might be a very long shot ...)\n\n-- \nMax Gautier\n"},{"id":"491150","messageId":"ZfxI5et2h6Bx1bym@framework","threadId":"61145","inReplyTo":"ZfxHtetjUIUByqeF@tanuki","subject":"Re: [RFC PATCH 1/5] maintenance: package systemd units","fromName":"Max Gautier","fromEmail":"mg@max.gautier.name","sentAt":"2024-03-21T14:49:09Z","receivedAt":"2024-03-21T14:49:11Z","isPatch":true,"sender":{"key":"mg@max.gautier.name","avatar":"https://avatars.githubusercontent.com/u/13346812?v=4"},"body":"On Thu, Mar 21, 2024 at 03:44:05PM +0100, Patrick Steinhardt wrote:\n> On Thu, Mar 21, 2024 at 02:38:36PM +0100, Max Gautier wrote:\n> > On Thu, Mar 21, 2024 at 01:37:30PM +0100, Patrick Steinhardt wrote:\n> > > On Mon, Mar 18, 2024 at 04:31:15PM +0100, Max Gautier wrote:\n> > > \n> > > It would be great to document _why_ we want to package the systemd units\n> > > alongside with Git.\n> > > \n> > > > Signed-off-by: Max Gautier \n> > > > ---\n> > > > Makefile | 4 ++++\n> > > > systemd/user/git-maintenance@.service | 16 ++++++++++++++++\n> > > > systemd/user/git-maintenance@.timer | 9 +++++++++\n> > > > 3 files changed, 29 insertions(+)\n> > > > create mode 100644 systemd/user/git-maintenance@.service\n> > > > create mode 100644 systemd/user/git-maintenance@.timer\n> > > > \n> > > > diff --git a/Makefile b/Makefile\n> > > > index 4e255c81f2..276b4373c6 100644\n> > > > --- a/Makefile\n> > > > +++ b/Makefile\n> > > > @@ -619,6 +619,7 @@ htmldir = $(prefix)/share/doc/git-doc\n> > > > ETC_GITCONFIG = $(sysconfdir)/gitconfig\n> > > > ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes\n> > > > lib = lib\n> > > > +libdir = $(prefix)/lib\n> > > > # DESTDIR =\n> > > > pathsep = :\n> > > > \n> > > > @@ -1328,6 +1329,8 @@ BUILTIN_OBJS += builtin/verify-tag.o\n> > > > BUILTIN_OBJS += builtin/worktree.o\n> > > > BUILTIN_OBJS += builtin/write-tree.o\n> > > > \n> > > > +SYSTEMD_USER_UNITS := $(wildcard systemd/user/*)\n> > > > +\n> > > > # THIRD_PARTY_SOURCES is a list of patterns compatible with the\n> > > > # $(filter) and $(filter-out) family of functions. They specify source\n> > > > # files which are taken from some third-party source where we want to be\n> > > > @@ -3469,6 +3472,7 @@ install: all\n> > > > \t$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'\n> > > > \t$(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)'\n> > > > \t$(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)'\n> > > > +\t$(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(libdir)/systemd/user' $(SYSTEMD_USER_UNITS)\n> > > \n> > > I wonder whether we want to unconditionally install those units. Many of\n> > > the platforms that we support don't even have systemd available, so\n> > > certainly it wouldn't make any sense to install it on those platforms.\n> > > \n> > > Assuming that this is something we want in the first place I thus think\n> > > that we should at least make this conditional and add some platform\n> > > specific quirk to \"config.mak.uname\".\n> > > \n> > \n> > We probably want that (conditional install) but I'm not sure where that\n> > should go ; I'm not super familiar with autoconf. \n> > \n> > I just noticed than man 7 daemon (shipped by systemd) propose the\n> > following snippet for installing systemd system services (should be easy\n> > enough to adapt for user ervices, I think):\n> > \n> > PKG_PROG_PKG_CONFIG()\n> > AC_ARG_WITH([systemdsystemunitdir],\n> > [AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd service files])],,\n> > [with_systemdsystemunitdir=auto])\n> > AS_IF([test \"x$with_systemdsystemunitdir\" = \"xyes\" -o \"x$with_systemdsystemunitdir\" = \"xauto\"], [\n> > def_systemdsystemunitdir=$($PKG_CONFIG --variable=systemdsystemunitdir systemd)\n> > \n> > AS_IF([test \"x$def_systemdsystemunitdir\" = \"x\"],\n> > [AS_IF([test \"x$with_systemdsystemunitdir\" = \"xyes\"],\n> > [AC_MSG_ERROR([systemd support requested but pkg-config unable to query systemd package])])\n> > with_systemdsystemunitdir=no],\n> > [with_systemdsystemunitdir=\"$def_systemdsystemunitdir\"])])\n> > AS_IF([test \"x$with_systemdsystemunitdir\" != \"xno\"],\n> > [AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])])\n> > AM_CONDITIONAL([HAVE_SYSTEMD], [test \"x$with_systemdsystemunitdir\" != \"xno\"])\n> > \n> > Would something like that work ?\n> \n> Probably. But while we do have autoconf wired up, the primary way of\n> building Git does not use it, but rather uses `config.mak.uname`. I'd\n> recommend to have a look at it to figure out how we handle other\n> build-time options there.\n> \n> Patrick\n\nI'll take a look, thanks.\n\n\n-- \nMax Gautier\n"}]}