| Message ID | 1629bcfcf82dbc2ed9889a0e9ea2d08427901c4e.1605276024.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Maintenance IV: Platform-specific background maintenance | expand |
On Fri, Nov 13, 2020 at 9:00 AM 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/builtin/gc.c b/builtin/gc.c > @@ -1491,6 +1491,200 @@ static int maintenance_unregister(void) > +static int boot_plist(int enable, const char *filename) > +{ > + struct child_process child = CHILD_PROCESS_INIT; > + child.no_stderr = 1; > + child.no_stdout = 1; > + if (start_command(&child)) > + die(_("failed to start launchctl")); Not necessarily worth a re-roll -- in fact, it could be done atop this series to avoid holding this series up -- but this too-succinct error reporting won't help users diagnose the failure. An alternative would be to capture stdout and stderr and only print them if the command fails. Perhaps something like this: struct strbuf out = STRBUF_INIT; struct strbuf err = STRBUF_INIT; ... if (pipe_command(child, NULL, 0, &out, 0, &err, 0) { if (out.len && err.len) strbuf_addstr(&out, "; "); strbuf_addbuf(&out, &err); die(_("launchctl failed: %s"), out.buf); } By the way, won't this die() be a problem when schedule_plist() calls boot_plist() to remove the old scheduled tasks before calling it again to register the new ones? If the old ones don't exist, then it will die() unnecessarily and never register the new ones. Or am I misunderstanding? (I'm guessing that I must be misunderstanding since the test script presumably passes.) > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh > @@ -389,12 +389,54 @@ test_expect_success 'stop from existing schedule' ' > +test_expect_success MACOS_MAINTENANCE 'start and stop macOS maintenance' ' > + write_script print-args "#!/bin/sh\necho \$* >>args" && write_script() takes the script body as stdin, not as an argument, and you don't need to specify /bin/sh. What you have here works by accident only because write_script() takes an optional second argument specifying the shell to use in place of the default /bin/sh. Nevertheless, it should really be written: write_script print-args <<-\EOF echo $* EOF Patch [4/4] uses write_script() correctly. > + 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 && Not sure what the comment above the `ls` is meant to be conveying. Could be dropped but not itself worth a re-roll. > + 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 --noout "$PLIST" && > + grep schedule=$frequency "$PLIST" && > + echo "bootout gui/$UID $PLIST" >>expect && > + echo "bootstrap gui/$UID $PLIST" >>expect || return 1 > + done && This is still relying upon $UID picked up from the users environment (as far as I can tell), which seems fragile. As mentioned in my first review, it probably would be more robust to compute UID manually the same way git-maintenance itself does. > + test_cmp expect args && > + > + rm -f args && > + GIT_TEST_CRONTAB="./print-args" git maintenance stop && Minor: No need for the quotes around ./print-args (though they don't hurt either, and certainly not worth re-rolling just to drop them, and it's subjective so don't drop them just for my sake). > + # stop does not unregister the repo > + git config --get --global maintenance.repo "$(pwd)" && > + > + printf "bootout gui/$UID $HOME/Library/LaunchAgents/org.git-scm.git.%s.plist\n" \ > + hourly daily weekly >expect && > + test_cmp expect args && > + ls "$HOME/Library/LaunchAgents" >actual && > + test_line_count = 0 actual > +'
On 11/13/2020 3:19 PM, Eric Sunshine wrote: > On Fri, Nov 13, 2020 at 9:00 AM 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/builtin/gc.c b/builtin/gc.c >> @@ -1491,6 +1491,200 @@ static int maintenance_unregister(void) >> +static int boot_plist(int enable, const char *filename) >> +{ >> + struct child_process child = CHILD_PROCESS_INIT; >> + child.no_stderr = 1; >> + child.no_stdout = 1; >> + if (start_command(&child)) >> + die(_("failed to start launchctl")); > > Not necessarily worth a re-roll -- in fact, it could be done atop this > series to avoid holding this series up -- but this too-succinct error > reporting won't help users diagnose the failure. An alternative would > be to capture stdout and stderr and only print them if the command > fails. Perhaps something like this: > > struct strbuf out = STRBUF_INIT; > struct strbuf err = STRBUF_INIT; > ... > if (pipe_command(child, NULL, 0, &out, 0, &err, 0) { > if (out.len && err.len) > strbuf_addstr(&out, "; "); > strbuf_addbuf(&out, &err); > die(_("launchctl failed: %s"), out.buf); > } We would also want to pass a "die_on_failure" into the method, since in the 'git maintenance start' case we don't want to report a failure when 'launchctl bootout' fails before we call 'launchctl bootstrap'. > By the way, won't this die() be a problem when schedule_plist() calls > boot_plist() to remove the old scheduled tasks before calling it again > to register the new ones? If the old ones don't exist, then it will > die() unnecessarily and never register the new ones. Or am I > misunderstanding? (I'm guessing that I must be misunderstanding since > the test script presumably passes.) This die() is only if the process cannot _start_, for example due to launchctl not existing on $PATH. The result from finish_command() would be non-zero when we bootout a plist that doesn't exist. >> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh >> @@ -389,12 +389,54 @@ test_expect_success 'stop from existing schedule' ' >> +test_expect_success MACOS_MAINTENANCE 'start and stop macOS maintenance' ' >> + write_script print-args "#!/bin/sh\necho \$* >>args" && > > write_script() takes the script body as stdin, not as an argument, and > you don't need to specify /bin/sh. What you have here works by > accident only because write_script() takes an optional second argument > specifying the shell to use in place of the default /bin/sh. > Nevertheless, it should really be written: > > write_script print-args <<-\EOF > echo $* > EOF > > Patch [4/4] uses write_script() correctly. Ah. Sorry for misunderstanding. That explains why it works this way on macOS but it did _not_ work that way on Windows. >> + 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 && > > Not sure what the comment above the `ls` is meant to be conveying. > Could be dropped but not itself worth a re-roll. > >> + 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 --noout "$PLIST" && >> + grep schedule=$frequency "$PLIST" && >> + echo "bootout gui/$UID $PLIST" >>expect && >> + echo "bootstrap gui/$UID $PLIST" >>expect || return 1 >> + done && > > This is still relying upon $UID picked up from the users environment > (as far as I can tell), which seems fragile. As mentioned in my first > review, it probably would be more robust to compute UID manually the > same way git-maintenance itself does. Sorry, I missed this comment from v1 when reapplying the changes for v3. >> + test_cmp expect args && >> + >> + rm -f args && >> + GIT_TEST_CRONTAB="./print-args" git maintenance stop && > > Minor: No need for the quotes around ./print-args (though they don't > hurt either, and certainly not worth re-rolling just to drop them, and > it's subjective so don't drop them just for my sake). Thank you for your continued attention and patience. -Stolee
On Fri, Nov 13, 2020 at 3:42 PM Derrick Stolee <stolee@gmail.com> wrote: > On 11/13/2020 3:19 PM, Eric Sunshine wrote: > > if (pipe_command(child, NULL, 0, &out, 0, &err, 0) { > > if (out.len && err.len) > > strbuf_addstr(&out, "; "); > > strbuf_addbuf(&out, &err); > > die(_("launchctl failed: %s"), out.buf); > > } > > We would also want to pass a "die_on_failure" into the method, since > in the 'git maintenance start' case we don't want to report a failure > when 'launchctl bootout' fails before we call 'launchctl bootstrap'. Right. I started writing that we'd also need a `die_one_failure` flag but deleted the comment since I decided to wait until I got an answer... > > By the way, won't this die() be a problem when schedule_plist() calls > > boot_plist() to remove the old scheduled tasks before calling it again > > to register the new ones? If the old ones don't exist, then it will > > die() unnecessarily and never register the new ones. Or am I > > misunderstanding? (I'm guessing that I must be misunderstanding since > > the test script presumably passes.) > > This die() is only if the process cannot _start_, for example due to > launchctl not existing on $PATH. The result from finish_command() > would be non-zero when we bootout a plist that doesn't exist. ... to this question. Another thought I had was simply checking for the presence of the file and skipping `bootout` altogether if it doesn't exist. That would, I think, obviate the need for mucking with stdout/stderr oppression. > > write_script() takes the script body as stdin, not as an argument, and > > you don't need to specify /bin/sh. What you have here works by > > accident only because write_script() takes an optional second argument > > specifying the shell to use in place of the default /bin/sh. > > Nevertheless, it should really be written: > > > > write_script print-args <<-\EOF > > echo $* > > EOF > > > > Patch [4/4] uses write_script() correctly. > > Ah. Sorry for misunderstanding. That explains why it works this way > on macOS but it did _not_ work that way on Windows. Sorry on my part too. I missed the `args` redirect in my example. It should be: write_script print-args <<-\EOF echo $* >args EOF
On Fri, Nov 13, 2020 at 3:53 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > Another thought I had was simply checking for the presence of the file > and skipping `bootout` altogether if it doesn't exist. That would, I > think, obviate the need for mucking with stdout/stderr oppression. Erm, s/oppression/suppression/.
diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt index 1aa1112418..5f8f63f098 100644 --- a/Documentation/git-maintenance.txt +++ b/Documentation/git-maintenance.txt @@ -273,6 +273,46 @@ schedule to ensure you are 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 does 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 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/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 +launchctl.plist(5) for more information. + + GIT --- Part of the linkgit:git[1] suite diff --git a/builtin/gc.c b/builtin/gc.c index c1f7d9bdc2..da2c892f68 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1491,6 +1491,200 @@ 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 boot_plist(int enable, const char *filename) +{ + int result; + struct child_process child = CHILD_PROCESS_INIT; + char *uid = get_uid(); + const char *launchctl = getenv("GIT_TEST_CRONTAB"); + if (!launchctl) + launchctl = "/bin/launchctl"; + + strvec_split(&child.args, launchctl); + + if (enable) + strvec_push(&child.args, "bootstrap"); + else + strvec_push(&child.args, "bootout"); + strvec_pushf(&child.args, "gui/%s", uid); + strvec_push(&child.args, filename); + + child.no_stderr = 1; + child.no_stdout = 1; + + if (start_command(&child)) + die(_("failed to start launchctl")); + + result = finish_command(&child); + + 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 = boot_plist(0, filename); + unlink(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 = xfopen(filename, "w"); + + 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 */ + boot_plist(0, filename); + if (boot_plist(1, 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 +1779,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..29d340a828 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,54 @@ 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' ' + write_script print-args "#!/bin/sh\necho \$* >>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 --noout "$PLIST" && + 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)" && + + printf "bootout gui/$UID $HOME/Library/LaunchAgents/org.git-scm.git.%s.plist\n" \ + hourly daily weekly >expect && + test_cmp expect args && + ls "$HOME/Library/LaunchAgents" >actual && + test_line_count = 0 actual +' + 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