| Message ID | 0fafd75d100f343f7cff6471772ed9b12793f81e.1604520368.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Maintenance IV: Platform-specific background maintenance | expand |
On Wed, Nov 4, 2020 at 3:06 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote: > [...] > The solution is to switch from cron to the Apple-recommended [1] > 'launchd' tool. > [...] > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt > +While macOS technically supports `cron`, using `crontab -e` requires > +elevated privileges and the executed process do not have a full user Either s/process/processes/ or s/do/does/ > +context. Without a full user context, Git and its credential helpers > +cannot access stored credentials, so some maintenance tasks are not > +functional. Nicely explained. > +Instead, `git maintenance start` interacts with the `launchctl` tool, > +which is the recommended way to > +https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/ScheduledJobs.html[schedule timed jobs in macOS]. Nit: I worry a bit about links to Apple documentation becoming outdated. It might not hurt to omit this link altogether, or perhaps demote it to a footnote (which might allow it to be somewhat usable even when Git documentation is rendered into something other than HTML). > +Scheduling maintenance through `git maintenance (start|stop)` requires > +some `launchctl` features available only in macOS 10.11 or later. Nit: This leaves the reader wondering what modern features are needed. Would it make sense to mention that "bootstrap" is used in place of "load" in older versions of 'launchctl'? > +Your user-specific scheduled tasks are stored as XML-formatted `.plist` > +files in `~/Library/LaunchAgents/`. You can see the currently-registered > +tasks using the following command: > + > +----------------------------------------------------------------------- > +$ ls ~/Library/LaunchAgents/ | grep org.git-scm.git Alternately (unimportant): ls ~/Library/LaunchAgents/org.git-scm.git.* although that would emit "No such file" if you don't have any registered, which might suggest: find ~/Library/LaunchAgents -name 'org.git-scm.git.*' > +To create more advanced customizations to your background tasks, see > +https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/CreatingLaunchdJobs.html#//apple_ref/doc/uid/TP40001762-104142[the `launchctl` documentation] > +for more information. I really worry about this sort of URL becoming outdated. Would it make sense instead to just point the user at the man page, launchd.plist(5)? It's not quite the same, as it doesn't provide the range of examples as the URL you cite, but it should get the user started. > diff --git a/builtin/gc.c b/builtin/gc.c > @@ -1491,6 +1491,214 @@ static int maintenance_unregister(void) > +static int remove_plist(enum schedule_priority schedule) > +{ > + const char *frequency = get_frequency(schedule); > + char *name = get_service_name(frequency); > + char *filename = get_service_filename(name); > + int result = bootout(filename); > + free(filename); > + free(name); > + return result; > +} > > +static int remove_plists(void) > +{ > + return remove_plist(SCHEDULE_HOURLY) || > + remove_plist(SCHEDULE_DAILY) || > + remove_plist(SCHEDULE_WEEKLY); > +} The new documentation you added says that the plist files will be deleted after they are deregistered using launchctl, but I don't see anything actually deleting them. Am I missing something obvious? > +static int schedule_plist(const char *exec_path, enum schedule_priority schedule) > +{ > + plist = fopen(filename, "w"); > + if (!plist) > + die(_("failed to open '%s'"), filename); As mentioned previously, these could be replaced with a simple xfopen(). In fact, I'm having trouble seeing changes in this re-roll which you had planned on making, such as consolidating the repeated code in bootout() and bootstrap(), and ensuring that bootout() doesn't complain if the plist files are already missing, and so forth. Did you opt to not make those changes? (Which would be fine; they were minor suggestions.) > + preamble = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" > + "<!DOCTYPE plist PUBLIC \"-//Apple//DTD PLIST 1.0//EN\" \"http://www.apple.com/DTDs/PropertyList-1.0.dtd\">\n" > + "<plist version=\"1.0\">" > + "<dict>\n" > + "<key>Label</key><string>%s</string>\n" > + "<key>ProgramArguments</key>\n" > + "<array>\n" > + "<string>%s/git</string>\n" > + "<string>--exec-path=%s</string>\n" > + "<string>for-each-repo</string>\n" > + "<string>--config=maintenance.repo</string>\n" > + "<string>maintenance</string>\n" > + "<string>run</string>\n" > + "<string>--schedule=%s</string>\n" > + "</array>\n" > + "<key>StartCalendarInterval</key>\n" > + "<array>\n"; > + fprintf(plist, preamble, name, exec_path, exec_path, frequency); The Git test framework ensures that this will be written into the test directory rather than the user's actual ~/Library/LaunchAgents directory during testing. Okay. > +test_expect_success MACOS_MAINTENANCE 'start and stop macOS maintenance' ' > + echo "#!/bin/sh\necho \$@ >>args" >print-args && > + chmod a+x print-args && Earlier review already mentioned write_script() and "$@". (Not necessarily worth a re-roll.) > + for frequency in hourly daily weekly > + do > + PLIST="$HOME/Library/LaunchAgents/org.git-scm.git.$frequency.plist" && > + xmllint "$PLIST" >/dev/null && Do we really need to suppress xmllint's stdout? > + grep schedule=$frequency "$PLIST" && > + echo "bootout gui/$UID $PLIST" >>expect && > + echo "bootstrap gui/$UID $PLIST" >>expect || return 1 > + done && > + test_cmp expect args && > + > + rm -f args && > + GIT_TEST_CRONTAB="./print-args" git maintenance stop && There is still an extra space between the closing quote and git command (mentioned previously). > + # stop does not unregister the repo > + git config --get --global maintenance.repo "$(pwd)" && > + > + # stop does not remove plist files, but boots them out Documentation added in this re-roll claims that the plist files do get deleted.
On 11/11/2020 3:12 AM, Eric Sunshine wrote: > On Wed, Nov 4, 2020 at 3:06 PM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> [...] >> The solution is to switch from cron to the Apple-recommended [1] >> 'launchd' tool. >> [...] >> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> >> --- >> diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt >> +While macOS technically supports `cron`, using `crontab -e` requires >> +elevated privileges and the executed process do not have a full user > > Either s/process/processes/ or s/do/does/ > >> +context. Without a full user context, Git and its credential helpers >> +cannot access stored credentials, so some maintenance tasks are not >> +functional. > > Nicely explained. > >> +Instead, `git maintenance start` interacts with the `launchctl` tool, >> +which is the recommended way to >> +https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/ScheduledJobs.html[schedule timed jobs in macOS]. > > Nit: I worry a bit about links to Apple documentation becoming > outdated. It might not hurt to omit this link altogether, or perhaps > demote it to a footnote (which might allow it to be somewhat usable > even when Git documentation is rendered into something other than > HTML). > >> +Scheduling maintenance through `git maintenance (start|stop)` requires >> +some `launchctl` features available only in macOS 10.11 or later. > > Nit: This leaves the reader wondering what modern features are needed. > Would it make sense to mention that "bootstrap" is used in place of > "load" in older versions of 'launchctl'? > >> +Your user-specific scheduled tasks are stored as XML-formatted `.plist` >> +files in `~/Library/LaunchAgents/`. You can see the currently-registered >> +tasks using the following command: >> + >> +----------------------------------------------------------------------- >> +$ ls ~/Library/LaunchAgents/ | grep org.git-scm.git > > Alternately (unimportant): > > ls ~/Library/LaunchAgents/org.git-scm.git.* > > although that would emit "No such file" if you don't have any > registered, which might suggest: > > find ~/Library/LaunchAgents -name 'org.git-scm.git.*' > >> +To create more advanced customizations to your background tasks, see >> +https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/CreatingLaunchdJobs.html#//apple_ref/doc/uid/TP40001762-104142[the `launchctl` documentation] >> +for more information. > > I really worry about this sort of URL becoming outdated. Would it make > sense instead to just point the user at the man page, > launchd.plist(5)? It's not quite the same, as it doesn't provide the > range of examples as the URL you cite, but it should get the user > started. I shared similar concerns. I'll use the man page references instead. All of the information should be a short web search away after the user is given the right terminology. >> diff --git a/builtin/gc.c b/builtin/gc.c >> @@ -1491,6 +1491,214 @@ static int maintenance_unregister(void) >> +static int remove_plist(enum schedule_priority schedule) >> +{ >> + const char *frequency = get_frequency(schedule); >> + char *name = get_service_name(frequency); >> + char *filename = get_service_filename(name); >> + int result = bootout(filename); >> + free(filename); >> + free(name); >> + return result; >> +} >> >> +static int remove_plists(void) >> +{ >> + return remove_plist(SCHEDULE_HOURLY) || >> + remove_plist(SCHEDULE_DAILY) || >> + remove_plist(SCHEDULE_WEEKLY); >> +} > > The new documentation you added says that the plist files will be > deleted after they are deregistered using launchctl, but I don't see > anything actually deleting them. Am I missing something obvious? As mentioned below, this was a change that I made but somehow lost while juggling multiple copies of my branch. >> +static int schedule_plist(const char *exec_path, enum schedule_priority schedule) >> +{ >> + plist = fopen(filename, "w"); >> + if (!plist) >> + die(_("failed to open '%s'"), filename); > > As mentioned previously, these could be replaced with a simple xfopen(). > > In fact, I'm having trouble seeing changes in this re-roll which you > had planned on making, such as consolidating the repeated code in > bootout() and bootstrap(), and ensuring that bootout() doesn't > complain if the plist files are already missing, and so forth. Did you > opt to not make those changes? (Which would be fine; they were minor > suggestions.) No, I definitely made those changes _somewhere_ but I must have gotten confused as to which of my machines had those changes. I guess that's part of the risk of testing across three platforms. Thank you for noticing, and I'll be more careful from now on. >> +test_expect_success MACOS_MAINTENANCE 'start and stop macOS maintenance' ' >> + echo "#!/bin/sh\necho \$@ >>args" >print-args && >> + chmod a+x print-args && > > Earlier review already mentioned write_script() and "$@". (Not > necessarily worth a re-roll.) I'm going to go back to all of your earlier comments to make sure they are _actually_ applied in v3. >> + for frequency in hourly daily weekly >> + do >> + PLIST="$HOME/Library/LaunchAgents/org.git-scm.git.$frequency.plist" && >> + xmllint "$PLIST" >/dev/null && > > Do we really need to suppress xmllint's stdout? It outputs the XML itself. Maybe there is a command to stop that from happening, but nulling stdout keeps the test log clean. Thanks, -Stolee
On Thu, Nov 12, 2020 at 8:43 AM Derrick Stolee <stolee@gmail.com> wrote: > On 11/11/2020 3:12 AM, Eric Sunshine wrote: > > On Wed, Nov 4, 2020 at 3:06 PM Derrick Stolee via GitGitGadget > > <gitgitgadget@gmail.com> wrote: > >> + xmllint "$PLIST" >/dev/null && > > > > Do we really need to suppress xmllint's stdout? > > It outputs the XML itself. Maybe there is a command to stop that from > happening, but nulling stdout keeps the test log clean. xmllint's --noout option should do the trick.
diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt index 4c7aac877d..451ebac131 100644 --- a/Documentation/git-maintenance.txt +++ b/Documentation/git-maintenance.txt @@ -273,6 +273,49 @@ for advanced scheduling techniques. Please do use the full path and executing the correct binaries in your schedule. +BACKGROUND MAINTENANCE ON MACOS SYSTEMS +--------------------------------------- + +While macOS technically supports `cron`, using `crontab -e` requires +elevated privileges and the executed process do not have a full user +context. Without a full user context, Git and its credential helpers +cannot access stored credentials, so some maintenance tasks are not +functional. + +Instead, `git maintenance start` interacts with the `launchctl` tool, +which is the recommended way to +https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/ScheduledJobs.html[schedule timed jobs in macOS]. + +Scheduling maintenance through `git maintenance (start|stop)` requires +some `launchctl` features available only in macOS 10.11 or later. + +Your user-specific scheduled tasks are stored as XML-formatted `.plist` +files in `~/Library/LaunchAgents/`. You can see the currently-registered +tasks using the following command: + +----------------------------------------------------------------------- +$ ls ~/Library/LaunchAgents/ | grep org.git-scm.git +org.git-scm.git.daily.plist +org.git-scm.git.hourly.plist +org.git-scm.git.weekly.plist +----------------------------------------------------------------------- + +One task is registered for each `--schedule=<frequency>` option. To +inspect how the XML format describes each schedule, open one of these +`.plist` files in an editor and inspect the `<array>` element following +the `<key>StartCalendarInterval</key>` element. + +`git maintenance start` will overwrite these files and register the +tasks again with `launchctl`, so any customizations should be done by +creating your own `.plist` files with distinct names. Similarly, the +`git maintenance stop` command will unregister the tasks with `launchctl` +and delete the `.plist` files. + +To create more advanced customizations to your background tasks, see +https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/CreatingLaunchdJobs.html#//apple_ref/doc/uid/TP40001762-104142[the `launchctl` documentation] +for more information. + + GIT --- Part of the linkgit:git[1] suite diff --git a/builtin/gc.c b/builtin/gc.c index c1f7d9bdc2..7604064a8d 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1491,6 +1491,214 @@ static int maintenance_unregister(void) return run_command(&config_unset); } +#if defined(__APPLE__) + +static char *get_service_name(const char *frequency) +{ + struct strbuf label = STRBUF_INIT; + strbuf_addf(&label, "org.git-scm.git.%s", frequency); + return strbuf_detach(&label, NULL); +} + +static char *get_service_filename(const char *name) +{ + char *expanded; + struct strbuf filename = STRBUF_INIT; + strbuf_addf(&filename, "~/Library/LaunchAgents/%s.plist", name); + + expanded = expand_user_path(filename.buf, 1); + if (!expanded) + die(_("failed to expand path '%s'"), filename.buf); + + strbuf_release(&filename); + return expanded; +} + +static const char *get_frequency(enum schedule_priority schedule) +{ + switch (schedule) { + case SCHEDULE_HOURLY: + return "hourly"; + case SCHEDULE_DAILY: + return "daily"; + case SCHEDULE_WEEKLY: + return "weekly"; + default: + BUG("invalid schedule %d", schedule); + } +} + +static char *get_uid(void) +{ + struct strbuf output = STRBUF_INIT; + struct child_process id = CHILD_PROCESS_INIT; + + strvec_pushl(&id.args, "/usr/bin/id", "-u", NULL); + if (capture_command(&id, &output, 0)) + die(_("failed to discover user id")); + + strbuf_trim_trailing_newline(&output); + return strbuf_detach(&output, NULL); +} + +static int bootout(const char *filename) +{ + int result; + struct strvec args = STRVEC_INIT; + char *uid = get_uid(); + const char *launchctl = getenv("GIT_TEST_CRONTAB"); + if (!launchctl) + launchctl = "/bin/launchctl"; + + strvec_split(&args, launchctl); + strvec_push(&args, "bootout"); + strvec_pushf(&args, "gui/%s", uid); + strvec_push(&args, filename); + + result = run_command_v_opt(args.v, 0); + + strvec_clear(&args); + free(uid); + return result; +} + +static int bootstrap(const char *filename) +{ + int result; + struct strvec args = STRVEC_INIT; + char *uid = get_uid(); + const char *launchctl = getenv("GIT_TEST_CRONTAB"); + if (!launchctl) + launchctl = "/bin/launchctl"; + + strvec_split(&args, launchctl); + strvec_push(&args, "bootstrap"); + strvec_pushf(&args, "gui/%s", uid); + strvec_push(&args, filename); + + result = run_command_v_opt(args.v, 0); + + strvec_clear(&args); + free(uid); + return result; +} + +static int remove_plist(enum schedule_priority schedule) +{ + const char *frequency = get_frequency(schedule); + char *name = get_service_name(frequency); + char *filename = get_service_filename(name); + int result = bootout(filename); + free(filename); + free(name); + return result; +} + +static int remove_plists(void) +{ + return remove_plist(SCHEDULE_HOURLY) || + remove_plist(SCHEDULE_DAILY) || + remove_plist(SCHEDULE_WEEKLY); +} + +static int schedule_plist(const char *exec_path, enum schedule_priority schedule) +{ + FILE *plist; + int i; + const char *preamble, *repeat; + const char *frequency = get_frequency(schedule); + char *name = get_service_name(frequency); + char *filename = get_service_filename(name); + + if (safe_create_leading_directories(filename)) + die(_("failed to create directories for '%s'"), filename); + plist = fopen(filename, "w"); + + if (!plist) + die(_("failed to open '%s'"), filename); + + preamble = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" + "<!DOCTYPE plist PUBLIC \"-//Apple//DTD PLIST 1.0//EN\" \"http://www.apple.com/DTDs/PropertyList-1.0.dtd\">\n" + "<plist version=\"1.0\">" + "<dict>\n" + "<key>Label</key><string>%s</string>\n" + "<key>ProgramArguments</key>\n" + "<array>\n" + "<string>%s/git</string>\n" + "<string>--exec-path=%s</string>\n" + "<string>for-each-repo</string>\n" + "<string>--config=maintenance.repo</string>\n" + "<string>maintenance</string>\n" + "<string>run</string>\n" + "<string>--schedule=%s</string>\n" + "</array>\n" + "<key>StartCalendarInterval</key>\n" + "<array>\n"; + fprintf(plist, preamble, name, exec_path, exec_path, frequency); + + switch (schedule) { + case SCHEDULE_HOURLY: + repeat = "<dict>\n" + "<key>Hour</key><integer>%d</integer>\n" + "<key>Minute</key><integer>0</integer>\n" + "</dict>\n"; + for (i = 1; i <= 23; i++) + fprintf(plist, repeat, i); + break; + + case SCHEDULE_DAILY: + repeat = "<dict>\n" + "<key>Day</key><integer>%d</integer>\n" + "<key>Hour</key><integer>0</integer>\n" + "<key>Minute</key><integer>0</integer>\n" + "</dict>\n"; + for (i = 1; i <= 6; i++) + fprintf(plist, repeat, i); + break; + + case SCHEDULE_WEEKLY: + fprintf(plist, + "<dict>\n" + "<key>Day</key><integer>0</integer>\n" + "<key>Hour</key><integer>0</integer>\n" + "<key>Minute</key><integer>0</integer>\n" + "</dict>\n"); + break; + + default: + /* unreachable */ + break; + } + fprintf(plist, "</array>\n</dict>\n</plist>\n"); + + /* bootout might fail if not already running, so ignore */ + bootout(filename); + if (bootstrap(filename)) + die(_("failed to bootstrap service %s"), filename); + + fclose(plist); + free(filename); + free(name); + return 0; +} + +static int add_plists(void) +{ + const char *exec_path = git_exec_path(); + + return schedule_plist(exec_path, SCHEDULE_HOURLY) || + schedule_plist(exec_path, SCHEDULE_DAILY) || + schedule_plist(exec_path, SCHEDULE_WEEKLY); +} + +static int platform_update_schedule(int run_maintenance, int fd) +{ + if (run_maintenance) + return add_plists(); + else + return remove_plists(); +} +#else #define BEGIN_LINE "# BEGIN GIT MAINTENANCE SCHEDULE" #define END_LINE "# END GIT MAINTENANCE SCHEDULE" @@ -1585,6 +1793,7 @@ static int platform_update_schedule(int run_maintenance, int fd) fclose(cron_list); return result; } +#endif static int update_background_schedule(int run_maintenance) { diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 20184e96e1..1c43b34a93 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -367,7 +367,7 @@ test_expect_success 'register and unregister' ' test_cmp before actual ' -test_expect_success 'start from empty cron table' ' +test_expect_success !MACOS_MAINTENANCE 'start from empty cron table' ' GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start && # start registers the repo @@ -378,7 +378,7 @@ test_expect_success 'start from empty cron table' ' grep "for-each-repo --config=maintenance.repo maintenance run --schedule=weekly" cron.txt ' -test_expect_success 'stop from existing schedule' ' +test_expect_success !MACOS_MAINTENANCE 'stop from existing schedule' ' GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance stop && # stop does not unregister the repo @@ -389,12 +389,59 @@ test_expect_success 'stop from existing schedule' ' test_must_be_empty cron.txt ' -test_expect_success 'start preserves existing schedule' ' +test_expect_success !MACOS_MAINTENANCE 'start preserves existing schedule' ' echo "Important information!" >cron.txt && GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start && grep "Important information!" cron.txt ' +test_expect_success MACOS_MAINTENANCE 'start and stop macOS maintenance' ' + echo "#!/bin/sh\necho \$@ >>args" >print-args && + chmod a+x print-args && + + rm -f args && + GIT_TEST_CRONTAB="./print-args" git maintenance start && + + # start registers the repo + git config --get --global maintenance.repo "$(pwd)" && + + # ~/Library/LaunchAgents + ls "$HOME/Library/LaunchAgents" >actual && + cat >expect <<-\EOF && + org.git-scm.git.daily.plist + org.git-scm.git.hourly.plist + org.git-scm.git.weekly.plist + EOF + test_cmp expect actual && + + rm expect && + for frequency in hourly daily weekly + do + PLIST="$HOME/Library/LaunchAgents/org.git-scm.git.$frequency.plist" && + xmllint "$PLIST" >/dev/null && + grep schedule=$frequency "$PLIST" && + echo "bootout gui/$UID $PLIST" >>expect && + echo "bootstrap gui/$UID $PLIST" >>expect || return 1 + done && + test_cmp expect args && + + rm -f args && + GIT_TEST_CRONTAB="./print-args" git maintenance stop && + + # stop does not unregister the repo + git config --get --global maintenance.repo "$(pwd)" && + + # stop does not remove plist files, but boots them out + rm expect && + for frequency in hourly daily weekly + do + PLIST="$HOME/Library/LaunchAgents/org.git-scm.git.$frequency.plist" && + grep schedule=$frequency "$PLIST" && + echo "bootout gui/$UID $PLIST" >>expect || return 1 + done && + test_cmp expect args +' + test_expect_success 'register preserves existing strategy' ' git config maintenance.strategy none && git maintenance register && diff --git a/t/test-lib.sh b/t/test-lib.sh index 4a60d1ed76..620ffbf3af 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1703,6 +1703,10 @@ test_lazy_prereq REBASE_P ' test -z "$GIT_TEST_SKIP_REBASE_P" ' +test_lazy_prereq MACOS_MAINTENANCE ' + launchctl list +' + # Ensure that no test accidentally triggers a Git command # that runs 'crontab', affecting a user's cron schedule. # Tests that verify the cron integration must set this locally