> From: Junio C Hamano [mailto:[email protected]]
> Sent: Wednesday, August 22, 2012 7:13 PM
> To: Joachim Schmitz
> Cc: [email protected]; Erik Faye-Lund; Johannes Sixt; Marius Storm-Olsen
> Subject: Re: [PATCH] Support non-WIN32 system lacking poll() while keeping
> the WIN32 part intact
>
> "Joachim Schmitz" <[email protected]> writes:
>
> > Signed-off-by: Joachim Schmitz <[email protected]>
> > ---
> > Makefile | 18 ++++++++++++++----
> > compat/win32/poll.c | 8 ++++++--
> > 2 files changed, 20 insertions(+), 6 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 6b0c961..2af4db3 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -152,6 +152,11 @@ all::
> > #
> > # Define NO_MMAP if you want to avoid mmap.
> > #
> > +# Define NO_SYS_POLL_H if you don't have sys/poll.h.
> > +#
> > +# Define NO_POLL if you do not have or do not want to use poll.
> > +# This also implies NO_SYS_POLL_H.
>
> Do you really need to have both? I suspect "If you do not have a usable
> sys/poll.h, set NO_SYS_POLL_H" may be a simpler alternative, but there
must
Hmm, Not having <sys/poll.h> and not having poll() are different thinks,
aren't they?
Using NO_SYS_POL_H so far only affects BASIC_CFLAGS:
ifdef NO_SYS_POLL_H
BASIC_CFLAGS += -DNO_SYS_POLL_H
endif
It does not add compat/win32/poll.c to COMPAT_OBJS, NO_POLL does that.
> be a reason why you had to add a new one instead of going that route. It
would
> be a good idea to describe that reason in the log message above, in the
space
> before your Sign-off.
>
> > @@ -1257,7 +1262,7 @@ ifeq ($(uname_S),Windows)
> > BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild
> > -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H
> > -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE
> > COMPAT_OBJS = compat/msvc.o compat/winansi.o \
> > compat/win32/pthread.o compat/win32/syslog.o \
> > - compat/win32/poll.o compat/win32/dirent.o
> > + compat/win32/dirent.o
> > COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -
> DHAVE_STRING_H
> > ...
> > @@ -1347,7 +1352,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
> > COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
> > COMPAT_OBJS += compat/mingw.o compat/winansi.o \
> > compat/win32/pthread.o compat/win32/syslog.o \
> > - compat/win32/poll.o compat/win32/dirent.o
> > + compat/win32/dirent.o
> > EXTLIBS += -lws2_32
> > ...
> > @@ -1601,6 +1606,11 @@ ifdef NO_GETTEXT ...
> > +ifdef NO_POLL
> > + NO_SYS_POLL_H = YesPlease
> > + COMPAT_CFLAGS += -DNO_POLL -Icompat/win32 # so it find poll.h
> > + COMPAT_OBJS += compat/win32/poll.c endif
>
> In general, I think this is a good direction to go. If the existing
emulation in
> win32/poll.c turns out to be usable across platforms and not windows
specific,
> sharing it would be a good idea.
>
> But if the emulation is no longer windows specific, shouldn't you also
move it
> outside compat/win32/ and somewhere more generic?
Should be possible. Esp. as with the current setup make issues a warning:
Makefile:2329: target `compat/win32/poll.c' doesn't match the target pattern
Haven't yet been able to spot where that comes from.
> > diff --git a/compat/win32/poll.c b/compat/win32/poll.c index
> > 403eaa7..49541f1 100644
> > --- a/compat/win32/poll.c
> > +++ b/compat/win32/poll.c
> > @@ -24,7 +24,9 @@
> > # pragma GCC diagnostic ignored "-Wtype-limits"
> > #endif
> >
> > -#include <malloc.h>
> > +#if defined(WIN32)
> > +# include <malloc.h>
> > +#endif
>
> Hrm, are the Windows folks OK with this? MINGW and MSVC are affected;
> Cygwin should be OK, I think.
I stole that define from another place in git (compat/nedmalloc/nedmalloc.c"
line 36), there too it was used to hide <malloc.h>, so I assumed it to be
the proper method?
> > #include <sys/types.h>
> >
> > @@ -48,7 +50,9 @@
> > #else
> > # include <sys/time.h>
> > # include <sys/socket.h>
> > -# include <sys/select.h>
> > +# ifndef NO_SYS_SELECT_H
> > +# include <sys/select.h>
> > +# endif
> > # include <unistd.h>
> > #endif
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
