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 <jeffhostetler@xxxxxxxxxx> >>> >>> 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]
