Base port to Windows #2810

SeanCampbell · 2018-06-08T00:21:09Z

No description provided.

SeanCampbell · 2018-06-08T00:25:33Z

@igorpeshansky FYI
@tokkee Can you take a look? I tried to keep it small, but let me know if I should break this into smaller PRs.

rubenk · 2018-06-11T21:20:58Z

Makefile.am

@@ -228,7 +254,7 @@ collectd_SOURCES = \


 collectd_CFLAGS = $(AM_CFLAGS)
-collectd_CPPFLAGS = $(AM_CPPFLAGS)
+collectd_CPPFLAGS = $(AM_CPPFLAGS) $(LTDLINCL)


what does this variable do? if it's for libltdl, we don't use that anymore.

Yup, I copied this over from an old commit and didn't realize it wasn't needed anymore. Removed.

rubenk · 2018-06-11T21:25:00Z

src/daemon/common.c

+    if (recv(fd, buffer, sizeof(buffer),
+             MSG_PEEK
+#ifndef WIN32
+                 | MSG_DONTWAIT


this looks a bit ugly, how about defining MSG_DONTWAIT to 0 for win32 like

#ifdef WIN32 #define MSG_DONTWAIT 0 #endif

and then keeping the recv call unchanged?

rubenk · 2018-06-11T21:25:55Z

src/daemon/plugin.h

 #include <pthread.h>

+#ifdef WIN32
+#undef ERROR


can you add a comment as to why you do this?

rubenk · 2018-06-11T21:28:30Z

src/libcollectdclient/client.c

+#include <winsock2.h>
+#else
+#include <sys/socket.h>
+#include <sys/un.h>


moving these includes up will probably break some platform like OpenBSD. See 34471c0 for an example.

Ah ok. Moved them down. gnulib_config.h needs to be before config.h, so I left the Windows includes at the top.

rubenk · 2018-06-11T21:30:42Z

src/libcollectdclient/client.c

+  struct addrinfo ai_hints = {
+      .ai_family = AF_UNSPEC, .ai_flags = 0, .ai_socktype = SOCK_STREAM};
+#ifdef AI_ADDRCONFIG
+  ai_hints.ai_flags = AI_ADDRCONFIG;


could you define AI_ADDRCONFIG to 0 if not previously defined? that would clean this up nicely.

rubenk · 2018-06-11T21:31:51Z

src/libcollectdclient/collectd/network.h

@@ -60,7 +64,7 @@ int lcc_server_destroy(lcc_network_t *net, lcc_server_t *srv);

 /* Configure servers */
 int lcc_server_set_ttl(lcc_server_t *srv, uint8_t ttl);
-int lcc_server_set_interface(lcc_server_t *srv, char const *interface);
+int lcc_server_set_interface(lcc_server_t *srv, char const *interface_);


I take it interface shadows something? How about calling it iface instead?

rubenk · 2018-06-11T21:34:32Z

build.sh

+    if [ -d "_gnulib" ]; then
+        echo "Assuming that gnulib is already built, because _gnulib exists."
+    else
+        git clone git://git.savannah.gnu.org/gnulib.git


Ouch, cloning a git repo into a git repo, I'm not sure how I feel about this yet ;-) Won't git submodules work?

Hm, I hadn't heard of that before. I'll check this out.

That works really nicely here; updated. Thanks for pointing this out.

rubenk · 2018-06-11T21:35:02Z

build.sh

+          strtok_r \
+          poll \
+          recv \
+          net_if


could you sort this list?

rubenk · 2018-06-11T21:36:13Z

build.sh

+    && aclocal -I /usr/share/aclocal \
+    && $libtoolize --copy --force \
+    && automake --add-missing --copy \
+    && autoconf


is there no autoreconf on cygwin that can replace the above 5 lines?

I was following how it builds it for other platforms, but autoreconf seems to work just as well and is simpler :)

Ah indeed! We should probably just do that for all platforms then. Care to do a separate PR for that?

rubenk · 2018-06-11T21:37:53Z

configure.ac

@@ -99,6 +99,10 @@ case $host_os in
    AC_DEFINE([KERNEL_SOLARIS], [1], [True if program is to be compiled for a Solaris kernel])
    ac_system="Solaris"
    ;;
+  *mingw32*)
+    AC_DEFINE([KERNEL_WIN32], [1], [True if program is to be compiled for a Windows kernel])


this define looks unused in this PR, is that correct?

Correct, it's not used in this PR, but I use it in some plugins I ported (in follow-up PRs). Would you prefer I wait to define it until it's used?

Thanks for the explanation. No problem, just leave it in.

rubenk · 2018-06-11T21:38:49Z

src/daemon/cmd_windows.c

+  int err;
+  wVersionRequested = MAKEWORD(2, 2);
+
+  err = WSAStartup(wVersionRequested, &wsaData);


