| Message ID | 587580489473a7a2ad665bdf3c482ea5d2c54f61.1625150864.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Builtin FSMonitor Feature | expand |
On Thu, Jul 01 2021, Jeff Hostetler via GitGitGadget wrote: > From: Jeff Hostetler <jeffhost@microsoft.com> > > Stub in empty implementation of fsmonitor--daemon > backend for MacOS. > > Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> > --- > compat/fsmonitor/fsmonitor-fs-listen-macos.c | 20 ++++++++++++++++++++ > config.mak.uname | 2 ++ > contrib/buildsystems/CMakeLists.txt | 3 +++ > 3 files changed, 25 insertions(+) > create mode 100644 compat/fsmonitor/fsmonitor-fs-listen-macos.c > > diff --git a/compat/fsmonitor/fsmonitor-fs-listen-macos.c b/compat/fsmonitor/fsmonitor-fs-listen-macos.c > new file mode 100644 > index 00000000000..b91058d1c4f > --- /dev/null > +++ b/compat/fsmonitor/fsmonitor-fs-listen-macos.c > @@ -0,0 +1,20 @@ > +#include "cache.h" > +#include "fsmonitor.h" > +#include "fsmonitor-fs-listen.h" > + > +int fsmonitor_fs_listen__ctor(struct fsmonitor_daemon_state *state) > +{ > + return -1; > +} > + > +void fsmonitor_fs_listen__dtor(struct fsmonitor_daemon_state *state) > +{ > +} > + > +void fsmonitor_fs_listen__stop_async(struct fsmonitor_daemon_state *state) > +{ > +} > + > +void fsmonitor_fs_listen__loop(struct fsmonitor_daemon_state *state) > +{ > +} > diff --git a/config.mak.uname b/config.mak.uname > index fcd88b60b14..394355463e1 100644 > --- a/config.mak.uname > +++ b/config.mak.uname > @@ -147,6 +147,8 @@ ifeq ($(uname_S),Darwin) > MSGFMT = /usr/local/opt/gettext/bin/msgfmt > endif > endif > + FSMONITOR_DAEMON_BACKEND = macos A rather trivial point, but can't we pick one of "macos" or "darwin" (I'd think going with the existing uname is better) and name the file after the uname (or lower-case thereof)? Makes these make rules more consistent too, we could just set this to "YesPlease" here, and then lower case the uname for the file compilation/include.
Hi Ævar, On Fri, 2 Jul 2021, Ævar Arnfjörð Bjarmason wrote: > On Thu, Jul 01 2021, Jeff Hostetler via GitGitGadget wrote: > > > From: Jeff Hostetler <jeffhost@microsoft.com> > > > > Stub in empty implementation of fsmonitor--daemon > > backend for MacOS. > > > > Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> > > --- > > compat/fsmonitor/fsmonitor-fs-listen-macos.c | 20 ++++++++++++++++++++ > > config.mak.uname | 2 ++ > > contrib/buildsystems/CMakeLists.txt | 3 +++ > > 3 files changed, 25 insertions(+) > > create mode 100644 compat/fsmonitor/fsmonitor-fs-listen-macos.c > > > > diff --git a/compat/fsmonitor/fsmonitor-fs-listen-macos.c b/compat/fsmonitor/fsmonitor-fs-listen-macos.c > > new file mode 100644 > > index 00000000000..b91058d1c4f > > --- /dev/null > > +++ b/compat/fsmonitor/fsmonitor-fs-listen-macos.c > > @@ -0,0 +1,20 @@ > > +#include "cache.h" > > +#include "fsmonitor.h" > > +#include "fsmonitor-fs-listen.h" > > + > > +int fsmonitor_fs_listen__ctor(struct fsmonitor_daemon_state *state) > > +{ > > + return -1; > > +} > > + > > +void fsmonitor_fs_listen__dtor(struct fsmonitor_daemon_state *state) > > +{ > > +} > > + > > +void fsmonitor_fs_listen__stop_async(struct fsmonitor_daemon_state *state) > > +{ > > +} > > + > > +void fsmonitor_fs_listen__loop(struct fsmonitor_daemon_state *state) > > +{ > > +} > > diff --git a/config.mak.uname b/config.mak.uname > > index fcd88b60b14..394355463e1 100644 > > --- a/config.mak.uname > > +++ b/config.mak.uname > > @@ -147,6 +147,8 @@ ifeq ($(uname_S),Darwin) > > MSGFMT = /usr/local/opt/gettext/bin/msgfmt > > endif > > endif > > + FSMONITOR_DAEMON_BACKEND = macos > > A rather trivial point, but can't we pick one of "macos" or "darwin" > (I'd think going with the existing uname is better) and name the file > after the uname (or lower-case thereof)? > > Makes these make rules more consistent too, we could just set this to > "YesPlease" here, and then lower case the uname for the file > compilation/include. So you suggest that we name the new stuff after an `uname` that reflects a name that is no longer relevant? I haven't seen a real Darwin system in quite a long time, have you? I don't find such a suggestion constructive, I have to admit. Ciao, Johannes
On Fri, Jul 16 2021, Johannes Schindelin wrote: > Hi Ævar, > > On Fri, 2 Jul 2021, Ævar Arnfjörð Bjarmason wrote: > >> On Thu, Jul 01 2021, Jeff Hostetler via GitGitGadget wrote: >> >> > From: Jeff Hostetler <jeffhost@microsoft.com> >> > >> > Stub in empty implementation of fsmonitor--daemon >> > backend for MacOS. >> > >> > Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> >> > --- >> > compat/fsmonitor/fsmonitor-fs-listen-macos.c | 20 ++++++++++++++++++++ >> > config.mak.uname | 2 ++ >> > contrib/buildsystems/CMakeLists.txt | 3 +++ >> > 3 files changed, 25 insertions(+) >> > create mode 100644 compat/fsmonitor/fsmonitor-fs-listen-macos.c >> > >> > diff --git a/compat/fsmonitor/fsmonitor-fs-listen-macos.c b/compat/fsmonitor/fsmonitor-fs-listen-macos.c >> > new file mode 100644 >> > index 00000000000..b91058d1c4f >> > --- /dev/null >> > +++ b/compat/fsmonitor/fsmonitor-fs-listen-macos.c >> > @@ -0,0 +1,20 @@ >> > +#include "cache.h" >> > +#include "fsmonitor.h" >> > +#include "fsmonitor-fs-listen.h" >> > + >> > +int fsmonitor_fs_listen__ctor(struct fsmonitor_daemon_state *state) >> > +{ >> > + return -1; >> > +} >> > + >> > +void fsmonitor_fs_listen__dtor(struct fsmonitor_daemon_state *state) >> > +{ >> > +} >> > + >> > +void fsmonitor_fs_listen__stop_async(struct fsmonitor_daemon_state *state) >> > +{ >> > +} >> > + >> > +void fsmonitor_fs_listen__loop(struct fsmonitor_daemon_state *state) >> > +{ >> > +} >> > diff --git a/config.mak.uname b/config.mak.uname >> > index fcd88b60b14..394355463e1 100644 >> > --- a/config.mak.uname >> > +++ b/config.mak.uname >> > @@ -147,6 +147,8 @@ ifeq ($(uname_S),Darwin) >> > MSGFMT = /usr/local/opt/gettext/bin/msgfmt >> > endif >> > endif >> > + FSMONITOR_DAEMON_BACKEND = macos >> >> A rather trivial point, but can't we pick one of "macos" or "darwin" >> (I'd think going with the existing uname is better) and name the file >> after the uname (or lower-case thereof)? >> >> Makes these make rules more consistent too, we could just set this to >> "YesPlease" here, and then lower case the uname for the file >> compilation/include. > > So you suggest that we name the new stuff after an `uname` that reflects a > name that is no longer relevant? I haven't seen a real Darwin system in > quite a long time, have you? It's not current? On an Mac Mini M1 which got released this year: % uname -s Darwin We then have the same in config.mak.uname, it seemed the most obvious and consistent to carry that through to file inclusion.
Hi Ævar, On Fri, 16 Jul 2021, Ævar Arnfjörð Bjarmason wrote: > On Fri, Jul 16 2021, Johannes Schindelin wrote: > > > So you suggest that we name the new stuff after an `uname` that > > reflects a name that is no longer relevant? I haven't seen a real > > Darwin system in quite a long time, have you? > > It's not current? On an Mac Mini M1 which got released this year: > > % uname -s > Darwin > > We then have the same in config.mak.uname, it seemed the most obvious > and consistent to carry that through to file inclusion. Sorry. I assumed that you knew that Darwin was the name for an open source Operating System. See https://en.wikipedia.org/wiki/Darwin_%28operating_system%29 for more details. Ciao, Johannes
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Hi Ævar, > > On Fri, 16 Jul 2021, Ævar Arnfjörð Bjarmason wrote: > >> On Fri, Jul 16 2021, Johannes Schindelin wrote: >> >> > So you suggest that we name the new stuff after an `uname` that >> > reflects a name that is no longer relevant? I haven't seen a real >> > Darwin system in quite a long time, have you? >> >> It's not current? On an Mac Mini M1 which got released this year: >> >> % uname -s >> Darwin >> >> We then have the same in config.mak.uname, it seemed the most obvious >> and consistent to carry that through to file inclusion. > > Sorry. I assumed that you knew that Darwin was the name for an open source > Operating System. See > https://en.wikipedia.org/wiki/Darwin_%28operating_system%29 for more > details. > > Ciao, > Johannes Sorry, but I do not see that you are being more constructive than the other party, whom you blame to be not constructive, in this exchange. The part of the file that the patch applies to uses $(uname_S) to implement platform specific special cases, and we are looking at ifeq ($(uname_S),Darwin) ... FSMONITOR_DAEMON_BACKEND = macos ... endif I find it a fair question why the name used there has to be different from the one we can automatically and mechanically get out of "uname -s". Then you respond that uname output is no longer relevant because Darwin is a name that is no longer relevant? And when asked why the name is no longer relevant, you make a sniding comment implying that the other party does not know the name is an operating system? What is going on here? It does not really matter how "Darwin" is described in an encyclopedia in the context of this discussion. What matters is that it is what the system's "uname -s" currently uses to identify itself, and what we guard the section of makefile snippet with, isn't it? ci/lib.sh seems to have an attempt to unify/translate among these names, and * on azure-pipelines, it wants to translate darwin to osx * on github-actions, it wants to translate macos to osx Presumably that is because these two systems call the platform with these two different names, and you want to pick a middle ground that nobody uses to be neutral, or something? Also, in contrib/vscode/init.sh, I see Darwin obtained from "uname -s" gets translated to "macOS". In any case, if your argument was "we picked macos because we use the same token elsewhere, while trying to translate away from Darwin as much as possible for such and such reasons", I would have found it a productive exchange, but unfortunately that is not what I am seeing here.
On 7/26/21 7:26 PM, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > >> Hi Ævar, >> >> On Fri, 16 Jul 2021, Ævar Arnfjörð Bjarmason wrote: >> >>> On Fri, Jul 16 2021, Johannes Schindelin wrote: >>> ... I'm not sure that there is a "correct" answer here, but for the sake of harmony, in V4 I'll set this to "darwin" and update the name of the backend driver source file to match. So that we are consistently using 1 term throughout "Makefile" and "config.mak.uname". ifeq ($(uname_S),Darwin) ... FSMONITOR_DAEMON_BACKEND = darwin endif FWIW, I suspect that it is not worth the effort to directly set the backend name from $(uname_S). For example, on Windows we currently have two different uname values depending on which compiler is being used. ifeq ($(uname_S),Windows) ... FSMONITOR_DAEMON_BACKEND = win32 endif ifneq (,$(findstring MINGW,$(uname_S))) ... FSMONITOR_DAEMON_BACKEND = win32 endif Also, since the backend layer is highly platform-specific, it may be a while (if ever) before we have universal coverage for all platforms. Until then, we can simply set $FSMONITOR_DAEMON_BACKEND to a literal value on a platform-by-platform basis as support is added. Thanks, Jeff
On Tue, Jul 27 2021, Jeff Hostetler wrote: > On 7/26/21 7:26 PM, Junio C Hamano wrote: >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> >>> Hi Ævar, >>> >>> On Fri, 16 Jul 2021, Ævar Arnfjörð Bjarmason wrote: >>> >>>> On Fri, Jul 16 2021, Johannes Schindelin wrote: >>>> > ... > > > I'm not sure that there is a "correct" answer here, but for the sake > of harmony, in V4 I'll set this to "darwin" and update the name of > the backend driver source file to match. So that we are consistently > using 1 term throughout "Makefile" and "config.mak.uname". > > ifeq ($(uname_S),Darwin) > ... > FSMONITOR_DAEMON_BACKEND = darwin > endif > > > > FWIW, I suspect that it is not worth the effort to directly set the > backend name from $(uname_S). For example, on Windows we currently have > two different uname values depending on which compiler is being used. > > ifeq ($(uname_S),Windows) > ... > FSMONITOR_DAEMON_BACKEND = win32 > endif > > ifneq (,$(findstring MINGW,$(uname_S))) > ... > FSMONITOR_DAEMON_BACKEND = win32 > endif > > > Also, since the backend layer is highly platform-specific, it may be > a while (if ever) before we have universal coverage for all platforms. > Until then, we can simply set $FSMONITOR_DAEMON_BACKEND to a literal > value on a platform-by-platform basis as support is added. Re "harmony": For what it's worth I don't think you should change it on my accord. I should probably have more explicitly said (but I've also been trying to check the general verbosity of my E-Mails), that when I read a series like this and have some general trivial comments like this, I mean them as something like: Just a thought while reading this through, i.e. a person familiar with the general codebase but not necessarily your specific are. Maybe this suggestion makes things easier/simpler, but if you think not and decide not to take the suggestion that's fine too. I.e. that along with the general implicit suggestion that I'd say applies in general on list that if someone is perplexed by a patch by default that's a comment on the commit message. That person (i.e. me in this case) could also just be hopelessly confused & nothing needs to change. When I get comments like that I sometimes change things, sometimes not. You should do the same. As noted in another reply on this general thread & what's cooking I seem to have poked a bit of a hornet's nest here that I wasn't expecting to poke. I'd not been following earlier rounds of this topic, and didn't know that it had (seemingly) reached some phase of critical updates only in the minds of its authors.
Jeff Hostetler <git@jeffhostetler.com> writes: > I'm not sure that there is a "correct" answer here, but for the sake > of harmony, in V4 I'll set this to "darwin" and update the name of > the backend driver source file to match. So that we are consistently > using 1 term throughout "Makefile" and "config.mak.uname". > > ifeq ($(uname_S),Darwin) > ... > FSMONITOR_DAEMON_BACKEND = darwin > endif > ... I do not think it would help "harmony" to change the name, but any one of 'darwin', 'macos' and 'osx' would be fine. It was irritating to see a simple "why is this particular name chosen?" answered with such a hostility, when even a "we have no deep reasoning behind the choice of the name. It is only seen by developers in names of the source files, and it does not make much difference" would have been sufficient. I somehow view it more problematic. I guess the blame goes both ways, though. We all have worked with each other long enough to know which of your recipients are prone to go overly defensive when asked questions, and we should know that it would help to prepend "I am just being curious but..." to your questions whose answers do not make a huge difference at the end either way (or not asking such questions at all). Thanks.
Junio C Hamano wrote: > I guess the blame goes both ways, though. We all have worked with > each other long enough to know which of your recipients are prone to > go overly defensive when asked questions, and we should know that it > would help to prepend "I am just being curious but..." to your > questions whose answers do not make a huge difference at the end > either way (or not asking such questions at all). I disagree. It's not OK for contributors to react defensively when asked questions, and in particular I don't think it's OK that some contributors are punished for merely disagreeing, while others are given a pass for snarling at feedback. This is double standards. The Git project should not play favorites, and all contributors should be asked to assume good faith. https://en.wikipedia.org/wiki/Wikipedia:Assume_good_faith It should be implied that the feedback given is to try to improve the project, it should not need to be stated.
diff --git a/compat/fsmonitor/fsmonitor-fs-listen-macos.c b/compat/fsmonitor/fsmonitor-fs-listen-macos.c new file mode 100644 index 00000000000..b91058d1c4f --- /dev/null +++ b/compat/fsmonitor/fsmonitor-fs-listen-macos.c @@ -0,0 +1,20 @@ +#include "cache.h" +#include "fsmonitor.h" +#include "fsmonitor-fs-listen.h" + +int fsmonitor_fs_listen__ctor(struct fsmonitor_daemon_state *state) +{ + return -1; +} + +void fsmonitor_fs_listen__dtor(struct fsmonitor_daemon_state *state) +{ +} + +void fsmonitor_fs_listen__stop_async(struct fsmonitor_daemon_state *state) +{ +} + +void fsmonitor_fs_listen__loop(struct fsmonitor_daemon_state *state) +{ +} diff --git a/config.mak.uname b/config.mak.uname index fcd88b60b14..394355463e1 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -147,6 +147,8 @@ ifeq ($(uname_S),Darwin) MSGFMT = /usr/local/opt/gettext/bin/msgfmt endif endif + FSMONITOR_DAEMON_BACKEND = macos + BASIC_LDFLAGS += -framework CoreServices endif ifeq ($(uname_S),SunOS) NEEDS_SOCKET = YesPlease diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt index 1ab94eb284f..aa80671045a 100644 --- a/contrib/buildsystems/CMakeLists.txt +++ b/contrib/buildsystems/CMakeLists.txt @@ -266,6 +266,9 @@ endif() if(CMAKE_SYSTEM_NAME STREQUAL "Windows") add_compile_definitions(HAVE_FSMONITOR_DAEMON_BACKEND) list(APPEND compat_SOURCES compat/fsmonitor/fsmonitor-fs-listen-win32.c) +elseif(CMAKE_SYSTEM_NAME STREQUAL "Darwin") + add_compile_definitions(HAVE_FSMONITOR_DAEMON_BACKEND) + list(APPEND compat_SOURCES compat/fsmonitor/fsmonitor-fs-listen-macos.c) endif() set(EXE_EXTENSION ${CMAKE_EXECUTABLE_SUFFIX})