fsmonitor: eliminate call to deprecated FSEventStream function

Commit Message

Jeff Hostetler Dec. 2, 2022, 2:24 p.m. UTC

From: Jeff Hostetler <[e-mail]>

Replace the call to `FSEventStreamScheduleWithRunLoop()` function with
the suggested `FSEventStreamSetDispatchQueue()` function.

The MacOS version of the builtin FSMonitor feature uses the
`FSEventStreamScheduleWithRunLoop()` function to drive the event loop
and process FSEvents from the system.  This routine has now been
deprecated by Apple.  The MacOS 13 (Ventana) compiler tool chain now
generates a warning when compiling calls to this function.  In
DEVELOPER=1 mode, this now causes a compile error.

The `FSEventStreamSetDispatchQueue()` function is conceptually similar
and is the suggested replacement.  However, there are some subtle
thread-related differences.

Previously, the event stream would be processed by the
`fsm_listen__loop()` thread while it was in the `CFRunLoopRun()`
method.  (Conceptually, this was a blocking call on the lifetime of
the event stream where our thread drove the event loop and individual
events were handled by the `fsevent_callback()`.)

With the change, a "dispatch queue" is created and FSEvents will be
processed by a hidden queue-related thread (that calls the
`fsevent_callback()` on our behalf).  Our `fsm_listen__loop()` thread
maintains the original blocking model by waiting on a mutex/condition
variable pair while the hidden thread does all of the work.

Signed-off-by: Jeff Hostetler <[e-mail]>
---
    fsmonitor: eliminate call to deprecated FSEventStream function
    
    This patch replaces the call to the now deprecated
    FSEventStreamScheduleWithRunLoop() function with
    FSEventStreamSetDispatchQueue() as suggested by the compiler warning in
    the MacOS version of FSMonitor.
    
    I have tested this on MacOS 13 on my Intel and M1 laptops. And it has
    passed all tests on the GGG CI builds. However, I don't have access to
    an older version of MacOS anymore. The docs say that this new (to us)
    function has been available since 10.6, so we should be OK using it on
    older versions of MacOS, but I cannot confirm that.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1436%2Fjeffhostetler%2Ffsmonitor-macos-dispatch-queue-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1436/jeffhostetler/fsmonitor-macos-dispatch-queue-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1436

 compat/fsmonitor/fsm-darwin-gcc.h    |  4 +---
 compat/fsmonitor/fsm-listen-darwin.c | 35 +++++++++++++++++++---------
 2 files changed, 25 insertions(+), 14 deletions(-)


base-commit: 7452749a781d84244ecd08c6f6ca7e5df67dfce8

Comments

Victoria Dye Dec. 2, 2022, 5:42 p.m. UTC | #1

Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <[e-mail]>
> 
> Replace the call to `FSEventStreamScheduleWithRunLoop()` function with
> the suggested `FSEventStreamSetDispatchQueue()` function.
> 
> The MacOS version of the builtin FSMonitor feature uses the
> `FSEventStreamScheduleWithRunLoop()` function to drive the event loop
> and process FSEvents from the system.  This routine has now been
> deprecated by Apple.  The MacOS 13 (Ventana) compiler tool chain now
> generates a warning when compiling calls to this function.  In
> DEVELOPER=1 mode, this now causes a compile error.

I just updated to MacOS 13 and have been building without 'DEVELOPER=1' to
work around this error, so thank you so much for fixing it! 

> 
> The `FSEventStreamSetDispatchQueue()` function is conceptually similar
> and is the suggested replacement.  However, there are some subtle
> thread-related differences.
> 
> Previously, the event stream would be processed by the
> `fsm_listen__loop()` thread while it was in the `CFRunLoopRun()`
> method.  (Conceptually, this was a blocking call on the lifetime of
> the event stream where our thread drove the event loop and individual
> events were handled by the `fsevent_callback()`.)
> 
> With the change, a "dispatch queue" is created and FSEvents will be
> processed by a hidden queue-related thread (that calls the
> `fsevent_callback()` on our behalf).  Our `fsm_listen__loop()` thread
> maintains the original blocking model by waiting on a mutex/condition
> variable pair while the hidden thread does all of the work.

It's unfortunate that Apple doesn't seem to have any documentation on how
they'd recommend migrating in either [1] or [2], but your approach sounds
straightforward. Rearranging the patch a bit...

[1] https://developer.apple.com/documentation/coreservices/1447824-fseventstreamschedulewithrunloop
[2] https://developer.apple.com/documentation/coreservices/1444164-fseventstreamsetdispatchqueue

