Re: [PATCH] fsmonitor: eliminate call to deprecated FSEventStream function

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 2, 2022, 23:02 UTC
Message-ID: <221203.86tu2de6x4.gmgdl@evledraar.gmail.com>
In-Reply-To: <CAMDaVWGKx3YMuzRvqGTvvcwh1y9CotFtOCpyAzm=QW3YLy5ROg@mail.gmail.com>

On Fri, Dec 02 2022, Stefan Sundin wrote:

Show 47 quoted lines

> On Fri, Dec 2, 2022 at 6:24 AM Jeff Hostetler via GitGitGadget
> <gitgitgadget@gmail.com> 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 <vdye@github.com> 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),