could you combine this with line 31? We generally prefer C99-style declarations for newer code.

rubenk · 2018-06-11T21:40:26Z

src/daemon/cmd_windows.c

+
+  err = WSAStartup(wVersionRequested, &wsaData);
+  if (err != 0) {
+    printf("WSAStartup failed with error: %d\n", err);


does windows have a stderr to log this to?

Yeah, this seems to work fine using ERROR as is the case with other platforms.

SeanCampbell · 2018-06-12T22:26:42Z

Thanks for checking this out and for all your suggestions. As far as CI, I'm not very familiar with the space, but I saw a few places that recommended AppVeyor as the Travis alternative for Windows, and it looks like it also has free CI for open source projects.

rubenk · 2018-06-13T08:17:58Z

Perhaps @mfournier has some ideas as well, he's more familiar with our CI infra.

rubenk · 2018-06-13T08:20:32Z

Just curious, did you manage to build these changes with something like mingw on Fedora, or only on cygwin?

SeanCampbell · 2018-06-13T22:42:51Z

I've only built it on Cygwin at this point.

rubenk · 2018-06-14T08:37:58Z

build.sh

+          net_if \
+          poll \
+          recv \
+              regex \


indentation is off

rubenk · 2018-06-14T08:39:37Z

configure.ac

@@ -848,11 +854,17 @@ AC_CHECK_FUNCS([socket],
  [
    AC_CHECK_LIB([socket], [socket],
      [socket_needs_socket="yes"],
-      [AC_MSG_ERROR([cannot find socket() in libsocket])]
+      [
+        AC_CHECK_LIB(gnu, rpl_socket,


please quote the arguments with []

rubenk · 2018-06-14T08:43:04Z

@SeanCampbell could you add a few lines of documentation on how to build this for Windows? Not sure what's the best location, probably the README.

SeanCampbell · 2018-06-15T12:53:39Z

Sure thing, added that in.

tokkee

Generally looks good to me but see a few comments.

I'm not super-happy about all the added conditionals but this is C, so what can you do? ;-)

tokkee · 2018-06-29T13:20:06Z

README

+
+$ INSTALL_DIR="C:/some/other/install/directory" ./build.sh
+
+As of now, the only plugin tested to work on Windows is the `logfile' plugin.


This is useful information but can easily go out of sync. We're also not yet tracking it for other OSes, but maybe we should. I'm wondering if we can have a good way to track and manage this information.

tokkee · 2018-06-29T13:22:21Z

build.sh

-set -x
+    echo "Installing collectd to ${INSTALL_DIR}."
+    TOP_SRCDIR=$(pwd)
+    MINGW_ROOT="/usr/x86_64-w64-mingw32/sys-root/mingw"


Is this path name valid universally? If not, can it be auto-detected?

Yep, looks like you can detect it.

tokkee · 2018-06-29T13:24:19Z

configure.ac

-        [AC_MSG_ERROR([cannot find nanosleep])]
+        [
+          AC_CHECK_FUNCS([rpl_nanosleep],
+            [nanosleep_needs_gnulib="yes"],


This seems to be unused. Do we need it?

I think this is leftover from changes I patched in from kele's port, so it's not needed anymore.

tokkee · 2018-06-29T13:26:27Z

src/daemon/common.c

 #define MSG_DONTWAIT MSG_NONBLOCK
 #endif

-#if !HAVE_GETPWNAM_R
+#ifdef WIN32


Nit: Putting this into an else branch seems cleaner.

tokkee · 2018-06-29T13:31:56Z

👍 from me. @rubenk do you want to merge once you're happy?

SeanCampbell · 2018-07-12T15:29:22Z

@rubenk Do you think any other changes are needed?

tokkee · 2018-07-27T13:18:55Z

Since there were no further comments, I'll merge now.

Thank you for your work @SeanCampbell!

rpv-tomsk · 2018-08-23T08:23:17Z

Hi!

Can you please fix the following warnings:

+ automake --add-missing --copy
Makefile.am:5: warning: pkgdatadir was already defined in condition TRUE, which includes condition BUILD_WIN32 ...
/usr/share/automake-1.15/am/header-vars.am: ... 'pkgdatadir' previously defined here
Makefile.am:6: warning: pkglibdir was already defined in condition TRUE, which includes condition BUILD_WIN32 ...
/usr/share/automake-1.15/am/header-vars.am: ... 'pkglibdir' previously defined here
+ autoconf

?

SeanCampbell · 2018-08-23T14:27:54Z

Hi Pavel,

I opened a PR with the fix here: #2907

Change introduced in df13592 (#2810) This change causes error in 'master-aggregation' hook: ERROR: files left in build directory after distclean: ./liboconfig.la ./libcommon.la ./libmount.la ./libformat_json.la ./libheap.la ./libignorelist.la ./libavltree.la ./libformat_graphite.la ./liblatency.la ./liblookup.la ./libmetadata.la ./libcmds.la

Change introduced in df13592 (#2810) causes build failure on Solaris: src/libcollectdclient/server.c: In function ‘server_multicast_join’: src/libcollectdclient/server.c:103:9: error: unknown field ‘imr_address’ specified in initializer .imr_address.s_addr = INADDR_ANY, .imr_multiaddr.s_addr = sa->s_addr, ^ src/libcollectdclient/server.c:103:69: error: ‘struct sockaddr_in’ has no member named ‘S_un’ .imr_address.s_addr = INADDR_ANY, .imr_multiaddr.s_addr = sa->s_addr, ^ src/libcollectdclient/server.c:102:27: error: missing braces around initializer [-Werror=missing-braces] struct ip_mreq mreq = { ^ src/libcollectdclient/server.c:102:27: note: (near initialization for ‘mreq’) Removed this change as not related to WIN32 platform.

rpv-tomsk · 2018-10-14T11:02:51Z

@SeanCampbell

Hi, Sean!
I added some commits to fix some build issues, they revert some changes which was added in this PR.
Please check them and propose other solutions if they are needed.

Thanks!

make clean && ./build.sh && ./configure && make dist-gzip && ./version-gen.sh && make -s distcheck -j 5

Change introduced in df13592 (#2810) causes build failure on Solaris: src/libcollectdclient/server.c: In function ‘server_multicast_join’: src/libcollectdclient/server.c:103:9: error: unknown field ‘imr_address’ specified in initializer .imr_address.s_addr = INADDR_ANY, .imr_multiaddr.s_addr = sa->s_addr, ^ src/libcollectdclient/server.c:103:69: error: ‘struct sockaddr_in’ has no member named ‘S_un’ .imr_address.s_addr = INADDR_ANY, .imr_multiaddr.s_addr = sa->s_addr, ^ src/libcollectdclient/server.c:102:27: error: missing braces around initializer [-Werror=missing-braces] struct ip_mreq mreq = { ^ src/libcollectdclient/server.c:102:27: note: (near initialization for ‘mreq’) Removed this change as not related to WIN32 platform.

* Update configure.ac / Makefile.am to build for Windows using Cygwin. * Update build.sh to build for Windows. * Base port of the daemon. * Include gnulib as a submodule.

Update configure.ac to configure for Windows.

74a5245

8000

SeanCampbell changed the title ~~Base port to Windows~~ [WiP] Base port to Windows Jun 8, 2018

SeanCampbell force-pushed the campbellsean-winport branch from a7afc59 to 36647db Compare June 8, 2018 00:23

SeanCampbell force-pushed the campbellsean-winport branch from 36647db to 6440b58 Compare June 8, 2018 02:56

SeanCampbell changed the title ~~[WiP] Base port to Windows~~ Base port to Windows Jun 8, 2018

SeanCampbell force-pushed the campbellsean-winport branch 3 times, most recently from 26baff2 to 0a726c9 Compare June 11, 2018 14:05

SeanCampbell added 6 commits June 11, 2018 10:06

Update build.sh to build for Windows.

0208b8a

Update Makefile.am to make for Windows.

4caddc6

Windows port

eb04805

collectd.c

3939968

cmd_windows.c

c114a21

Format changed files

aab3f3f

SeanCampbell force-pushed the campbellsean-winport branch from 0a726c9 to aab3f3f Compare June 11, 2018 14:06

rubenk reviewed Jun 11, 2018

View reviewed changes

SeanCampbell added 2 commits June 11, 2018 18:02

Fixes for base port.

722b3c2

Alphabetize gnulib modules.

58671e0

rubenk reviewed Jun 14, 2018

View reviewed changes

SeanCampbell added 3 commits June 15, 2018 08:36

Quote AC_CHECK_LIB args with []

14b84f6

Fix indentation in build.sh

af675e6

Update README file with instructions for building for Windows.

c99f946

tokkee reviewed Jun 29, 2018

View reviewed changes

SeanCampbell added 5 commits June 29, 2018 12:06

Make definition of MSG_DONTWAIT clearer.

a99dbfe

Remove unneccesary addition to configure.ac.

6003ff8

Update README and build.sh

3a3904d

Terminate #if

494e7f7

Trivial commit

847d265

tokkee merged commit df13592 into collectd:master Jul 27, 2018

SeanCampbell deleted the campbellsean-winport branch July 27, 2018 19:33


		$ INSTALL_DIR="C:/some/other/install/directory" ./build.sh

		As of now, the only plugin tested to work on Windows is the `logfile' plugin.

Base port to Windows #2810

Base port to Windows #2810

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!