> @@ -490,9 +499,11 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)
>  
>  	data = state->listen_data;
>  
> -	data->rl = CFRunLoopGetCurrent();
> +	pthread_mutex_init(&data->dq_lock, NULL);
> +	pthread_cond_init(&data->dq_finished, NULL);
> +	data->dq = dispatch_queue_create("FSMonitor", NULL);
>  
> -	FSEventStreamScheduleWithRunLoop(data->stream, data->rl, kCFRunLoopDefaultMode);
> +	FSEventStreamSetDispatchQueue(data->stream, data->dq);
>  	data->stream_scheduled = 1;
>  
>  	if (!FSEventStreamStart(data->stream)) {

First, you create the dispatch queue and schedule the stream on it. The docs
mention "If there’s a problem scheduling the stream on the queue, an error
will be returned when you try to start the stream.", and that appears to be
handled by the 'if (!FEventStreamStart(data->stream))' that already exists. 

Looks good.

> @@ -501,7 +512,9 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)
>  	}
>  	data->stream_started = 1;
>  
> -	CFRunLoopRun();
> +	pthread_mutex_lock(&data->dq_lock);
> +	pthread_cond_wait(&data->dq_finished, &data->dq_lock);
> +	pthread_mutex_unlock(&data->dq_lock);
>  
>  	switch (data->shutdown_style) {
>  	case FORCE_ERROR_STOP:
> 

Then, you block on the 'dq_finished' condition variable until it indicates a
shutdown (forced or otherwise). The two causes for that are...

> @@ -379,8 +382,11 @@ force_shutdown:
>  	fsmonitor_batch__free_list(batch);
>  	string_list_clear(&cookie_list, 0);
>  
> +	pthread_mutex_lock(&data->dq_lock);
>  	data->shutdown_style = FORCE_SHUTDOWN;
> -	CFRunLoopStop(data->rl);
> +	pthread_cond_broadcast(&data->dq_finished);
> +	pthread_mutex_unlock(&data->dq_lock);
> +
>  	strbuf_release(&tmp);
>  	return;
>  }

...force shutdown due to a major change in the repo detected in
'fsevent_callback()' (e.g. moving the .git dir)...

> @@ -479,9 +486,11 @@ void fsm_listen__stop_async(struct fsmonitor_daemon_state *state)
>  	struct fsm_listen_data *data;
>  
>  	data = state->listen_data;
> -	data->shutdown_style = SHUTDOWN_EVENT;
>  
> -	CFRunLoopStop(data->rl);
> +	pthread_mutex_lock(&data->dq_lock);
> +	data->shutdown_style = SHUTDOWN_EVENT;
> +	pthread_cond_broadcast(&data->dq_finished);
> +	pthread_mutex_unlock(&data->dq_lock);
>  }
>  

...or, shutdown due to an 'fsm_listen__stop_async()' call from the cleanup
of 'fsmonitor_run_daemon_1()'.

Between those two, all of the possible "stop listener" scenarios seem to be
covered (as they were when using 'CFRunLoopStop()').

> @@ -471,6 +473,11 @@ void fsm_listen__dtor(struct fsmonitor_daemon_state *state)
>  		FSEventStreamRelease(data->stream);
>  	}
>  
> +	if (data->dq)
> +		dispatch_release(data->dq);
> +	pthread_cond_destroy(&data->dq_finished);
> +	pthread_mutex_destroy(&data->dq_lock);
> +
>  	FREE_AND_NULL(state->listen_data);
>  }

And finally, cleanup the queue, lock, and condition var in the destructor.

This all looks good to me. Practically, I'd like to see this merged sooner
rather than later to stop (myself and others working on MacOS 13) needing to
hack some way around the deprecation error, but I'll defer to others more
familiar with FSMonitor if there's anything I didn't spot.

Thanks again!

Ævar Arnfjörð Bjarmason Dec. 2, 2022, 6:02 p.m. UTC | #2

On Fri, Dec 02 2022, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <[e-mail]>
>
> Replace the call to `FSEventStreamScheduleWithRunLoop()` function with
> the suggested `FSEventStreamSetDispatchQueue()` function.
>
> The MacOS version of the builtin FSMonitor feature uses the
> `FSEventStreamScheduleWithRunLoop()` function to drive the event loop
> and process FSEvents from the system.  This routine has now been
> deprecated by Apple.  The MacOS 13 (Ventana) compiler tool chain now
> generates a warning when compiling calls to this function.  In
> DEVELOPER=1 mode, this now causes a compile error.
>
> The `FSEventStreamSetDispatchQueue()` function is conceptually similar
> and is the suggested replacement.  However, there are some subtle
> thread-related differences.
>
> Previously, the event stream would be processed by the
> `fsm_listen__loop()` thread while it was in the `CFRunLoopRun()`
> method.  (Conceptually, this was a blocking call on the lifetime of
> the event stream where our thread drove the event loop and individual
> events were handled by the `fsevent_callback()`.)
>
> With the change, a "dispatch queue" is created and FSEvents will be
> processed by a hidden queue-related thread (that calls the
> `fsevent_callback()` on our behalf).  Our `fsm_listen__loop()` thread
> maintains the original blocking model by waiting on a mutex/condition
> variable pair while the hidden thread does all of the work.

I just skimmed the code change and didn't see anything out of place, but
one thing that's missing about this explanation is:

Ok, it's deprecated, but when was it introduced? I.e. we now presumably
have a hard dependency on a newer API released with a newer version of
OSX?

