fsmonitor: eliminate call to deprecated FSEventStream function

PATCH Superseded Jeff Hostetler via GitGitGadget 6 participants 17 messages Dec 2, 2022 – Dec 15, 2022

lore

Share & tools

lore

Jeff Hostetler via GitGitGadget

author

2022-12-02 14:24 UTC #0 lore raw

PATCH 1/1 fsmonitor: eliminate call to deprecated FSEventStream function

From: Jeff Hostetler <[email protected]>

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 <[email protected]>

---
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(-)

compat/fsmonitor/fsm-darwin-gcc.h +1 -3

index 1c75c3d48e7..3496e29b3a1 100644
@@ -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);

compat/fsmonitor/fsm-listen-darwin.c +24 ~~-11~~

index cc9af1e3cb3..97a55a6f0a4 100644
@@ -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:

base-commit: 7452749a781d84244ecd08c6f6ca7e5df67dfce8

Signature · 1 line

gitgitgadget

Victoria Dye

2022-12-02 17:42 UTC #1 lore raw

Quoted 11 lines · Jeff Hostetler via GitGitGadget

From: Jeff Hostetler <[email protected]>
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!

Quoted 11 lines

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().)

Quoted 5 lines

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

Quoted 4 lines

compat/fsmonitor/fsm-listen-darwin.c +0 -0

@@ -490,9 +499,11 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)
 
 	data = state->listen_data;

compat/fsmonitor/fsm-listen-darwin.c void fsm_listen__loop(struct fsmonitor_daemon_state *state) · line 509

Quoted 10 lines

compat/fsmonitor/fsm-listen-darwin.c +4 -2

-	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.

compat/fsmonitor/fsm-listen-darwin.c void fsm_listen__loop(struct fsmonitor_daemon_state *state) · line 520

Quoted 12 lines

compat/fsmonitor/fsm-listen-darwin.c +3 -1

@@ -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...

compat/fsmonitor/fsm-listen-darwin.c force_shutdown: · line 392

Quoted 13 lines

compat/fsmonitor/fsm-listen-darwin.c +4 -1

@@ -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)...

compat/fsmonitor/fsm-listen-darwin.c void fsm_listen__stop_async(struct fsmonitor_daemon_state *state) · line 495

Quoted 13 lines

compat/fsmonitor/fsm-listen-darwin.c +4 -2

@@ -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()').

compat/fsmonitor/fsm-listen-darwin.c void fsm_listen__dtor(struct fsmonitor_daemon_state *state) · line 482

Quoted 11 lines

compat/fsmonitor/fsm-listen-darwin.c +5 -0

@@ -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

2022-12-02 18:06 UTC #2 lore raw

In reply to Jeff Hostetler via GitGitGadget

Quoted 22 lines · Jeff Hostetler via GitGitGadget

From: Jeff Hostetler <[email protected]>
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().)

Quoted 5 lines

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

2022-12-02 18:39 UTC #3 lore raw

Quoted 43 lines · Ævar Arnfjörð Bjarmason

Quoted 22 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
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().)
Quoted 5 lines
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?

Quoted 6 lines

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

2022-12-02 19:11 UTC #4 lore raw

Quoted 61 lines · Jeff Hostetler

Quoted 40 lines · Ævar Arnfjörð Bjarmason
Quoted 22 lines · Jeff Hostetler via GitGitGadget
From: Jeff Hostetler <[email protected]>
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().)
Quoted 5 lines
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
Quoted 8 lines
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.)

Quoted 6 lines

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

2022-12-02 19:51 UTC #5 lore raw

Quoted 9 lines · Ævar Arnfjörð Bjarmason

Quoted 6 lines · Jeff Hostetler
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.)

Quoted 7 lines

Quoted 1 line
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

Quoted 10 lines

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

2022-12-02 20:51 UTC #6 lore raw

Quoted 12 lines · Victoria Dye

