- The select.c file should not actually be built or linked on Windows; it is only correct for systems that implement fd_set as a bitarray.
- It looks like the regress_et patch is just a matter of setting the line endings correctly in subversion; are there other changes in there too?
- I've got no objection to #include <event-config.h> in event_iocp.h, but I'd like to know if lacking it breaks the compilation. Our first non-system include is event2/util.h, which is supposed to include event-config.h. If it isn't actually working for some reason, I would want to fix that. Is event-config.h doing something that affects the behaviour of one of the system headers included before.
(Also, we should really just remove all of the #ifdef HAVE_CONFIG_H tests; they're from an obsolete long-ago time when we supported building without an event-config.h file. I have no idea if that even works any more.)
- Why is the change from "regress.gen.h" to "test/regress.gen.h" needed? Other files in test include header files in that directory without prefixing them with "test/". I am probably missing something.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
>- The select.c file should not actually be built or linked on Windows; it
>is only correct for systems that implement fd_set as a bitarray.
Is this necessarily the case? It may explain part of the problem I
observed at runtime. On Windows you could sort the known handles
and indirect from the index to the handle - or just avoid trying to
do the set allocation that way (on Windows) since the FD_SET and FD_ISSET
macros seem to be in use and they work fine on Windows, albeit slowly.
Since using sockets only is quite common in practice the inability to
use Windows' crappy select emulator in that case seems unfortunate.
Need some special code for empty sets too of course.
Can I suggest that *all* sources should compile on *all* platforms and
use configured defines to elide themselves if they are inapplicable?
I'm pretty sure it will make life a lot easier overall (not least IDE
users can easily browse source for other platforms).
>- It looks like the regress_et patch is just a matter of setting the line
>endings correctly in subversion; are there other changes in there too?
That went a bit wrong! Not sure why yet. I hooked up to your svn rather
than import your tarball to my svn as a vendor branch, and it may have
been a mistake in retrospect. Its already causing me issues as I pull
patches to assorted VMs for testing. Maybe I should try running a bzr
workspace off that svn workspace and try bzr locally now that bzr has line
ending support. Gotta be worth a learning exercise! ;-)
>- I've got no objection to #include <event-config.h> in event_iocp.h, but
>I'd like to know if lacking it breaks the compilation. Our first
>non-system include is event2/util.h, which is supposed to include
>event-config.h. If it isn't actually working for some reason, I would want
event_iocp.c I think?
Anyway, the problem is that the first system include is:
#include <windows.h>
and that will include winsock.h, not winsock2.h, and for reasons that
I don't understand they seem to be incompatible.
Without it I get:
event_iocp.c
C:\Program Files\Microsoft SDKs\Windows\v7.0\Include\ws2def.h(91) : warning C4005: 'AF_IPX' : macro redefinition
C:\Program Files\Microsoft SDKs\Windows\v7.0\Include\winsock.h(460) : see previous definition of 'AF_IPX'
and then errors relating to structs rather than the warnings relating to
macros.
>(Also, we should really just remove all of the #ifdef HAVE_CONFIG_H tests;
>they're from an obsolete long-ago time when we supported building without
>an event-config.h file. I have no idea if that even works any more.)
If you do this and include the config first, then that would be helpful
and more consistent, and there would be one place to force inclusion of
necessary system headers to pick up types used in libevent headers. On
Windows you need winsock(2).h for the timeval stuff from memory.
>- Why is the change from "regress.gen.h" to "test/regress.gen.h" needed?
>Other files in test include header files in that directory without
>prefixing them with "test/". I am probably missing something.
This relates to building out of tree. You can't do that at the
moment with the rpcgen generating to the source directory. In this
case I used waf which wants to generate files to a build directory,
and other systems do the same. That doesn't necessitate having
test/ on the front per se, but waf will then find the header without
doing a wafscript in the regress directory. I think scons would be
in the same boat, and perhaps cmake too. They can be fixed to pick up
the header with a -I easily enough, so its no big deal either way.
It seems to me that the include structure is a bit of a mess anyway at
the moment wrt include/event2, ., compat/sys and WIN32-Code.
Can it not be rationalised?
----------
BTW I started on OpenSolaris and I immediately ran into a problem in
buffer.c - I have mmap, but MAP_FILE is not portable. (Later: I think
that's the only real issue, given that the guard generation is related
to the path I used rather than the host system)
What operating systems are expected to be supported in libevent 2?
I have another fix for the event_rpcgen relating to the guard gen: on
Solaris I have to convert '-' characters to '_' as well. The issue here
is essectially that the guard is generated from file names and the universe
of valid characters is different.
I think it would be better to just generate an UUID-based guard name and
forget about the original path of the rpc file for this purpose.
----------
Not sure how I'm going to attack FreeBSD or NetBSD at the moment: I've
been using VirtualBox and support for *BSD seems rather poor. I'll see
if I can move my old VMWare Workstation license to my new laptop, and
failing that try VMWare Server.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
misc patches for msvc/windows sdk
Hi, looking this over now.
- The select.c file should not actually be built or linked on Windows; it is only correct for systems that implement fd_set as a bitarray.
- It looks like the regress_et patch is just a matter of setting the line endings correctly in subversion; are there other changes in there too?
- I've got no objection to #include <event-config.h> in event_iocp.h, but I'd like to know if lacking it breaks the compilation. Our first non-system include is event2/util.h, which is supposed to include event-config.h. If it isn't actually working for some reason, I would want to fix that. Is event-config.h doing something that affects the behaviour of one of the system headers included before.
(Also, we should really just remove all of the #ifdef HAVE_CONFIG_H tests; they're from an obsolete long-ago time when we supported building without an event-config.h file. I have no idea if that even works any more.)
- Why is the change from "regress.gen.h" to "test/regress.gen.h" needed? Other files in test include header files in that directory without prefixing them with "test/". I am probably missing something.
>- The select.c file should not actually be built or linked on Windows; it
>is only correct for systems that implement fd_set as a bitarray.
Is this necessarily the case? It may explain part of the problem I
observed at runtime. On Windows you could sort the known handles
and indirect from the index to the handle - or just avoid trying to
do the set allocation that way (on Windows) since the FD_SET and FD_ISSET
macros seem to be in use and they work fine on Windows, albeit slowly.
Since using sockets only is quite common in practice the inability to
use Windows' crappy select emulator in that case seems unfortunate.
Need some special code for empty sets too of course.
Can I suggest that *all* sources should compile on *all* platforms and
use configured defines to elide themselves if they are inapplicable?
I'm pretty sure it will make life a lot easier overall (not least IDE
users can easily browse source for other platforms).
>- It looks like the regress_et patch is just a matter of setting the line
>endings correctly in subversion; are there other changes in there too?
That went a bit wrong! Not sure why yet. I hooked up to your svn rather
than import your tarball to my svn as a vendor branch, and it may have
been a mistake in retrospect. Its already causing me issues as I pull
patches to assorted VMs for testing. Maybe I should try running a bzr
workspace off that svn workspace and try bzr locally now that bzr has line
ending support. Gotta be worth a learning exercise! ;-)
That should be:
Index: test/regress_et.c
--- test/regress_et.c (revision 1311)
+++ test/regress_et.c (working copy)
@@ -29,9 +29,15 @@
#ifdef WIN32
#include <winsock2.h>
#endif
+#ifdef _EVENT_HAVE_SYS_TYPES_H
#include <sys/types.h>
+#endif
+#ifdef _EVENT_HAVE_SYS_STAT_H
#include <sys/stat.h>
+#endif
+#ifdef _EVENT_HAVE_SYS_TIME_H
#include <sys/time.h>
+#endif
#ifdef _EVENT_HAVE_SYS_SOCKET_H
#include <sys/socket.h>
#endif
@@ -39,7 +45,9 @@
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
+#if _EVENT_HAVE_UNISTD_H
#include <unistd.h>
+#endif
#include <errno.h>
#include <event2/event.h>
>- I've got no objection to #include <event-config.h> in event_iocp.h, but
>I'd like to know if lacking it breaks the compilation. Our first
>non-system include is event2/util.h, which is supposed to include
>event-config.h. If it isn't actually working for some reason, I would want
event_iocp.c I think?
Anyway, the problem is that the first system include is:
#include <windows.h>
and that will include winsock.h, not winsock2.h, and for reasons that
I don't understand they seem to be incompatible.
Without it I get:
event_iocp.c
C:\Program Files\Microsoft SDKs\Windows\v7.0\Include\ws2def.h(91) : warning C4005: 'AF_IPX' : macro redefinition
C:\Program Files\Microsoft SDKs\Windows\v7.0\Include\winsock.h(460) : see previous definition of 'AF_IPX'
and then errors relating to structs rather than the warnings relating to
macros.
>(Also, we should really just remove all of the #ifdef HAVE_CONFIG_H tests;
>they're from an obsolete long-ago time when we supported building without
>an event-config.h file. I have no idea if that even works any more.)
If you do this and include the config first, then that would be helpful
and more consistent, and there would be one place to force inclusion of
necessary system headers to pick up types used in libevent headers. On
Windows you need winsock(2).h for the timeval stuff from memory.
>- Why is the change from "regress.gen.h" to "test/regress.gen.h" needed?
>Other files in test include header files in that directory without
>prefixing them with "test/". I am probably missing something.
This relates to building out of tree. You can't do that at the
moment with the rpcgen generating to the source directory. In this
case I used waf which wants to generate files to a build directory,
and other systems do the same. That doesn't necessitate having
test/ on the front per se, but waf will then find the header without
doing a wafscript in the regress directory. I think scons would be
in the same boat, and perhaps cmake too. They can be fixed to pick up
the header with a -I easily enough, so its no big deal either way.
It seems to me that the include structure is a bit of a mess anyway at
the moment wrt include/event2, ., compat/sys and WIN32-Code.
Can it not be rationalised?
----------
BTW I started on OpenSolaris and I immediately ran into a problem in
buffer.c - I have mmap, but MAP_FILE is not portable. (Later: I think
that's the only real issue, given that the guard generation is related
to the path I used rather than the host system)
What operating systems are expected to be supported in libevent 2?
I have another fix for the event_rpcgen relating to the guard gen: on
Solaris I have to convert '-' characters to '_' as well. The issue here
is essectially that the guard is generated from file names and the universe
of valid characters is different.
I think it would be better to just generate an UUID-based guard name and
forget about the original path of the rpc file for this purpose.
----------
Not sure how I'm going to attack FreeBSD or NetBSD at the moment: I've
been using VirtualBox and support for *BSD seems rather poor. I'll see
if I can move my old VMWare Workstation license to my new laptop, and
failing that try VMWare Server.