Is it OK that we're going to throw compilation errors on older versions
that don't have it? What version is that? Is that older or newer than
our oldest supported OSX version in general, or is the plan to support
older OSX, but those users would need to compile without fsmonitor?

Depending on the answers to the above (hopefully in a re-rolled commit
message): Should we patch the bit in config.mak.uname where we do the
OSX version detection? I.e. if we're deprecating an older version anyone
still on it would be much better off with a straight-up "$(error)" from
the Makefile, rather than running into a compilation error, only to find
that we've stopped supporting that older version.

Jeff Hostetler Dec. 2, 2022, 6:37 p.m. UTC | #3

On 12/2/22 1:02 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Dec 02 2022, Jeff Hostetler via GitGitGadget wrote:
> 
>> From: Jeff Hostetler <[e-mail]>
>>
>> Replace the call to `FSEventStreamScheduleWithRunLoop()` function with
>> the suggested `FSEventStreamSetDispatchQueue()` function.
>>
>> The MacOS version of the builtin FSMonitor feature uses the
>> `FSEventStreamScheduleWithRunLoop()` function to drive the event loop
>> and process FSEvents from the system.  This routine has now been
>> deprecated by Apple.  The MacOS 13 (Ventana) compiler tool chain now
>> generates a warning when compiling calls to this function.  In
>> DEVELOPER=1 mode, this now causes a compile error.
>>
>> The `FSEventStreamSetDispatchQueue()` function is conceptually similar
>> and is the suggested replacement.  However, there are some subtle
>> thread-related differences.
>>
>> Previously, the event stream would be processed by the
>> `fsm_listen__loop()` thread while it was in the `CFRunLoopRun()`
>> method.  (Conceptually, this was a blocking call on the lifetime of
>> the event stream where our thread drove the event loop and individual
>> events were handled by the `fsevent_callback()`.)
>>
>> With the change, a "dispatch queue" is created and FSEvents will be
>> processed by a hidden queue-related thread (that calls the
>> `fsevent_callback()` on our behalf).  Our `fsm_listen__loop()` thread
>> maintains the original blocking model by waiting on a mutex/condition
>> variable pair while the hidden thread does all of the work.
> 
> I just skimmed the code change and didn't see anything out of place, but
> one thing that's missing about this explanation is:
> 
> Ok, it's deprecated, but when was it introduced? I.e. we now presumably
> have a hard dependency on a newer API released with a newer version of
> OSX?
> 
> Is it OK that we're going to throw compilation errors on older versions
> that don't have it? What version is that? Is that older or newer than
> our oldest supported OSX version in general, or is the plan to support
> older OSX, but those users would need to compile without fsmonitor?
> 
> Depending on the answers to the above (hopefully in a re-rolled commit
> message): Should we patch the bit in config.mak.uname where we do the
> OSX version detection? I.e. if we're deprecating an older version anyone
> still on it would be much better off with a straight-up "$(error)" from
> the Makefile, rather than running into a compilation error, only to find
> that we've stopped supporting that older version.

Lots of questions here.  Let me take a quick stab at answering them.
 From [1] the old routine was introduced in 10.5 and marked deprecated
in 10.13.  From [2] the new routine was introduced in 10.6.

10.5 (Leopard) was released October 2007.
10.6 (Snow Leopard) was released August 2009.

So the only people that would be affected by this must be running
exactly 10.5, right?  (Those with 10.4 and before don't have either
API and are already broken regardless.)

So, based on the ages of those two Apple releases, I'd like to think
that we're fine just switching over and not having to ifdef-up the
config.mak.uname.  (If it were a more recent change in the OS, then
yeah the answer would be different.)

Thoughts ???

[1] 
https://developer.apple.com/documentation/coreservices/1447824-fseventstreamschedulewithrunloop
[2] 
https://developer.apple.com/documentation/coreservices/1444164-fseventstreamsetdispatchqueue
[3] https://en.wikipedia.org/wiki/Mac_OS_X_Leopard
[4] https://en.wikipedia.org/wiki/Mac_OS_X_Snow_Leopard

Ævar Arnfjörð Bjarmason Dec. 2, 2022, 7:08 p.m. UTC | #4

On Fri, Dec 02 2022, Jeff Hostetler wrote:

> On 12/2/22 1:02 PM, Ævar Arnfjörð Bjarmason wrote:
>> On Fri, Dec 02 2022, Jeff Hostetler via GitGitGadget wrote:
>> 
>>> From: Jeff Hostetler <[e-mail]>
>>>
>>> Replace the call to `FSEventStreamScheduleWithRunLoop()` function with
>>> the suggested `FSEventStreamSetDispatchQueue()` function.
>>>
>>> The MacOS version of the builtin FSMonitor feature uses the
>>> `FSEventStreamScheduleWithRunLoop()` function to drive the event loop
>>> and process FSEvents from the system.  This routine has now been
>>> deprecated by Apple.  The MacOS 13 (Ventana) compiler tool chain now
>>> generates a warning when compiling calls to this function.  In
>>> DEVELOPER=1 mode, this now causes a compile error.
>>>
>>> The `FSEventStreamSetDispatchQueue()` function is conceptually similar
>>> and is the suggested replacement.  However, there are some subtle
>>> thread-related differences.
>>>
>>> Previously, the event stream would be processed by the
>>> `fsm_listen__loop()` thread while it was in the `CFRunLoopRun()`
>>> method.  (Conceptually, this was a blocking call on the lifetime of
>>> the event stream where our thread drove the event loop and individual
>>> events were handled by the `fsevent_callback()`.)
>>>
>>> With the change, a "dispatch queue" is created and FSEvents will be
>>> processed by a hidden queue-related thread (that calls the
>>> `fsevent_callback()` on our behalf).  Our `fsm_listen__loop()` thread
>>> maintains the original blocking model by waiting on a mutex/condition
>>> variable pair while the hidden thread does all of the work.
>> I just skimmed the code change and didn't see anything out of place,
>> but
>> one thing that's missing about this explanation is:
>> Ok, it's deprecated, but when was it introduced? I.e. we now
>> presumably
>> have a hard dependency on a newer API released with a newer version of
>> OSX?
>> Is it OK that we're going to throw compilation errors on older
>> versions
>> that don't have it? What version is that? Is that older or newer than
>> our oldest supported OSX version in general, or is the plan to support
>> older OSX, but those users would need to compile without fsmonitor?
>> Depending on the answers to the above (hopefully in a re-rolled
>> commit
>> message): Should we patch the bit in config.mak.uname where we do the
>> OSX version detection? I.e. if we're deprecating an older version anyone
>> still on it would be much better off with a straight-up "$(error)" from
>> the Makefile, rather than running into a compilation error, only to find
>> that we've stopped supporting that older version.
>
> Lots of questions here.  Let me take a quick stab at answering them.
> From [1] the old routine was introduced in 10.5 and marked deprecated
> in 10.13.  From [2] the new routine was introduced in 10.6.
>
> 10.5 (Leopard) was released October 2007.
> 10.6 (Snow Leopard) was released August 2009.
>
> So the only people that would be affected by this must be running
> exactly 10.5, right?  (Those with 10.4 and before don't have either
> API and are already broken regardless.)
>
> So, based on the ages of those two Apple releases, I'd like to think
> that we're fine just switching over and not having to ifdef-up the
> config.mak.uname.  (If it were a more recent change in the OS, then
> yeah the answer would be different.)
>
> Thoughts ???

That seems reasonable to me, but it came out in 2001, and we'd be moving
the dependency to a 2007 version.

Is that OK? No idea, I don't know how old of an OSX version people
reasonably run & want to compile Git on.

But in 842c9edec64 (fsmonitor: enable fsmonitor for Linux, 2022-11-23)
which is new in this upcoming release we seem to have set that
dependency at 10.4.

Now, you can unset FSMONITOR_DAEMON_BACKEND and FSMONITOR_OS_SETTINGS in
your config.mak.uname, but that's probably something that should be
noted more prominently.

Eric? [CC'd]

Victoria Dye Dec. 2, 2022, 7:51 p.m. UTC | #5

Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Dec 02 2022, Jeff Hostetler wrote:
> 
>> On 12/2/22 1:02 PM, Ævar Arnfjörð Bjarmason wrote:
>> So, based on the ages of those two Apple releases, I'd like to think
>> that we're fine just switching over and not having to ifdef-up the
>> config.mak.uname.  (If it were a more recent change in the OS, then
>> yeah the answer would be different.)
>>
>> Thoughts ???
> 
> That seems reasonable to me, but it came out in 2001, and we'd be moving
> the dependency to a 2007 version.
> 
> Is that OK? No idea, I don't know how old of an OSX version people
> reasonably run & want to compile Git on.

I appreciate the diligence, but I don't think continuing this discussion
will be productive use anyone's time.

Apple doesn't seem to provide official end-of-life dates for their OS
versions, but we can extrapolate from the list of obsolete hardware [1] that
it likely doesn't support OS versions older than 2014; that's corroborated
by their official set of release notes going only as far back as 10.14,
released in 2018). In other words, I think it's safe to say that a version
supplanted in 2009 is old enough to not warrant Git support.

Thanks,
- Victoria

[1] https://support.apple.com/en-us/HT201624
[2] ttps://developer.apple.com/documentation/macos-release-notes