Quoted 11 lines · Ævar Arnfjörð Bjarmason
Quoted 7 lines · Jeff Hostetler
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 ???

Quoted 8 lines

Quoted 5 lines
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.

Quoted 6 lines

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/[email protected]/

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

2022-12-02 21:17 UTC #7 lore raw

Quoted 4 lines · Ævar Arnfjörð Bjarmason

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.

...

Quoted 4 lines

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

2022-12-02 21:45 UTC #8 lore raw

Quoted 6 lines · Jeff Hostetler via GitGitGadget

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.

Quoted 9 lines · Victoria Dye

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:

config.mak.uname +3 -1

@@ -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

2022-12-02 23:13 UTC #9 lore raw

Quoted 37 lines · Stefan Sundin

Quoted 6 lines · Jeff Hostetler via GitGitGadget
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.
Quoted 9 lines · Victoria Dye
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:
config.mak.uname +1 -0
@@ -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

config.mak.uname ifeq ($(uname_S),Darwin)

Quoted 10 lines

config.mak.uname +2 -1

+       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

Quoted 3 lines

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

2022-12-03 01:05 UTC #10 lore raw

In reply to Victoria Dye

Quoted 10 lines · Victoria Dye

Quoted 4 lines
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.

Quoted 4 lines

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

2022-12-05 00:58 UTC #11 lore raw

Quoted 6 lines · Junio C Hamano

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 <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

Jeff Hostetler

2022-12-05 14:34 UTC #12 lore raw

Quoted 25 lines · Junio C Hamano

Quoted 6 lines · Junio C Hamano
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.
+

Quoted 3 lines

Signed-off-by Jeff Hostetler <[email protected]>
Signed-off-by Junio C Hamano <[email protected]>

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

2022-12-05 23:14 UTC #13 lore raw

Quoted 1 line · Jeff Hostetler

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

Good. Thanks.

Quoted 5 lines

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

2022-12-06 17:25 UTC #14 lore raw

Quoted 10 lines · Junio C Hamano

Quoted 1 line · Jeff Hostetler
I like this new text. Let's do this and call it done.
Good. Thanks.
Quoted 4 lines
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.

Quoted 7 lines

Quoted 1 line
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

Jeff Hostetler via GitGitGadget

author

2022-12-14 19:12 UTC #15 lore raw

PATCH v2 1/1 fsmonitor: eliminate call to deprecated FSEventStream function

In reply to Jeff Hostetler via GitGitGadget

From: Jeff Hostetler <[email protected]>

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 (Ventura) 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.

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 <[email protected]>

---
fsmonitor: eliminate call to deprecated FSEventStream function

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

Range-diff vs v1:

1: 9000f6f9073 ! 1: 9ef89416a7b fsmonitor: eliminate call to deprecated FSEventStream function
@@ Commit message
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
+ deprecated by Apple. The MacOS 13 (Ventura) compiler tool chain now
generates a warning when compiling calls to this function. In DEVELOPER=1 mode, this now causes a compile error.

@@ Commit message
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 <[email protected]>

## compat/fsmonitor/fsm-darwin-gcc.h ##

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

compat/fsmonitor/fsm-darwin-gcc.h +1 -3

index 1c75c3d48e7..3496e29b3a1 100644
@@ -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);

compat/fsmonitor/fsm-listen-darwin.c +24 ~~-11~~

index cc9af1e3cb3..97a55a6f0a4 100644
@@ -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:

base-commit: 7452749a781d84244ecd08c6f6ca7e5df67dfce8

Signature · 1 line

gitgitgadget

Junio C Hamano

2022-12-15 00:13 UTC #16 lore raw

Quoted 11 lines · Jeff Hostetler via GitGitGadget

From: Jeff Hostetler <[email protected]>
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 (Ventura) compiler tool chain now generates a warning when compiling calls to this function. In DEVELOPER=1 mode, this now causes a compile error.

Thanks, will requeue.