> 
> But in 842c9edec64 (fsmonitor: enable fsmonitor for Linux, 2022-11-23)
> which is new in this upcoming release we seem to have set that
> dependency at 10.4.
> 
> Now, you can unset FSMONITOR_DAEMON_BACKEND and FSMONITOR_OS_SETTINGS in
> your config.mak.uname, but that's probably something that should be
> noted more prominently.
> 
> Eric? [CC'd]

Ævar Arnfjörð Bjarmason Dec. 2, 2022, 8:34 p.m. UTC | #6

On Fri, Dec 02 2022, Victoria Dye wrote:

> Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Fri, Dec 02 2022, Jeff Hostetler wrote:
>> 
>>> On 12/2/22 1:02 PM, Ævar Arnfjörð Bjarmason wrote:
>>> So, based on the ages of those two Apple releases, I'd like to think
>>> that we're fine just switching over and not having to ifdef-up the
>>> config.mak.uname.  (If it were a more recent change in the OS, then
>>> yeah the answer would be different.)
>>>
>>> Thoughts ???
>> 
>> That seems reasonable to me, but it came out in 2001, and we'd be moving
>> the dependency to a 2007 version.
>> 
>> Is that OK? No idea, I don't know how old of an OSX version people
>> reasonably run & want to compile Git on.
>
> I appreciate the diligence, but I don't think continuing this discussion
> will be productive use anyone's time.

It's quite useful in general to know the lower boundaries of what
versions of OS's are supported at all.

For example, we support non-GNU iconv implementations that have some
weird edge cases. As the config.mak.uname shows OLD_ICONV is required
for OSX 10.4 or later. If we know we'd like to draw a hard line at OSX
10.5 we can scratch it off the list of platforms we care about.

Anyway, that larger topic aside. All I'm suggesting here is that the
proposed patch seems to at least soft-deprecate versions of OSX we
supported before. Maybe that's fine, but the commit message didn't get
across whether that was considered, part of the plan etc.

> Apple doesn't seem to provide official end-of-life dates for their OS
> versions, but we can extrapolate from the list of obsolete hardware [1] that
> it likely doesn't support OS versions older than 2014; that's corroborated
> by their official set of release notes going only as far back as 10.14,
> released in 2018). In other words, I think it's safe to say that a version
> supplanted in 2009 is old enough to not warrant Git support.

I wish :)

It may happen to be true for OSX, and I suspect that OS has a relatively
aggressive timeline as OS's go, as opposed to AIX or something, which
people tend to run long past EOL.

But our support for OS versions has neither historically or currently
mirrored EOL dates.

A lot of those are *very* aggressive, e.g. FreeBSD's releases go EOL
around a year or so after release, and we certainly support building on
way older FreeBSD than that:
https://www.freebsd.org/security/unsupported/

For some other in-tree software we're >10 yrs past EOL:
https://lore.kernel.org/git/[e-mail]/

In general I think OS EOL's are most useful as an indicator of what
versions of that OS you'd want to run in some Internet-connected
high-vulnerability scenario.

But git gets ported and backported to a long tail of systems way beyond
that. Eventually we do need to let got, but we've generally drawn the
line at some fuzzy notion of when users don't care anymore, along with
whether it's worth the effort to find out.

Victoria Dye Dec. 2, 2022, 9:17 p.m. UTC | #7

Ævar Arnfjörð Bjarmason wrote:
> Anyway, that larger topic aside. All I'm suggesting here is that the
> proposed patch seems to at least soft-deprecate versions of OSX we
> supported before. Maybe that's fine, but the commit message didn't get
> across whether that was considered, part of the plan etc.

...

> But git gets ported and backported to a long tail of systems way beyond
> that. Eventually we do need to let got, but we've generally drawn the
> line at some fuzzy notion of when users don't care anymore, along with
> whether it's worth the effort to find out.

My point is that such a user for this scenario is so unlikely to exist that
holding up this patch - which provides a real, tangible benefit to
developers *right now* - to implement your suggestion or modify the commit
message is, at best, an unnecessary distraction.

If, somewhere, there is a user that 1) keeps up-to-date with the latest
version of Git, 2) uses FSMonitor, and 3) is working on the sole version of
MacOS that was theoretically compatible with FSMonitor before this change
but now is not, we can accommodate that once such a need is shown to exist.

Stefan Sundin Dec. 2, 2022, 9:44 p.m. UTC | #8

On Fri, Dec 2, 2022 at 6:24 AM Jeff Hostetler via GitGitGadget
<[e-mail]> wrote:
> The MacOS version of the builtin FSMonitor feature uses the
> `FSEventStreamScheduleWithRunLoop()` function to drive the event loop
> and process FSEvents from the system.  This routine has now been
> deprecated by Apple.  The MacOS 13 (Ventana) compiler tool chain now
> generates a warning when compiling calls to this function.  In
> DEVELOPER=1 mode, this now causes a compile error.

Typo here, MacOS 13 is Ventura not Ventana.


On Fri, Dec 2, 2022 at 1:17 PM Victoria Dye <[e-mail]> wrote:
> My point is that such a user for this scenario is so unlikely to exist that
> holding up this patch - which provides a real, tangible benefit to
> developers *right now* - to implement your suggestion or modify the commit
> message is, at best, an unnecessary distraction.
>
> If, somewhere, there is a user that 1) keeps up-to-date with the latest
> version of Git, 2) uses FSMonitor, and 3) is working on the sole version of
> MacOS that was theoretically compatible with FSMonitor before this change
> but now is not, we can accommodate that once such a need is shown to exist.

Looking at config.mak.uname it seems quite easy to keep git working on
old MacOS versions by adding a check like Ævar suggested.

Something like this:

--- a/config.mak.uname
+++ b/config.mak.uname
@@ -161,12 +161,15 @@ ifeq ($(uname_S),Darwin)

        # The builtin FSMonitor on MacOS builds upon Simple-IPC.  Both require
        # Unix domain sockets and PThreads.
+       # FSMonitor on Darwin requires MacOS 10.6 or later.
        ifndef NO_PTHREADS
        ifndef NO_UNIX_SOCKETS
+       ifeq ($(shell test "`expr "$(uname_R)" : '\([0-9][0-9]*\)\.'`"
-ge 10 && echo 1),1)
        FSMONITOR_DAEMON_BACKEND = darwin
        FSMONITOR_OS_SETTINGS = darwin
        endif
        endif
+       endif

        BASIC_LDFLAGS += -framework CoreServices
 endif


Looking at that file it seems like a lot of care has gone into keeping
compatibility with older MacOS versions so in my mind it seems
appropriate to continue that legacy, especially since it is so easy.

Great work Jeff.

Ævar Arnfjörð Bjarmason Dec. 2, 2022, 11:02 p.m. UTC | #9

On Fri, Dec 02 2022, Stefan Sundin wrote:

> On Fri, Dec 2, 2022 at 6:24 AM Jeff Hostetler via GitGitGadget
> <[e-mail]> wrote:
>> The MacOS version of the builtin FSMonitor feature uses the
>> `FSEventStreamScheduleWithRunLoop()` function to drive the event loop
>> and process FSEvents from the system.  This routine has now been
>> deprecated by Apple.  The MacOS 13 (Ventana) compiler tool chain now
>> generates a warning when compiling calls to this function.  In
>> DEVELOPER=1 mode, this now causes a compile error.
>
> Typo here, MacOS 13 is Ventura not Ventana.
>
>
> On Fri, Dec 2, 2022 at 1:17 PM Victoria Dye <[e-mail]> wrote:
>> My point is that such a user for this scenario is so unlikely to exist that
>> holding up this patch - which provides a real, tangible benefit to
>> developers *right now* - to implement your suggestion or modify the commit
>> message is, at best, an unnecessary distraction.
>>
>> If, somewhere, there is a user that 1) keeps up-to-date with the latest
>> version of Git, 2) uses FSMonitor, and 3) is working on the sole version of
>> MacOS that was theoretically compatible with FSMonitor before this change
>> but now is not, we can accommodate that once such a need is shown to exist.
>
> Looking at config.mak.uname it seems quite easy to keep git working on
> old MacOS versions by adding a check like Ævar suggested.
>
> Something like this:
>
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -161,12 +161,15 @@ ifeq ($(uname_S),Darwin)
>
>         # The builtin FSMonitor on MacOS builds upon Simple-IPC.  Both require
>         # Unix domain sockets and PThreads.
> +       # FSMonitor on Darwin requires MacOS 10.6 or later.
>         ifndef NO_PTHREADS
>         ifndef NO_UNIX_SOCKETS
> +       ifeq ($(shell test "`expr "$(uname_R)" : '\([0-9][0-9]*\)\.'`"
> -ge 10 && echo 1),1)
>         FSMONITOR_DAEMON_BACKEND = darwin
>         FSMONITOR_OS_SETTINGS = darwin
>         endif
>         endif
> +       endif
>
>         BASIC_LDFLAGS += -framework CoreServices
>  endif

That looks reasonable, but just to be clear I'm completely neutral on
the question of whether it even makes sense to support versions this
old. I.e. maybe it should just be:

	ifeq [some expression detecting older than OSX <=10.X]
	$(error We do not support building on versions this old, sorry...)
	endif

> Looking at that file it seems like a lot of care has gone into keeping
> compatibility with older MacOS versions so in my mind it seems
> appropriate to continue that legacy, especially since it is so easy.

There's a lot of care for some of it, but some of it's just old build
definitions covered in cobwebs that nobody cares about anymore :)

E.g. there's bits in there to support AIX v1..4, the last v4 came out
before this millenium, and I v1..3 was in the 1980s. Ditto probably
SunOS 5.6 and older (which would be *very* conservative),

Junio C Hamano Dec. 3, 2022, 1:05 a.m. UTC | #10

Victoria Dye <[e-mail]> writes:

>> But git gets ported and backported to a long tail of systems way beyond
>> that. Eventually we do need to let got, but we've generally drawn the
>> line at some fuzzy notion of when users don't care anymore, along with
>> whether it's worth the effort to find out.
>
> My point is that such a user for this scenario is so unlikely to exist that
> holding up this patch - which provides a real, tangible benefit to
> developers *right now* - to implement your suggestion or modify the commit
> message is, at best, an unnecessary distraction.
>
> If, somewhere, there is a user that 1) keeps up-to-date with the latest
> version of Git, 2) uses FSMonitor, and 3) is working on the sole version of
> MacOS that was theoretically compatible with FSMonitor before this change
> but now is not, we can accommodate that once such a need is shown to exist.

I'd still prefer that our commit messages keep records of the fact
that we stopped supporting certain older systems and what kind of
due dilligence we did to decide it is a safe thing to do, which all
already happened in this thread, thanks to you three discussing the
issue.  I would be happier even with "Anything older than 2014 does
not matter to Apple, and we follow that stance" than without any ;-)

Thanks.

Junio C Hamano Dec. 5, 2022, 12:58 a.m. UTC | #11

Junio C Hamano <[e-mail]> writes:

> I'd still prefer that our commit messages keep records of the fact
> that we stopped supporting certain older systems and what kind of
> due dilligence we did to decide it is a safe thing to do, which all
> already happened in this thread, thanks to you three discussing the
> issue.  I would be happier even with "Anything older than 2014 does
> not matter to Apple, and we follow that stance" than without any ;-)

I'd propose to have an extra paragraph at the end of the commit log
message.

1:  02a55477b6 ! 1:  df739b6087 fsmonitor: eliminate call to deprecated FSEventStream function
    @@ Commit message
         maintains the original blocking model by waiting on a mutex/condition
         variable pair while the hidden thread does all of the work.
     
    +    While the deprecated API used by the original were introduced in
    +    macOS 10.5 (Oct 2007), the API used by the updated code were
    +    introduced back in macOS 10.6 (Aug 2009) and has been available
    +    since then.  So this change _could_ break those who have happily
    +    been using 10.5 (if there were such people), but these two dates
    +    both predate the oldest versions of macOS Apple seems to support
    +    anyway, so we should be safe.
    +
         Signed-off-by: Jeff Hostetler <[e-mail]>
         Signed-off-by: Junio C Hamano <[e-mail]>

Jeff Hostetler Dec. 5, 2022, 2:34 p.m. UTC | #12

On 12/4/22 7:58 PM, Junio C Hamano wrote:
> Junio C Hamano <[e-mail]> writes:
> 
>> I'd still prefer that our commit messages keep records of the fact
>> that we stopped supporting certain older systems and what kind of
>> due dilligence we did to decide it is a safe thing to do, which all
>> already happened in this thread, thanks to you three discussing the
>> issue.  I would be happier even with "Anything older than 2014 does
>> not matter to Apple, and we follow that stance" than without any ;-)
> 
> I'd propose to have an extra paragraph at the end of the commit log
> message.
> 
> 1:  02a55477b6 ! 1:  df739b6087 fsmonitor: eliminate call to deprecated FSEventStream function
>      @@ Commit message
>           maintains the original blocking model by waiting on a mutex/condition
>           variable pair while the hidden thread does all of the work.
>       
>      +    While the deprecated API used by the original were introduced in
>      +    macOS 10.5 (Oct 2007), the API used by the updated code were
>      +    introduced back in macOS 10.6 (Aug 2009) and has been available
>      +    since then.  So this change _could_ break those who have happily
>      +    been using 10.5 (if there were such people), but these two dates
>      +    both predate the oldest versions of macOS Apple seems to support
>      +    anyway, so we should be safe.
>      +
>           Signed-off-by: Jeff Hostetler <[e-mail]>
>           Signed-off-by: Junio C Hamano <[e-mail]>
>       

I like this new text.  Let's do this and call it done.

Since I see that you have already added it to the commit message
in the branch, so I won't resubmit it unless there are further
technical review comments to address.

Thanks all,
Jeff

Junio C Hamano Dec. 5, 2022, 11:13 p.m. UTC | #13

Jeff Hostetler <[e-mail]> writes:

> I like this new text.  Let's do this and call it done.

Good. Thanks.

> Since I see that you have already added it to the commit message
> in the branch, so I won't resubmit it unless there are further
> technical review comments to address.
>
> Thanks all,

Thanks for working on this.  Let's mark it for 'next'.  Even though
we'll see -rc2 very soonish, being in 'next' this early would mean
it will be part of the first (if we need brown paper bag fixes) or
the second batch in the next cycle.

Jeff Hostetler Dec. 6, 2022, 5:25 p.m. UTC | #14

On 12/5/22 6:13 PM, Junio C Hamano wrote:
> Jeff Hostetler <[e-mail]> writes:
> 
>> I like this new text.  Let's do this and call it done.
> 
> Good. Thanks.
> 
>> Since I see that you have already added it to the commit message
>> in the branch, so I won't resubmit it unless there are further
>> technical review comments to address.
>>
>> Thanks all,
> 
> Thanks for working on this.  Let's mark it for 'next'.  Even though
> we'll see -rc2 very soonish, being in 'next' this early would mean
> it will be part of the first (if we need brown paper bag fixes) or
> the second batch in the next cycle.
> 


Great! Thanks!  And yes, there's nothing urgent for this fix,
so that would be fine.

Jeff

Patch

diff --git a/compat/fsmonitor/fsm-darwin-gcc.h b/compat/fsmonitor/fsm-darwin-gcc.h
index 1c75c3d48e7..3496e29b3a1 100644
--- a/compat/fsmonitor/fsm-darwin-gcc.h
+++ b/compat/fsmonitor/fsm-darwin-gcc.h
@@ -80,9 +80,7 @@  void CFRunLoopRun(void);
 void CFRunLoopStop(CFRunLoopRef run_loop);
 CFRunLoopRef CFRunLoopGetCurrent(void);
 extern CFStringRef kCFRunLoopDefaultMode;
-void FSEventStreamScheduleWithRunLoop(FSEventStreamRef stream,
-				      CFRunLoopRef run_loop,
-				      CFStringRef run_loop_mode);
+void FSEventStreamSetDispatchQueue(FSEventStreamRef stream, dispatch_queue_t q);
 unsigned char FSEventStreamStart(FSEventStreamRef stream);
 void FSEventStreamStop(FSEventStreamRef stream);
 void FSEventStreamInvalidate(FSEventStreamRef stream);
diff --git a/compat/fsmonitor/fsm-listen-darwin.c b/compat/fsmonitor/fsm-listen-darwin.c
index cc9af1e3cb3..97a55a6f0a4 100644
--- a/compat/fsmonitor/fsm-listen-darwin.c
+++ b/compat/fsmonitor/fsm-listen-darwin.c
@@ -1,4 +1,5 @@ 
 #ifndef __clang__
+#include <dispatch/dispatch.h>
 #include "fsm-darwin-gcc.h"
 #else
 #include <CoreFoundation/CoreFoundation.h>
@@ -38,7 +39,9 @@  struct fsm_listen_data
 
 	FSEventStreamRef stream;
 
-	CFRunLoopRef rl;
+	dispatch_queue_t dq;
+	pthread_cond_t dq_finished;
+	pthread_mutex_t dq_lock;
 
 	enum shutdown_style {
 		SHUTDOWN_EVENT = 0,
@@ -379,8 +382,11 @@  force_shutdown:
 	fsmonitor_batch__free_list(batch);
 	string_list_clear(&cookie_list, 0);
 
+	pthread_mutex_lock(&data->dq_lock);
 	data->shutdown_style = FORCE_SHUTDOWN;
-	CFRunLoopStop(data->rl);
+	pthread_cond_broadcast(&data->dq_finished);
+	pthread_mutex_unlock(&data->dq_lock);
+
 	strbuf_release(&tmp);
 	return;
 }
@@ -441,10 +447,6 @@  int fsm_listen__ctor(struct fsmonitor_daemon_state *state)
 	if (!data->stream)
 		goto failed;
 
-	/*
-	 * `data->rl` needs to be set inside the listener thread.
-	 */
-
 	return 0;
 
 failed:
@@ -471,6 +473,11 @@  void fsm_listen__dtor(struct fsmonitor_daemon_state *state)
 		FSEventStreamRelease(data->stream);
 	}
 
+	if (data->dq)
+		dispatch_release(data->dq);
+	pthread_cond_destroy(&data->dq_finished);
+	pthread_mutex_destroy(&data->dq_lock);
+
 	FREE_AND_NULL(state->listen_data);
 }
 
@@ -479,9 +486,11 @@  void fsm_listen__stop_async(struct fsmonitor_daemon_state *state)
 	struct fsm_listen_data *data;
 
 	data = state->listen_data;
-	data->shutdown_style = SHUTDOWN_EVENT;
 
-	CFRunLoopStop(data->rl);
+	pthread_mutex_lock(&data->dq_lock);
+	data->shutdown_style = SHUTDOWN_EVENT;
+	pthread_cond_broadcast(&data->dq_finished);
+	pthread_mutex_unlock(&data->dq_lock);
 }
 
 void fsm_listen__loop(struct fsmonitor_daemon_state *state)
@@ -490,9 +499,11 @@  void fsm_listen__loop(struct fsmonitor_daemon_state *state)
 
 	data = state->listen_data;
 
-	data->rl = CFRunLoopGetCurrent();
+	pthread_mutex_init(&data->dq_lock, NULL);
+	pthread_cond_init(&data->dq_finished, NULL);
+	data->dq = dispatch_queue_create("FSMonitor", NULL);
 
-	FSEventStreamScheduleWithRunLoop(data->stream, data->rl, kCFRunLoopDefaultMode);
+	FSEventStreamSetDispatchQueue(data->stream, data->dq);
 	data->stream_scheduled = 1;
 
 	if (!FSEventStreamStart(data->stream)) {
@@ -501,7 +512,9 @@  void fsm_listen__loop(struct fsmonitor_daemon_state *state)
 	}
 	data->stream_started = 1;
 
-	CFRunLoopRun();
+	pthread_mutex_lock(&data->dq_lock);
+	pthread_cond_wait(&data->dq_finished, &data->dq_lock);
+	pthread_mutex_unlock(&data->dq_lock);
 
 	switch (data->shutdown_style) {
 	case FORCE_ERROR_STOP: